Uh oh!
There was an error while loading. Please reload this page.
conditionally modify darwin targets to macosx targets with versions - #60378
Merged
Conversation
rust-highfive
commented
Apr 29, 2019
Contributor
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
michaelwoerister
commented
Apr 30, 2019
Member
cc me |
michaelwoerister
commented
Apr 30, 2019
Member
Ideally we would run the cross-language LTO tests for at least one macOS job. @rust-lang/infra, is there any chance we can enable building Clang (via |
michaelwoerister
commented
Apr 30, 2019
Member
(see https://github.com/rust-lang/rust/blob/master/src/ci/docker/x86_64-gnu-debug/Dockerfile for how we run those tests for Linux) |
alexcrichton
commented
Apr 30, 2019
Member
I don't think we have enough resources on CI to run more tests on OSX right now (especially building lots more LLVM binaries). If it works with what we have seems reasonable to turn on but otherwise we probably can't budget it at this time. TBH it seems like this would have been good to discover the original bug but other than that it seems really unlikely to catch regressions. We've already got tests on other platforms to catch architectural problems with LTO, and a new OSX-specific bug coming up seems like it'd be pretty rare. |
michaelwoerister
commented
May 2, 2019
Member
I know there are resource constraints but in general I don't agree. Ideally I'd like every stable feature to be at least smoke-tested on every tier 1 platform. That being said, I don't want to block this bugfix. @alexcrichton, would you be able to review this PR? Or do you know who'd be a good fit? The changes look good to me and there test cases, but I know virtually nothing about macOS platform conventions. |
alexcrichton
commented
May 2, 2019
Member
@bors: r+ Looks good to me as well! |
froydnj
commented
May 4, 2019
ContributorAuthor
Hm, @bors did not seem to notice the r+? |
eddyb
commented
May 4, 2019
Contributor
@bors r=alexcrichton (maybe it doesn't like the |
bors
commented
May 4, 2019
Collaborator
📌 Commit 21fdfb7236e55195d74145973e56508904f29e82 has been approved by |
bors
commented
May 4, 2019
Collaborator
⌛ Testing commit 21fdfb7236e55195d74145973e56508904f29e82 with merge df1dd3204592604a93b3cfae8d9630ba61dca6a1... |
bors
commented
May 4, 2019
Collaborator
💔 Test failed - checks-travis |
rust-highfive
commented
May 4, 2019
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Member
@bors r- This has broken address-sanitizer support. |
froydnj
commented
May 6, 2019
ContributorAuthor
My plan here is to change the code that does the target triple checking: rust/src/librustc_metadata/creader.rs Lines 771 to 792 in c3b8ab5 to ask |
alexcrichton
commented
May 6, 2019
Member
Hm actually that's looking up in those tables with |
froydnj
commented
May 6, 2019
ContributorAuthor
Ah, that's a good point! You could make the tables contain |
froydnj
commented
May 6, 2019
ContributorAuthor
Ah, I see, |
This behavior matches clang's behavior, and makes cross-language LTO possible. Fixesrust-lang#60235.
froydnjforce-pushed
the
apple-target-modifications
branch
from
May 7, 2019 15:11
21fdfb7 to
97ba4c9Comparemichaelwoerister
commented
May 8, 2019
Member
bors
commented
May 8, 2019
Collaborator
📌 Commit 97ba4c9 has been approved by |
bors
commented
May 8, 2019
Collaborator
bors added a commit
that referenced
this pull request
May 8, 2019
…oerister conditionally modify darwin targets to macosx targets with versions We need this behavior so that Rust LLVM IR objects match the target triple for Clang LLVM IR objects. This matching then convinces the linker that yes, you really can do cross-language LTO with objects from different compilers. The newly-added tests seem to pass locally on x86_64-unknown-linux-gnu. I haven't done a full test run or tried the new compiler in an cross-language LTO setup yet.
bors
commented
May 8, 2019
Collaborator
☀️ Test successful - checks-travis, status-appveyor |
ehuss
commented
May 9, 2019
Contributor
This is causing warnings for me for very basic projects. For example: fnmain(){}This ends up breaking some of Cargo's testsuite. |
alexcrichton
commented
May 9, 2019
Member
I suspect this warning is printed by LLVM's own linker, @froydnj are you able to take a look into this and how to suppress/ignore the warnings? |
froydnj
commented
May 9, 2019
ContributorAuthor
I can reproduce the issue (only with Setting Since |
alexcrichton
commented
May 9, 2019
Member
Yeah I think the issue is that rustc from our nightlies is built with MACOSX_STD_DEPLOYMENT_TARGET set to 10.7 which means the bitcode for libstd itself is set to 10.7, while the bitcode for user-generated code via Cargo has no version at all (causing the mismatch). I'd be wary to switch to 10.7 everywhere because I don't really fully understand the implications of doing so. Even if we defaulted to 10.7 the warning would come up if the user configured to 10.9 for example during LTO. Perhaps we could patch the listed target triple for a module on OSX when we LTO them? Basically just reset the target on darwin to whatever the current target is during rustc's compilation? That would meant that libstd would get updated to x86_64-apple-darwin (no version) during normal LTO, and otherwise it would be switched to x86_64-apple-macosx10.9.0 if configured |
froydnj
commented
May 9, 2019
ContributorAuthor
I think this wouldn't happen, because AFAICT--just by looking at the bitcode files, not by examining the LLVM code--the previously 10.7 bitcode files get munged into 10.9 bitcode files when
I think this could work. What's not clear to me is whether |
alexcrichton
commented
May 9, 2019
Member
Oh so if we defaulted all OSX targets to The modules are linked here, and it's true yeah I'm just assuming that |
froydnj
commented
May 10, 2019
ContributorAuthor
My understanding is that this is incorrect: we have libstd with bitcode for 10.7 today due to the compilation environment for Rust releases. Defautling to Defaulting to I will try to verify that understanding today. |
alexcrichton
commented
May 10, 2019
Member
I believe this is true of user code, but unless we rewrite the target listed in the libstd bytecode I don't think that this is true of libstd code? (in that libstd will mention 10.7 no matter what unless it's changed after-the-fact) |
froydnj
commented
May 13, 2019
ContributorAuthor
For users that have Note that the check for triples is And the So it would be OK (and possibly more correct, even) to default to |
alexcrichton
commented
May 13, 2019
Member
Fascinating! Thanks for digging and finding that @froydnj! |
froydnj added a commit
to froydnj/rust
that referenced
this pull request
May 13, 2019
Over in rust-lang#60378, we made `rustc` switch LLVM target triples dynamically based on the `MACOSX_DEPLOYMENT_TARGET` environment variable. This change was made to align with `clang`'s behavior, and therefore make cross-language LTO feasible on OS X. Otherwise, `rustc` would produce LLVM bitcode files with a target triple of `x86_64-apple-darwin`, `clang` would produce LLVM bitcode files with a target triple of `x86_64-apple-macosx$VERSION`, and the linker would complain. This change worked fine, except for one corner case: if you didn't have `MACOSX_DEPLOYMENT_TARGET` set, and you wanted to do LTO on just Rust code, you'd get warning messages similar to: ``` warning: Linking two modules of different target triples: ' is 'x86_64-apple-macosx10.7.0' whereas 'main.7rcbfp3g-cgu.4' is 'x86_64-apple-darwin' ``` This message occurs because libstd is compiled with `MACOSX_DEPLOYMENT_TARGET` set to 10.7. The LLVM bitcode distributed in libstd's rlibs, then, is tagged with the target triple of `x86_64-apple-macosx10.7.0`, while the bitcode `rustc` produces for "user" code is tagged with the target triple of `x86_64-apple-darwin`. It's not good to have LTO on just Rust code (probably much more common than cross-language LTO) warn by default. These warnings also break Cargo's testsuite. This change defaults to acting as though `MACOSX_DEPLOYMENT_TARGET` was set to 10.7. "user" code will then be given a target triple that is equivalent to the target triple libstd bitcode is already using. The above warning will therefore go away. `rustc` already assumes that compiling without `MACOSX_DEPLOYMENT_TARGET` means that we're compiling for a target compatible with OS X 10.7 (e.g. that things like TLS work properly). So this change is really just making things conform more closely to the status quo. (It's also worth noting that before and after this patch, compiling with `MACOSX_DEPLOYMENT_TARGET` set to, say, 10.9, works just fine: target triples with an "apple" version ignore OS versions when checking compatibility, so bitcode with a `x86_64-apple-macosx10.7.0` triple works just fine with bitcode with a `x86_64-apple-macosx10.9.0` triple.)
nikomatsakis
commented
May 14, 2019
Contributor
I'm nominated for beta at the request of FF team -- this is blocking them from adopting cross-language ThinLTO. (This would have to be backported with #60788, which hasn't landed yet.) |
Centril added a commit
to Centril/rust
that referenced
this pull request
May 14, 2019
…followup, r=estebank default to $ARCH-apple-macosx10.7.0 LLVM triple for darwin targets Over in rust-lang#60378, we made `rustc` switch LLVM target triples dynamically based on the `MACOSX_DEPLOYMENT_TARGET` environment variable. This change was made to align with `clang`'s behavior, and therefore make cross-language LTO feasible on OS X. Otherwise, `rustc` would produce LLVM bitcode files with a target triple of `x86_64-apple-darwin`, `clang` would produce LLVM bitcode files with a target triple of `x86_64-apple-macosx$VERSION`, and the linker would complain. This change worked fine, except for one corner case: if you didn't have `MACOSX_DEPLOYMENT_TARGET` set, and you wanted to do LTO on just Rust code, you'd get warning messages similar to: ``` warning: Linking two modules of different target triples: ' is 'x86_64-apple-macosx10.7.0' whereas 'main.7rcbfp3g-cgu.4' is 'x86_64-apple-darwin' ``` This message occurs because libstd is compiled with `MACOSX_DEPLOYMENT_TARGET` set to 10.7. The LLVM bitcode distributed in libstd's rlibs, then, is tagged with the target triple of `x86_64-apple-macosx10.7.0`, while the bitcode `rustc` produces for "user" code is tagged with the target triple of `x86_64-apple-darwin`. It's not good to have LTO on just Rust code (probably much more common than cross-language LTO) warn by default. These warnings also break Cargo's testsuite. This change defaults to acting as though `MACOSX_DEPLOYMENT_TARGET` was set to 10.7. "user" code will then be given a target triple that is equivalent to the target triple libstd bitcode is already using. The above warning will therefore go away. `rustc` already assumes that compiling without `MACOSX_DEPLOYMENT_TARGET` means that we're compiling for a target compatible with OS X 10.7 (e.g. that things like TLS work properly). So this change is really just making things conform more closely to the status quo. (It's also worth noting that before and after this patch, compiling with `MACOSX_DEPLOYMENT_TARGET` set to, say, 10.9, works just fine: target triples with an "apple" version ignore OS versions when checking compatibility, so bitcode with a `x86_64-apple-macosx10.7.0` triple works just fine with bitcode with a `x86_64-apple-macosx10.9.0` triple.)
pnkfelix
commented
May 16, 2019
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We need this behavior so that Rust LLVM IR objects match the target triple for Clang LLVM IR objects. This matching then convinces the linker that yes, you really can do cross-language LTO with objects from different compilers.
The newly-added tests seem to pass locally on x86_64-unknown-linux-gnu. I haven't done a full test run or tried the new compiler in an cross-language LTO setup yet.