bazelbuild / rules_apple

Bazel rules to build apps for Apple platforms.
Apache License 2.0
512 stars 269 forks source link

Swift: missing symbols in `ios_framework` bundle #332

Open stristr opened 5 years ago

stristr commented 5 years ago

With most of the heavy lifting done by @steeve (see #223), I made a working ios_framework rule for building dynamic frameworks that can be dropped into vanilla (non-Bazel) Xcode projects. At runtime, I discovered that certain symbols that are present in the object file created by the swift_library rule are missing from the framework binary created by the ios_framework rule.

I believe I traced this to the following weird phenomenon: if a Swift source file contains any ObjC classes, all of its symbols make it into the framework binary, even if they're not bridged to ObjC. But if a Swift source file does not contain any ObjC classes, (say, if it only contains protocols, structs, and classes that don't extend NSObject), the apple_binary compile subcommand is totally stripping out all symbols generated from that source file's corresponding object file. I was able to debug up to this point, but it wasn't obvious to me from the wrapped clang command why this would be happening.

I created a simple repro repository here. The ./repro.sh bash script should print:

WillBeIncluded is in object file
WillBeExcluded is in object file
WillBeIncluded is in framework binary
WillBeExcluded is NOT in framework binary

Is this a known issue?

keith commented 5 years ago

This is an interesting bug. Thanks for the awesome repro case, without that I definitely wouldn't have looked into this.

So the core problem is this limitation of the linker when it comes to Swift code https://bugs.swift.org/browse/SR-6004 the tl;dr of that is that there is no equivalent of -ObjC for Swift. A quick workaround for this is to add alwayslink = 1 to your swift_library targets (note this breaks your repro script because the libraries when using this attribute have a .lo extension instead of .a). Another quick workaround is to pass --linkopt=-all_load (but the former is preferred).

