Skip to content

Improve binary stripping for Apple platforms#14951

Closed
keith wants to merge 1 commit intobazelbuild:masterfrom
keith:ks/improve-binary-stripping-for-apple-platforms
Closed

Improve binary stripping for Apple platforms#14951
keith wants to merge 1 commit intobazelbuild:masterfrom
keith:ks/improve-binary-stripping-for-apple-platforms

Conversation

@keith
Copy link
Copy Markdown
Member

@keith keith commented Mar 3, 2022

This change implements the suggestions from https://github.com/trybka/scraps/blob/f78a7fdb3a05a0d46ac4e1ba69e223157c966028/Apple_Stripping.md#proposal

Previously stripping for Apple platforms had a few different cases:

  1. Passing --strip=always / --strip=sometimes + fastbuild passes
    -Wl,-S to link invocations
  2. Using the .stripped outputs of some rules like cc_binary takes
    --stripopt into account
  3. Passing --objc_enable_binary_stripping + opt enabled -dead_strip,
    and called strip on linked binaries (passing -x for dylibs and
    similar)

With this change there is a different setup:

  1. The behavior of --strip is unchanged
  2. The behavior of --stripopt is unchanged
  3. -dead_strip is enabled by default for all opt builds
  4. strip -rSTx is invoked for any builds that produced dSYMs
  5. --objc_enable_binary_stripping is now a no-op

This simplifies what flags people have to be aware of to get the right
stripping behavior. This also unifies the stripping behavior for
cc_binary, all users of cc_common.link, and other Apple binary rules
when building for macOS.

@keith
Copy link
Copy Markdown
Member Author

keith commented Mar 3, 2022

cc @trybka

This change implements the suggestions from https://github.com/trybka/scraps/blob/f78a7fdb3a05a0d46ac4e1ba69e223157c966028/Apple_Stripping.md#proposal

Previously stripping for Apple platforms had a few different cases:

1. Passing `--strip=always` / `--strip=sometimes` + fastbuild passes
   `-Wl,-S` to link invocations
2. Using the `.stripped` outputs of some rules like cc_binary takes
   `--stripopt` into account
3. Passing `--objc_enable_binary_stripping` + opt enabled `-dead_strip`,
   and called `strip` on linked binaries (passing `-x` for dylibs and
   similar)

With this change there is a different setup:

1. The behavior of `--strip` is unchanged
2. The behavior of `--stripopt` is unchanged
3. `-dead_strip` is enabled by default for all opt builds
4. `strip -rSTx` is invoked for any builds that produced dSYMs
5. `--objc_enable_binary_stripping` is now a no-op

This simplifies what flags people have to be aware of to get the right
stripping behavior. This also unifies the stripping behavior for
cc_binary, all users of cc_common.link, and other Apple binary rules
when building for macOS.
@keith keith force-pushed the ks/improve-binary-stripping-for-apple-platforms branch from c95af6f to 7d0e116 Compare March 4, 2022 00:29
@keith
Copy link
Copy Markdown
Member Author

keith commented Mar 4, 2022

There are 2 potentially underserved cases here:

  1. People who want stripping but don't use dsyms. This likely isn't an issue since without dsyms they also won't have -g so the binary size impact shouldn't be as significant
  2. People who use dsyms for debugging and would prefer to not strip on every iteration cycle because of the time. Stripping our few hundred mb binary is almost instantaneous, so I'm not sure this is a big issue, but I'm interested to hear from folks on this

@trybka
Copy link
Copy Markdown
Contributor

trybka commented Mar 7, 2022

cc: @googlewalt @jyknight

@sgowroji sgowroji added z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple team-Rules-CPP Issues for C++ rules labels Mar 24, 2022
@brentleyjones
Copy link
Copy Markdown
Contributor

Bump.

@keith keith marked this pull request as ready for review April 16, 2022 03:33
@keith keith requested a review from lberki as a code owner April 16, 2022 03:33
@lberki lberki requested review from comius and googlewalt and removed request for lberki April 19, 2022 08:05
@lberki
Copy link
Copy Markdown
Contributor

lberki commented Apr 19, 2022

@comius I assume Walt is the right person to review this?

@comius
Copy link
Copy Markdown
Contributor

comius commented Apr 19, 2022

@comius I assume Walt is the right person to review this?

Yes.

@comius comius removed their request for review April 19, 2022 08:45
@sgowroji sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 21, 2022
@dflems
Copy link
Copy Markdown
Contributor

dflems commented Jul 15, 2022

Anyone looking into this? Would love to see this land as it removes a lot of dead weight from apple binaries, especially for folks who care about staying under that appstore OTA download limit 😬

@googlewalt
Copy link
Copy Markdown
Contributor

