Switch to rust-lang-nursery/compiler-builtins by alexcrichton · Pull Request #42899 · rust-lang/rust

alexcrichton · 2017-06-25T17:55:00Z

This commit migrates the in-tree libcompiler_builtins to the upstream version
at https://github.com/rust-lang-nursery/compiler-builtins. The upstream version
has a number of intrinsics written in Rust and serves as an in-progress rewrite
of compiler-rt into Rust. Additionally it also contains all the existing
intrinsics defined in libcompiler_builtins for 128-bit integers.

It's been the intention since the beginning to make this transition but
previously it just lacked the manpower to get done. As this PR likely shows it
wasn't a trivial integration! Some highlight changes are:

The PR Test with the 'c' feature enabled on CI compiler-builtins#166 contains a number of fixes
across platforms and also some refactorings to make the intrinsics easier to
read. The additional testing added there also fixed a number of integration
issues when pulling the repository into this tree.
LTO with the compiler-builtins crate was fixed to link in the entire crate
after the LTO process as these intrinsics are excluded from LTO.
Treatment of hidden symbols was updated as previously the
#![compiler_builtins] crate would mark all symbol imports as hidden
whereas it was only intended to mark exports as hidden.

rust-highfive · 2017-06-25T17:55:10Z

r? @nikomatsakis

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

alexcrichton · 2017-06-25T18:00:03Z

cc @japaric

alexcrichton · 2017-06-25T18:15:22Z

Note that some test updates and treatment of LTO are the same fixes as in #42727, just pulled over here as well to get tests passing.

nikomatsakis

This looks pretty good to me, but I'm not intimately familiar with how this part of the system is all interconnected. Perhaps @eddyb, @arielb1, or @nagisa wants to take a look?

nikomatsakis · 2017-06-26T15:21:39Z

This seems probably like a better place, but I'm curious: why move this logic here from where it was before?

Ah an excellent question! Sorry this was a bit of a last minute change and forgot to mention it in the PR description. In the previous location this block would set everything with a hidden visibility, including function imports. What we wanted, however, is only our exported symbols to have hidden visibility, not the imports we had.
This manifest itself as failures when we tried to import memcpy as a hidden symbol from libc.so on ARM, which apparently failed. The intention here was to move this logic to where Rust functions are defined, not where every LLVM function is defined.
Is there a better place for this, though? I put it roughtly around where we seemed to extract function attributes and apply LLVM attributes, but I'd be fine placing this elsewhere.

This seems like roughly the right spot to me.

nagisa · 2017-06-26T20:40:14Z

Does this work if --crate-name=foo is used?

No, but Cargo doesn't do that. This is only intended to ever work with Cargo.

nagisa · 2017-06-26T20:44:33Z

Why not ccx.sess.cstore.is_compiler_builtins()?

Because the previous version didn't do that. I'll switch to using it.

Oh that's actually intended for foreign crates, not the local crate.

nagisa · 2017-06-26T20:45:43Z

The PR does not remove the clibrary.rs itself, does it?

Mark-Simulacrum · 2017-06-27T14:51:32Z

This needs to also update the init_repo script as in #42928.

alexcrichton · 2017-06-27T16:45:58Z

@aidanhs mind helping out with that? The failure mode here is that the src/libcompiler_builtins folder is switching from a folder to a git submodule, which I believe the init_repo.sh script doesn't handle just yet. Do you have a suggestion on the best way to fix this? Perhaps just a local workaround to blow away the libcompiler_builtins folder temporarily?

aidanhs · 2017-06-27T17:09:35Z

I'm not by a computer to try it out, but I think you should just be able to change if [ ! -d "$cache_src_dir/$module" ] to if [ ! -e "$cache_src_dir/$module/.git" ] - this will do a more robust check for a submodule, rather than assuming it's a git repo if the directory exists.

aidanhs · 2017-06-27T17:16:01Z

As an aside, it does make me a bit sad that nested submodules are enabled unconditionally because it will now pointlessly clone the rust installer test repos.

alexcrichton · 2017-06-27T22:20:37Z

@aidanhs ok thanks! I'm now dubious this'll work on the bots, my guess is that the requirement for compiler-builtins to be recursive is going to break this...

