crate-ify compiler-rt into compiler-builtins #35021
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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. |
| let mut sources = Sources::new(); | ||
| sources.extend(&["absvdi2.c", | ||
| "absvsi2.c", | ||
| "adddf3.c", |
|
FWIW, I believe the name should be |
| @@ -0,0 +1 @@ | |||
| ../compiler-rt/lib/builtins | |||
There was a problem hiding this comment.
I think this may be a symlink? We may want to remove this for now and just assume the location of lib/builtins is what we expect
There was a problem hiding this comment.
You mean having the build.rs directly use the $RUST_REPO/src/compiler-rt/lib/builtins directory?
There was a problem hiding this comment.
Oh nah I mean just referencing ../compiler-rt/... in the paths being compiled
|
Awesome, thanks @japaric! It's probably fine to remove the no_compiler_rt logic in the compiler as well. I don't think it'll break any custom target specs because we should ignore unknown fields I believe. @brson so thinking about this again I'd like to touch briefly again on what the organization here should be. Dependency-wise everything in Rust depends on this crate, or in other words everything generated by rustc. In that sense libcore should depend on this, but in another sense that'll get very annoying once we actually want to write all this in Rust (b/c none of libcore is available, including required lang items). I think I'd personally prefer to have this as a libcore build script for now because:
It also feels a little weird for libcore to not actually be the core, but that's just a vague feeling of mine :). Curious what you think though! |
|
@alexcrichton I think it would make sense to have the builtins somewhere in |
|
@alexcrichton All your points make sense to me. 👍 from me to moving this to a libcore build script. |
|
My main reasons for not wanting it to be part of core is because it is already a self-contained compilation unit, so it should stay its own compilation unit; it's very much tied to rustc's present compiler backend, making it less likely that I do feel pretty strongly that this code does not belong in core. That said I concede that it would be quite challenging to write it in rust without inverting the dependencies. |
|
I guess in a certain light you could say that everything in |
|
@brson I'm basically only worried about the barrier we'll have to defining these things in Rust, I wouldn't want to get into a situation where libcore reexports things from rustc-builtins. I wouldn't mind the alternate route of inverting dependencies and have rustc-builtins depend on libcore. It's only "incorrect" in the sense that libcore may lower to intrinsics that are defined in rustc-builtins. We can paper over that with linker flags, though, so it's solvable and probably won't actually come up in practice. |
|
Let's go with the core buildscript for now. I've adequately registered my distaste for that solution, but any problems associated with it are theoretical and we can deal with them down the road. |
Done. Still missing the Makefiles part. Upon further testing this, I've found that cross compiling rustc for It seems that the problem is that If that's indeed the issue, is there a way to make |
|
@japaric that's probably because we compile with That's... an interesting problem! I don't think we want to export the compiler-rt intrinsics from every application, but they need to be available for further linkages. This works today because you'd just link multiple copies of the intrinsic via Hmm... I guess the "best" solution is to have libstd.so be special in that it's the only dylib exporting those symbols, but that's not exactly a great solution nor do I know a great way to implement that. |
|
I talked with @brson over lunch today, and the conclusion we reached was:
Does that make sense @japaric? This means we'll also have this be a separate crate, but with a custom crate type we can sequence it anywhere on the link line we want so I'm not to worried about the dependency tree. |
|
Oof. Sorry for all the runaround @japaric . |
Eek, couldn't this be done as |
|
@eddyb that part for sure could be done that way, although it wouldn't solve the linking problem. This object file basically wants to be duplicated, whereas all other libs don't. |
|
@alexcrichton I'm thinking of implying that to be able to reach the symbols, they must be available directly when linking the final crate. There's probably a better name that can be used here, I guess. |
|
Eh I'd be fine with that. I don't really mind if it's a new crate type or some other rando attribute, the semantics'll end up being the same either way. |
Done, but I used a crate level attribute instead of a new crate type. Only the Makefiles part is missing. Guess I can't postpone that anymore. rolls up sleeves |
|
Looks good to me! Could the dependency between rustc-builtins and libcore be inverted though? If we have rustc-builtins depend on libcore we'll be able to easily define all the builtins using libcore. |
japaric
changed the title
Done.
If the dependency is inverted, won't all crates have to explicitly depend on rustc_builtins (rustc_builtins has to appear somewhere in the dependency graph)? Or are you that all crates should implicitly depend on rustc_builtins? |
Oh right I was thinking that libstd would depend on rustc-builtins |
In that case, |
|
Hm yeah that's a good point. On the other hand though I might also expect that? @brson how do you feel about that? |
|
@bors: retry On Tue, Sep 13, 2016 at 8:26 AM, bors notifications@github.com wrote:
|
|
@bors: retry |
|
⌛ Testing commit 2656202 with merge 25d34f8... |
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
This commit fixes a test which now needs to explicitly link to the `compiler_builtins` crate as well as makes the `compiler_builtins` crate unstable.
|
@bors: r+ |
|
📌 Commit 848cfe2 has been approved by |
crate-ify compiler-rt into compiler-builtins libcompiler-rt.a is dead, long live libcompiler-builtins.rlib This commit moves the logic that used to build libcompiler-rt.a into a compiler-builtins crate on top of the core crate and below the std crate. This new crate still compiles the compiler-rt instrinsics using gcc-rs but produces an .rlib instead of a static library. Also, with this commit rustc no longer passes -lcompiler-rt to the linker. This effectively makes the "no-compiler-rt" field of target specifications a no-op. Users of `no_std` will have to explicitly add the compiler-builtins crate to their crate dependency graph *if* they need the compiler-rt intrinsics - this is a [breaking-change]. Users of the `std` have to do nothing extra as the std crate depends on compiler-builtins. Finally, this a step towards lazy compilation of std with Cargo as the compiler-rt intrinsics can now be built by Cargo instead of having to be supplied by the user by some other method. closes #34400 --- r? @alexcrichton
|
I spent all day today doing a |
|
It would help if you would post the linker errors. I'm almost certain that something to do with how we invoke linkers is what ended up hiding the symbols. |
|
Sorry, meant to share the output: |
How do I find the linker errors? Are they in the output in my previous comment? FWIW, I still can't use afl.rs with any nightly ever since this PR landed. |
libcompiler-rt.a is dead, long live libcompiler-builtins.rlib
This commit moves the logic that used to build libcompiler-rt.a into a
compiler-builtins crate on top of the core crate and below the std crate.
This new crate still compiles the compiler-rt instrinsics using gcc-rs
but produces an .rlib instead of a static library.
Also, with this commit rustc no longer passes -lcompiler-rt to the
linker. This effectively makes the "no-compiler-rt" field of target
specifications a no-op. Users of
no_stdwill have to explicitly addthe compiler-builtins crate to their crate dependency graph if they
need the compiler-rt intrinsics - this is a [breaking-change]. Users
of the
stdhave to do nothing extra as the std crate dependson compiler-builtins.
Finally, this a step towards lazy compilation of std with Cargo as the
compiler-rt intrinsics can now be built by Cargo instead of having to
be supplied by the user by some other method.
closes #34400
r? @alexcrichton