Sorry for not responding sooner. I did look into this internally.

This commit changes several distinct aspects of binary stripping. It would be better to split each distinct aspect into a separate commit. That way:

  • We can better track any incompatibility issues.
  • It would be easier to fix any internal breakages.
  • It would be eaiser to get the change submitted.
  • It would be eaiser to coordinate any changes to the toolchain, for which Google uses one that's different from open source.

Suggestion for how to break this up, in steps:

  1. Migrate the strip action that is currently hardwired in java code to be a part of wrapped clang. Create an interface in crosstool to pass strip flags to wrapped_clang, probably via an environment variable. This change provides several benefits:

    • Primarily, it gives all linking actions the same stripping behavior, whether they are from objc rules, cc rules, or cc starlark api.

    • It allows us to delete several hundred lines of java code. It gives the crosstool better control over the strip action.

    Note, however, this ties the strip action to be run on the same machine as the compiler. TODO(googlewalt): I need to investigate whether this is an issue internally (I suspect not).

  2. Update the flags we pass to strip.

    The default stripping behavior, -u -r, is not safe (See Appendix). This is a "conservative" proposal:

    When -c opt:

    • When --apple_generate_dsym: -S.
    • Otherwise: -x. (This affects local debugging, but that is typically done with -c dbg.)

    emergetools.com suggested -rSTx, but:

    • -x should subsume -S.
    • -r doesn't do anything when -S.
    • It's not clear why -T can safely be done.
  3. Enable linker dead_strip for -c opt. This can be done independently from the above. I tried to do that internally but found breakages, due to:

    Unfortunately, this exacerbates the known brokenness with avoid_deps:

    We have the expectation that if an app and a dylib depend on the same library, we can put that library in the avoid_deps of the app. Then the library won't be in app but will remain in dylib.

    This assumption is broken, because app and dylib may not need the same symbols, so dylib may not link in the file that app needs.

    And dead_strip makes things worse. Now even if dylib links in all the files that app needs, what app needs may get dead stripped.

    A possible path forward:

    • Do dead_strip only for binaries, not dylibs.
    • Bundle loaders and host binaries need to explicitly disable dead_strip via features = ["no_dead_strip"].
    • Other binaries that are dynamic linked against / have symbols looked up via dlsym will also need to explicitly disable dead_strip.

@cpsauer
Copy link
Copy Markdown
Contributor

cpsauer commented Jul 16, 2022

@googlewalt (and others), any chance there's a way we could keep dead_strip on for dylibs, too, in opt builds? [Will explain why below--please say if you think I've misunderstood something.]

Seems to me like the avoid_deps bug you're describing is rooted in dylib not always linking dependencies. By default, all symbols are exported from a dylib, right? So the not-always-linking optimization seems fundamentally buggy in this instance, since it changes the interface of the resulting library.

Regardless, in the absence of specifying the interface of a dylib, nothing should get dead stripped by the linker because all symbols are exported, right? And conversely, things are only stripped when you specify the interface with a symbols list or linker script or whatever, in which case all is well, since you get what you explicitly ask for. Maybe I'm missing something, but it seems offhand like dead_strip would be good to have on by default in opt (rather than exacerbating the root problem, above) because I'd expect it to be a no-op if you don't specify which symbols to export and behave very usefully if you do specify which symbols to export.

Super appreciate all you guys do!


Some related crossrefs in my memory, in case ever useful:
I smiled seeing -x as the base default. I remember this from #13314
Parallel discussion around dead strip being default for opt android (shared libraries), over in #13287.

@googlewalt
Copy link
Copy Markdown
Contributor

cpsauer@ I think you're right; it's been a while but the failures I had were from dead_strip of binaries, not dylibs. So we may be able to keep dead_strip on for dylibs, even if by default it's not likely to be able to strip anything.

@cpsauer
Copy link
Copy Markdown
Contributor

cpsauer commented Jul 21, 2022

Yay! Thanks for reading and being open to it @googlewalt

@oquenchil oquenchil added team-Rules-ObjC Issues for Objective-C maintainers and removed team-Rules-CPP Issues for C++ rules labels Dec 16, 2022
@meteorcloudy meteorcloudy requested a review from a team as a code owner November 13, 2023 10:19
@keith keith marked this pull request as draft February 29, 2024 17:04
@meisterT
Copy link
Copy Markdown
Member

meisterT commented May 8, 2025

Closing this as obsolete. If we still want it, it should be splitted as indicated above

@meisterT meisterT closed this May 8, 2025
@github-actions github-actions Bot removed the awaiting-review PR is awaiting review from an assigned reviewer label May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-Rules-ObjC Issues for Objective-C maintainers z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple

Projects

None yet

Development

Successfully merging this pull request may close these issues.