aidanhs · 2017-06-27T22:29:13Z

@alexcrichton you did -d rather than -e, which won't work on some git versions.

You should also take the final two --recursive additions from https://github.com/rust-lang/rust/pull/42928/files#diff-4c597da51e91a8853ee60a8733858f71 (there's no point doing it when initialising submodules in the cache because they won't be used as a reference).

alexcrichton · 2017-06-30T07:03:07Z

@bors: r=nikomatsakis

bors · 2017-06-30T07:03:08Z

📌 Commit 809c05c has been approved by nikomatsakis

aidanhs · 2017-06-30T12:56:20Z

@bors r-

[00:02:39] tidy error: /checkout/src/ci/init_repo.sh:72: line longer than 100 chars
[00:02:40] some tidy checks failed

alexcrichton · 2017-06-30T22:25:17Z

@bors: r=nikomatsakis

bors · 2017-06-30T22:25:18Z

📌 Commit ae33ff5 has been approved by nikomatsakis

bors · 2017-07-01T05:45:37Z

⌛ Testing commit ae33ff516b87c336a61a64aac29fcf7188479ecf with merge 0125d49203c5da649b2b4342e02f30f890121f29...

bors · 2017-07-01T05:52:05Z

💔 Test failed - status-appveyor

kennytm · 2017-07-01T07:07:28Z

Failed to build profiler on MSVC. Legit.

error: failed to run custom build command for `profiler_builtins v0.0.0 (file:///C:/projects/rust/src/libprofiler_builtins)`
process didn't exit successfully: `C:\projects\rust\build\x86_64-pc-windows-msvc\stage0-std\release\build\profiler_builtins-7f008c34aad7d625\build-script-build` (exit code: 101)
...
running: "C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\VC\\bin\\amd64\\cl.exe" "/nologo" "/MT" "/O2" "/Zl" "/Dstrdup=_strdup" "/Dopen=_open" "/Dfdopen=_fdopen" "/FoC:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\stage0-std\\x86_64-pc-windows-msvc\\release\\build\\profiler_builtins-bcaf90ff1dc76203\\out\\../libcompiler_builtins/compiler-rt/lib/profile\\WindowsMMap.o" "/c" "../libcompiler_builtins/compiler-rt/lib/profile\\WindowsMMap.c"
WindowsMMap.c
../libcompiler_builtins/compiler-rt/lib/profile\WindowsMMap.c(42): error C2065: 'DWORD': undeclared identifier
../libcompiler_builtins/compiler-rt/lib/profile\WindowsMMap.c(42): error C2146: syntax error: missing ';' before identifier 'flProtect'
../libcompiler_builtins/compiler-rt/lib/profile\WindowsMMap.c(42): error C2065: 'flProtect': undeclared identifier
../libcompiler_builtins/compiler-rt/lib/profile\WindowsMMap.c(45): error C2065: 'flProtect': undeclared identifier
...

bors · 2017-07-06T04:50:20Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 8cab2c7 to master...

japaric · 2017-07-06T04:57:14Z

@alexcrichton you are my hero. 🥇

Don't include custom chkstk on MSVC MSVC includes its own __chkstk so these aren't used. These aren't included in compiler-rt: https://github.com/llvm-mirror/compiler-rt/blob/cb42103777b0fa8b8f38a7bfe5592ed8e3e48388/lib/builtins/CMakeLists.txt#L224-L284. They also weren't included in compiler-builtins: https://github.com/rust-lang/rust/blob/1685c9298685f73db4fe890c1ed27b22408aaad7/src/libcompiler_builtins/build.rs#L255-L293 until rust-lang/rust#42899.

After the work in rust-lang#42899, it no longer auto-discovers it.

The docs for the `compiler_builtins_lib` library feature were removed in rust-lang#42899. But, though the `compiler_builtins` library has been migrated out-of-tree, the feature remains, and is needed to use the stand-alone crate. We reintroduce the docs for the feature, and add a reference to them when describing how to create a `no_std` executable.

…ichton Document use of `compiler_builtins` with `no_std` binaries See discussion in rust-lang#43264. The docs for the `compiler_builtins_lib` feature were removed in PR rust-lang#42899. But, though the `compiler_builtins` library has been migrated out-of-tree, the language feature remains, and is needed to use the stand-alone crate. So, we reintroduce the docs for the feature, and add a reference to them when describing how to create a `no_std` executable.

