default to $ARCH-apple-macosx10.7.0 LLVM triple for darwin targets #60788
Conversation
This file contains 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
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
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 |
|
@froydnj you'll have to update two tests @rust-lang/compiler who's more familiar with the state of the Darwin/MacOs arch to review this? The change itself looks good to me. |
alexcrichton
approved these changes
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.)
froydnj
force-pushed
the
apple-target-modifications-followup
branch
from
3127dce
to
7e94f9c
Compare
Doh, I thought about updating those tests, and then completely forgot to do it before pushing the PR! Expectation lines for the tests updated. |
|
@bors r+ rollup |
|
📌 Commit 7e94f9c has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 500, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
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.)
bors
added a commit
that referenced
this pull request
May 14, 2019
Rollup of 9 pull requests Successful merges: - #60130 (Add implementations of last in terms of next_back on a bunch of DoubleEndedIterators) - #60443 (as_ptr returns a read-only pointer) - #60444 (forego caching for all participants in cycles, apart from root node) - #60719 (Allow subdirectories to be tested by x.py test) - #60780 (fix Miri) - #60788 (default to $ARCH-apple-macosx10.7.0 LLVM triple for darwin targets) - #60799 (Allow late-bound regions in existential types) - #60808 (Improve the "must use" lint for `Future`) - #60819 (submodules: update clippy from 3710ec5 to ad3269c) Failed merges: r? @ghost
|
This breaks https://github.com/CraneStation/target-lexicon/ (https://travis-ci.org/bjorn3/rustc_codegen_cranelift/jobs/532621583#L358) and other crates parsing the target name cargo passes in the cc @sunfishcode |
|
@bjorn3 can you expand on the breakage? This is intended to be a purely internal change having no user-visible effects in terms of target strings showing up. |
|
Before this PR cargo filled |
|
Can you provide a standalone example which exhibits the issue? |
|
Seems like I misinterpreted the error. This issue is not during compilation of |
|
beta-nominating as coupled with PR #60378 |
pnkfelix
added
the
beta-nominated
|
discussed at T-compiler meeting. Declining to backport: this landed only very recently, and the beta-to-stable promotion is imminent, which means that this patch would be getting very little time in the wild for evaluation prior to hypothetically ending up in stable. That, coupled with the facts that 1. there is at least some evidence that this caused problem in the wild, and 2. we believe the FF team is able to use beta rather than stable for their bootstrapping, if necessary, leads us to decide that the risk of a backport here is not justified. |
pnkfelix
removed
the
beta-nominated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Over in #60378, we made
rustcswitch LLVM target triples dynamicallybased on the
MACOSX_DEPLOYMENT_TARGETenvironment variable. Thischange was made to align with
clang's behavior, and therefore makecross-language LTO feasible on OS X. Otherwise,
rustcwould produceLLVM bitcode files with a target triple of
x86_64-apple-darwin,clangwould produce LLVM bitcode files with a target triple ofx86_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_TARGETset, and you wanted to do LTO on just Rustcode, you'd get warning messages similar to:
This message occurs because libstd is compiled with
MACOSX_DEPLOYMENT_TARGETset to 10.7. The LLVM bitcode distributed inlibstd's rlibs, then, is tagged with the target triple of
x86_64-apple-macosx10.7.0, while the bitcoderustcproduces 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_TARGETwasset 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.
rustcalready assumes that compiling withoutMACOSX_DEPLOYMENT_TARGETmeans that we're compiling for a targetcompatible 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_TARGETset to, say, 10.9, works just fine: targettriples with an "apple" version ignore OS versions when checking
compatibility, so bitcode with a
x86_64-apple-macosx10.7.0triple works justfine with bitcode with a
x86_64-apple-macosx10.9.0triple.)