The bazel specific issue here, and how it differs from xcodebuild, is that bazel first builds a static library target from all your .o files, and that's the only thing it passes in the -filelist path/to/params to the final dynamic linking step. In this case the file list contains just foo.a and the linker is lazy. Specifically this means that the linker won't go get all the object files in the archive if it doesn't see that they're used. There's a ld flag that's slightly related to this (details in man ld) called -export_dynamic, but this just inhibits LTO stripping (which Swift seems to even be opting out of, but I'm not sure about that), it doesn't force loading more unused object files (this is what -all_load) does. So the reason Xcode doesn't have this problem is its -filelist argument contains each object file explicitly, so the linker happily loads each one, and then keeps the symbols.

I think one possible thing we could do in bazel is pass -all_load for everyone using ios_framework since if you're expecting a dynamic framework, you likely don't want to lose any symbols like this because you don't know what your consumers will use. I could see some arguments against this, but obviously this case is a very good one for it. I can look at how much work this would be to add (if it would be accepted) but in the meantime you can use one of the workarounds mentioned above.

stristr commented 5 years ago

Brilliant! Thanks for the great explanation. I confirmed alwayslink does solve this, and after looking at what that flag does I even get why it works 😄!

Agreed that handling this automatically with default linkopts makes a lot of sense for my use case. In general, if it helps make the case: for teams using Swift, ensuring the binary outputs from ios_[static_]framework rules are suitable for dropping into arbitrary Xcode projects unlocks "gradual adoption" in a broad sense. Along those lines, the work from #223 (prebuilt_swift_static_framework) really should live in the core rules as well. ios_framework should "just work" with Swift, Objective-C, and mixed sources.

sergiocampama commented 5 years ago

One thing I'd like to comment is that ios_framework is not a distribution solution, but a packaging solution, while ios_static_framework is meant only for distribution.

We already do a similar thing to -all_load but for Objc, which is the -Objc flag. This means that everything ObjC is kept, and was introduced because categories were being dropped, but over time we've found that we'd actually like to remove this and I think some people are looking into propagating .o files instead of .a.

Another thing to consider is that C code gets deadstripped from dylibs if not being used and not marked as public/exportable, so that adds another vector of issues for missing symbols.

We don't currently have proposed solution and I don't believe anyone's actively working on this given that workarounds are available.

kastiglione commented 5 years ago

we've found that we'd actually like to remove this

@sergiocampama what has made you want to remove use of -ObjC?

some people are looking into propagating .o files instead of .a

What's the difference between this and -all_load? Many apps are sensitive to size, and while these solve missing symbols they could cause added bloat.

sergiocampama commented 5 years ago

We'd like to remove -ObjC because it also adds bloat since it imports all code inside an archive that has any ObjC code that could otherwise be dead stripped, but it allows the linking of category-only objects. Moving to .os instead of .as would allow removing the -ObjC flag and an avenue for possible bloat as well.

kastiglione commented 5 years ago

Passing .o files to ld causes them to be added to the binary. Passing all inputs as .o files would be the same as -all_load, which is worse for bloat than -ObjC.

kastiglione commented 5 years ago

FWIW, I previously came up with a technique to solve the ObjC category linking problem, which works for linking apps. The approach is to annotate both category declarations and definitions with a macro. The definition gets a symbol that can be linked to (A), and the declaration gets a weak symbol that links to the one in the definition (&A). Then, if any file imports the category header, it will cause the linker to load the object file that contains the category definition via the associated symbol (A). This allows .a files to contain categories, and be linked without -ObjC. Here's an example: https://github.com/Instagram/IGListKit/pull/957/files

steeve commented 4 years ago

I just got this issue too, and making the swift_library.alwayslink = True fixed it. Perhaps ios_framework should consume all depending libraries as alwayslink ?

liuliu commented 4 years ago

Any idea how to make alwayslink propagate such that ios_framework will do the right thing if it is the direct dependency? I do see the value to not propagate all the way back, but it seems just propagate to the direct dependency is useful.

keith commented 4 years ago

I would expect setting alwayslink = True on the swift_library would correctly propagate to the ios_framework

liuliu commented 4 years ago

@keith it is a bit different in my case, I want it to propagate the other way, i.e., for the rule that will generate the final .so / .dylib, it will propagate back so any swift_library the final rule depends on has alwayslink = True set. Any examples how to make that happen?

keith commented 4 years ago

Ah got it, adding -all_load to the linkopts of the dylib target will probably work then

liuliu commented 4 years ago

Yeah. Found this rabbit hole actually related to bigger Bazel issue: https://github.com/bazelbuild/bazel/issues/7362

The -all_load would work on Apple platform but not on the Linux (yeah, I am doing some weird things with Swift on Linux :P). I think the Bazel team will resolve it with the decoupling of rules_cc. Ideally, ios_framework probably should default package all direct dependencies (i.e. whole archive all direct dependencies). And for transitive dependencies, dead code elimination should still work. That seems more "natural" for a static language like Swift.

keith commented 4 years ago

Is there no equivalent flag for gold?

keith commented 4 years ago

ios_framework isn't meant for packaging so it doesn't need to link all dependents. ios_static_framework does do a whole archive link.

liuliu commented 4 years ago

Somehow it doesn't work with: clang -fuse-ld=gold -all_load aSwiftPackaged.a.

Cool, thanks for the clarification on the ios_framework! I think my use case is niche enough will wait on how they work with rules_cc and we can make platform independent changes in rules_swift accordingly.

keith commented 4 years ago

The flag probably differs, you probably want --whole-archive

liuliu commented 4 years ago

Yeah, that has some whole other issues (multiple definitions due to some transitive dependencies), and even just --allow-multiple-definitions, it still doesn't package properly.

jerrymarino commented 2 years ago

I came across this issue and seemed to be having the same problem with a test using symbols in a test host .app that aren't used in the main app. Similar to the issue reported here, the app built under Xcode gets symbols in the test host's binary because sources were linked as object files.

For my case, it was possible workaround this in LD with the -force_load flag. I came up with a rule to force load the deps only. In other situations, I wouldn't use alwayslink because the deps are used in other places

swift_libary(name="app-bin", srcs=["some.swift", "main.swift"])
ios_application(name="app", deps=["app-bin"])

Also, in a large single module app, Bazel should also link the object files directly to the app for performance reasons e.g. not linking 2x. This would also be addressed with object file propagation!

You could probably take the rule in rules_ios as-is or re-work into a PR: https://github.com/bazel-ios/rules_ios/pull/358