Improve binary stripping for Apple platforms

[keith]

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]

cc @trybka

[keith]

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]

cc: @googlewalt @jyknight

[brentleyjones]

Bump.

[lberki]

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

[comius]

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

Yes.

[dflems]

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]

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]

@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 https://github.com/bazelbuild/bazel/pull/13314 Parallel discussion around dead strip being default for opt android (shared libraries), over in https://github.com/bazelbuild/bazel/issues/13287.

[googlewalt]

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]

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