Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustc: Enable embedding LLVM bitcode for iOS #48896

Merged
merged 2 commits into from
Mar 16, 2018

Conversation

alexcrichton
Copy link
Member

This commit updates rustc to embed bitcode in each object file generated by
default when compiling for iOS. This was determined in #35968 as a step
towards better compatibility with the iOS toolchain, so let's give it a spin and
see how it turns out!

Note that this also updates the cc dependency which should propagate this
change of embedding bitcode for C dependencies as well.

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2018
@alexcrichton
Copy link
Member Author

cc @comex, @michaeleiselsc, y'all may want to double check me on this!

@alexcrichton
Copy link
Member Author

@michaeleiselsc
Copy link

i'm still not sure if we want to always have bitcode on, i think it may cause binary bloat for internal distributions, @comex do you know?

@comex
Copy link
Contributor

comex commented Mar 10, 2018

Well, I don't know of anything that would normally strip it out. (In particular, strip doesn't remove bitcode, which makes sense because Xcode runs strip by default when building the archive you'd send to Apple.) I don't know whether any third-party app distribution platforms (for betas or internal corporate apps) have custom tooling to strip it out. I also don't know how much bitcode typically adds to binary sizes. So… no idea :)

I'm building this PR locally and will at least do a basic test of whether I can build a Rust static library and link it into an iOS app with bitcode enabled, without the linker complaining about missing bitcode objects. That wouldn't guarantee that the bitcode is actually reasonable: in particular, rustc currently defaults to building against LLVM 6, while the latest Xcode is based on LLVM 5, and I have no idea what Apple's servers are running. But it's a start.

@michaeleiselsc
Copy link

a few more questions:

  • what happens when the linker receives a mix of bitcode files and non-bitcode files, e.g. because a project doesn't want to use bitcode but is built with a mix of rust and other languages where bitcode is turned off?
  • does the app store assume that bitcode in the executable means that it should use the bitcode for its distribution process, or is there another toggle the user has to use? if it's the former, then we would be forcing people to use the app store bitcode process

@alexcrichton
Copy link
Member Author

@comex thanks for testing!

@michaeleiselsc I'm not so sure myself, I'm relying on y'all for guidance on what you need from rustc :(

@comex
Copy link
Contributor

comex commented Mar 10, 2018

I tried to test it but ran into some issues with the standard library build - in particular, #48906, which I just filed, combined with an odd bug where nm (and maybe other things?) rejects object files that were compiled with -fembed-bitcode-marker. I'll look further tomorrow.

@michaeleiselsc
Copy link

It feels like there are a lot of potential issues with always generating bitcode, so although I can take a look at them, I feel like we should provide an option not to emit bitcode. In fact, because bitcode in Xcode is off by default IIRC, perhaps we should make the default be not to generate bitcode and just provide this as an option

@bors
Copy link
Contributor

bors commented Mar 11, 2018