`src/compiler-rt` was moved out of tree in rust-lang#42899.

rust-highfive assigned nikomatsakis Jun 25, 2017

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2017

nikomatsakis approved these changes Jun 26, 2017
View reviewed changes

nagisa reviewed Jun 26, 2017
View reviewed changes

Comment threadsrc/test/run-pass/lib-defaults.rs Outdated
nagisaJun 26, 2017
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR does not remove the clibrary.rs itself, does it?

alexcrichton force-pushed the compiler-builtins branch from 9c427f1 to 22d25daCompare June 26, 2017 21:09

nagisa mentioned this pull request Jun 27, 2017
Clone submodules recursively #42928
Closed

alexcrichton force-pushed the compiler-builtins branch from 22d25da to 8c5aeddCompare June 27, 2017 22:20

alexcrichton force-pushed the compiler-builtins branch from 8c5aedd to 809c05cCompare June 30, 2017 07:02

alexcrichton force-pushed the compiler-builtins branch 2 times, most recently from eede01e to ae33ff5Compare June 30, 2017 22:25

bors merged commit 7e6c9f3 into rust-lang:masterJul 6, 2017

bors mentioned this pull request Jul 6, 2017
rustc: Implement stack probes for x86 #42816
Merged

petrochenkov mentioned this pull request Jul 6, 2017
A number of tests fails with undefined references to rust_eh_unwind_resume when codegen-units > 1 #43095
Closed

ollie27 mentioned this pull request Jul 12, 2017
Don't include custom chkstk on MSVC rust-lang/compiler-builtins#179
Merged

ranweiler mentioned this pull request Jul 19, 2017
Document use of compiler_builtins with no_std binaries #43342
Merged

ranweiler added a commit to ranweiler/rust that referenced this pull request Jul 21, 2017
Tell tidy about compiler_builtins_lib feature
9d0946a
After the work in rust-lang#42899, it no longer auto-discovers it.

alexcrichton deleted the compiler-builtins branch July 24, 2017 23:34

kennytm mentioned this pull request Aug 12, 2017
[gitignore] Add compiler-rt and rust-installer #43817
Closed

kennytm mentioned this pull request Aug 20, 2017
ThreadSanitizer broke between nightly build 07-06 and 07-07 #44002
Closed

hanna-kruppe mentioned this pull request Sep 13, 2017
Reloading a dylib stops working when moving from 1.19.0 to 1.20.0 #44365
Closed

kennytm mentioned this pull request Jan 22, 2018
Performance regressions of compiled code over the last year #47561
Open

frewsxcv mentioned this pull request Feb 17, 2018
Remove section about compiler-rt in COPYRIGHT. #48305
Closed

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 17, 2018
Remove section about compiler-rt in COPYRIGHT.
b7617a0
`src/compiler-rt` was moved out of tree in rust-lang#42899.

Uh oh!

Uh oh!

Conversation

alexcrichton commented Jun 25, 2017

Uh oh!

rust-highfive commented Jun 25, 2017

Uh oh!

alexcrichton commented Jun 25, 2017

Uh oh!

alexcrichton commented Jun 25, 2017

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented Jun 27, 2017

Uh oh!

alexcrichton commented Jun 27, 2017

Uh oh!

aidanhs commented Jun 27, 2017

Uh oh!

aidanhs commented Jun 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Jun 27, 2017

Uh oh!

aidanhs commented Jun 27, 2017

Uh oh!

alexcrichton commented Jun 30, 2017

Uh oh!

bors commented Jun 30, 2017

Uh oh!

aidanhs commented Jun 30, 2017

Uh oh!

alexcrichton commented Jun 30, 2017

Uh oh!

bors commented Jun 30, 2017

Uh oh!

bors commented Jul 1, 2017

Uh oh!

bors commented Jul 1, 2017

Uh oh!

kennytm commented Jul 1, 2017

Uh oh!

bors commented Jul 6, 2017

Uh oh!

japaric commented Jul 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

aidanhs commented Jun 27, 2017•
edited
Loading