compiler_builtins: base conditional compilation on the specification #36512
Conversation
of the target rather than on its triple (i.e. its name). As explained in rust-lang#35474, the current conditional compilation logic, which is based on the target triple (e.g. x86_64-unknown-linux-gnu), is not robust in the presence of custom targets as those can have arbitrary triples (e.g. cortex-m3). To fix that, this commit changes the conditional compilation logic to use the specification of the target (e.g. target_arch, target_os, etc.). For that, compiler_builtins build script now depends on the rustc-cfg crate, whose role is to shell out to `rustc --print cfg` and return its output parsed as a `struct`. With the goal of completely removing any direct dependency of the conditional compilation logic on the target triple, this commit also exposes the llvm-target field of the target specification via `rustc --print cfg`. closes rust-lang#35474
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
I'm probably biased but the build script now looks more readable to me. I think it does a better job at conveying its intent. |
Do we not want to expose llvm-features quite yet? If so, I might understand trying to use the llvm triple for now. |
All the
The absence of Also
Perhaps. But I think the (*) AFAICT, this is the case for
|
|
Oh great, the "TARGET" env var isnt confused by cross compiling? When did this happen again? |
|
|
|
I might be getting confused with rust-lang/rfcs#1721 (comment) |
|
Regarding
Is there some other item covered by Regarding things not making it to That said, it seems like it'd be fine to postpone that. This patch is already an improvement on our current situation, and seems unlikely to cause issues if we tweak our llvm-target usage. |
Oh, I see. That or expression is equivalent to target_arch == x86 AFAIK. I wonder how that or expression ended up like that :P.
I pretty much agree. I think the current implementation already does that because I don't see other way how |
|
☔ The latest upstream changes (presumably #36719) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -13,3 +13,4 @@ core = { path = "../libcore" } | |||
|
|
|||
| [build-dependencies] | |||
| gcc = "0.3.27" | |||
| rustc-cfg = { git = "https://github.com/japaric/rustc-cfg", branch = "llvm-target" } | |||
There was a problem hiding this comment.
I had some comments on the other PR, but I'm not 100% sure that this is buying us enough to pull in a dependency. Many of the .contains are now just relevant_piece == "foo", which is good to work over parsed forms but hasn't cause ambiguity problems in the past?
There was a problem hiding this comment.
The rationale is to make this crate more robust at handling custom targets whose triple may be foo or any other arbitrary name. My main motivation was to handle Cortex-M targets which are currently customs target and may be named whatever by downstream users. But if those Cortex-M targets land in tree then I'm OK with not landing this PR.
There was a problem hiding this comment.
I was talking to @japaric about this the other day. This basically does what the extra env vars from rust-lang/rfcs#1721 would do, so it can be considered a stop-gap perhaps.
| @@ -949,6 +950,7 @@ pub fn default_configuration(sess: &Session) -> ast::CrateConfig { | |||
| mk(InternedString::new("target_pointer_width"), intern(wordsz)), | |||
| mk(InternedString::new("target_env"), intern(env)), | |||
| mk(InternedString::new("target_vendor"), intern(vendor)), | |||
| mk(InternedString::new("llvm_target"), intern(llvm_target)), | |||
There was a problem hiding this comment.
This is a pretty weighty target-cfg to add that I'm not sure we'll want to do. Thoughts @brson?
There was a problem hiding this comment.
Note that this is not necessary. We can do conditional compilation for Cortex-M targets just fine without this change iff the Cortex-M are named thumbv* because we have to disambiguate betwen Cortex-M and our others ARM+Linux targets. This means we can no longer name them e.g. armv7m-none-eabi, which may be less strange to users, for who "thumb" may not ring a bell.
There was a problem hiding this comment.
I like this direction actually. I think our target names should just be mere aliases (i.e. just the contents of the target definition matters). As @brson actually has mentioned elsewhere, or current policy of associating fine-grained attributes with triple (via the default targets) unnecessarily clashes with LLVM and other tool.s
|
@brson do you have thoughts on this? I think this'll get more relevant when we do custom compiles of std. My preference currently would be to leave out the addition of the @japaric beyond the cortex targets which are likely to get added to rustc itself, do you know if there are other examples of where this would prevent the need of having to upstream a patch? |
|
@alexcrichton This sort of already landed upstream in japaric/rust-builtins#74 without the llvm_target part of course.
In theory, the 3DS (cf. https://github.com/rust3ds/). But I think this won't work for them without the llvm_target part because their target is named @FenrirWolf Could you do me a favor and try to compile the rustc-builtins crate for the 3DS but with the "c" Cargo feature enabled? As in |
|
Yeah, we don't have the standard library since the cross-compiling toolchain we're using is based on a hacked up newlib without pthreads and some other things. It's homebrew stuff, which is to say it's the height of instability and it's a miracle that it works at all. =P Anyway, this is what I got when trying to compile |
Oh, sorry. You have to checkout the submodule first: |
@alexcrichton On that note, does Cargo checkout the submodules of |
|
@japaric ok, thanks for the info. Also yeah, Cargo should check out submodules in git dependencies. |
|
@japaric My bad. Checked out the submodule and got a different set of errors: http://pastebin.com/FuXKxK0B |
|
@FenrirWolf This time is my bad; I forgot to tell you to configure the compiler. Please Then could you try the following as well? Rename Does it work in any of these scenarios? |
|
@japaric Okay, now we're getting somewhere. After setting CC_3ds, I get this result: If I let Xargo grab libcore for me, it keeps going until it can't find libc (which is expected since Xargo doesn't compile that). |
That's fine. src/bin/intrinsics.rs is actually a test and you are trying to compile its |
|
ping @japaric, should we wait to do this until moving to the upstream copy of compiler-builtins? |
|
This has landed "upstream" (in rust-lang-nursery/compiler-builtins) and will land in this repo when #36992 lands so I'm going to close this. |
of the target rather than on its triple (i.e. its name).
As explained in #35474, the current conditional compilation logic, which
is based on the target triple (e.g. x86_64-unknown-linux-gnu), is not
robust in the presence of custom targets as those can have arbitrary
triples (e.g. cortex-m3).
To fix that, this commit changes the conditional compilation logic to
use the specification of the target (e.g. target_arch, target_os, etc.).
For that, compiler_builtins build script now depends on the rustc-cfg
crate, whose role is to shell out to
rustc --print cfgand return itsoutput parsed as a
struct.With the goal of completely removing any direct dependency of the
conditional compilation logic on the target triple, this commit also
exposes the llvm-target field of the target specification via
rustc --print cfg.closes #35474
r? @alexcrichton
cc @brson
TODOs