☔ The latest upstream changes (presumably #48549) made this pull request unmergeable. Please resolve the merge conflicts.

@comex
Copy link
Contributor

comex commented Mar 12, 2018

Okay, I figured out what's going on.

  • LLVM's code for reading object files has a bug that causes it to mishandle -fembed-bitcode-marker objects. It's a pretty obvious bug - e.g. you get an error if you run nm on such objects - but I guess nobody noticed because -fembed-bitcode-marker has only been used by Xcode in limited circumstances. I submitted a patch to LLVM upstream. (This does not affect objects with actual embedded bitcode, i.e. -fembed-bitcode without -marker.)
  • rustc's bootstrap has a bug (filed as bootstrap should set optimization level when calculating cflags #48906) that causes it to build libbacktrace (among other things) with -fembed-bitcode-marker even when optimization is on. Not to be confused with an entirely separate bug I discovered during the process, libbacktrace is always built without optimization #48903, with a different cause, that causes libbacktrace to actually be built without optimization (on all platforms!) :)
  • The aforementioned LLVM bug causes rustc to produce broken static libraries: specifically, the index (as shown by nm --print-armap) is missing all symbols from objects compiled with -fembed-bitcode-marker. If you extract the object files from the archive, the symbols are there, but they're not indexed. (As far as I can tell, this issue doesn't affect libraries created with ar, which makes sense since Apple still ships ar from cctools rather than symlinking it to llvm-ar. But rustc produces static libraries directly using LLVM, without using ar.)
  • So on my test setup, which has rustc make a static library for Xcode to link to, ld complains about undefined symbols for _backtrace_create_state, _backtrace_syminfo, _backtrace_pcinfo. (This is the exact same error @michaeleiselsc encountered in this post, but in his case there was an entirely different cause: he was using --emit=llvm-bc, so libbacktrace wasn't being included at all.)
  • As a hack, if I run ranlib on the library before linking (which should not be necessary), my test setup works! (Well, it works for test builds, which are linked using -fembed-bitcode-marker. If I try an archive build, it naturally complains because libbacktrace doesn't actually have bitcode embedded, i.e. bootstrap should set optimization level when calculating cflags #48906.)

For now, as a workaround for the LLVM bug, I think the best approach is to avoid -fembed-bitcode-marker or equivalent, in both this PR and cc-rs. Either embed bitcode by default even for debug builds, or just disable entirely by default for now. Once the LLVM bug has been fixed and the fix is shipping in Xcode, the default can be changed back.

Also, I agree with @michaeleiselsc that there should be an option to control the behavior.

@michaeleiselsc
Copy link

@comex does this relate to the dsymutil segfault I was getting? It seems that rustc does not always generate good symbols (#46447), so perhaps this is triggering that

@alexcrichton
Copy link
Member Author

Thanks for the investigation @comex! I've updated to fix those various rustbuild issues you pointed out and went ahead and disabled this by default for iOS. It can be enabled with -Z embed-bitcode

@michaelwoerister
Copy link
Member

Ping me when this is ready for review.

* Pass `opt_level(2)` when calculating CFLAGS to get the right flags on iOS
* Unconditionally pass `-O2` when compiling libbacktrace

This should...

Close rust-lang#48903
Close rust-lang#48906
This commit updates rustc to embed bitcode in each object file generated by
default when compiling for iOS. This was determined in rust-lang#35968 as a step
towards better compatibility with the iOS toolchain, so let's give it a spin and
see how it turns out!

Note that this also updates the `cc` dependency which should propagate this
change of embedding bitcode for C dependencies as well.
@alexcrichton
Copy link
Member Author

Ok @michaelwoerister I think this should be ready!

@michaeleiselsc errors like #46447 may have been fixed by #46772, although maybe not as well

".llvmbc\0"
};
llvm::LLVMSetSection(llglobal, section.as_ptr() as *const _);
llvm::LLVMRustSetLinkage(llglobal, llvm::Linkage::PrivateLinkage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is what clang is doing?

@@ -86,6 +86,7 @@ fn main() {

fn build_libbacktrace(host: &str, target: &str) -> Result<(), ()> {
let native = native_lib_boilerplate("libbacktrace", "libbacktrace", "backtrace", ".libs")?;
let cflags = env::var("CFLAGS").unwrap_or_default() + " -fvisibility=hidden -O2";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to bitcode embedding?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of tangentially yeah, but it's otherwise fixing issues that @comex brought up when testing this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I later saw that this is in a separate commit with proper commit message anyway. Thanks for the info!

@michaelwoerister
Copy link
Member

Thanks, @alexcrichton! Looks good to me.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 13, 2018

📌 Commit 0e0f74b has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2018
@alexcrichton alexcrichton changed the title rustc: Embed LLVM bitcode by default on iOS rustc: Enable embedding LLVM bitcode for iOS Mar 15, 2018
@bors
Copy link
Contributor

bors commented Mar 16, 2018

⌛ Testing commit 0e0f74b with merge 3b6412b...

bors added a commit that referenced this pull request Mar 16, 2018
…ster

rustc: Enable embedding LLVM bitcode for iOS

This commit updates rustc to embed bitcode in each object file generated by
default when compiling for iOS. This was determined in #35968 as a step
towards better compatibility with the iOS toolchain, so let's give it a spin and
see how it turns out!

Note that this also updates the `cc` dependency which should propagate this
change of embedding bitcode for C dependencies as well.
@bors
Copy link
Contributor

bors commented Mar 16, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 3b6412b to master...

@bors bors merged commit 0e0f74b into rust-lang:master Mar 16, 2018
@alexcrichton alexcrichton deleted the bitcode-in-object branch March 19, 2018 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants