Switch to rust-lang-nursery/compiler-builtins #42899

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

src/librustc_trans/trans_item.rs

+           linkage != llvm::Linkage::PrivateLinkage &&
+           attr::contains_name(ccx.tcx().hir.krate_attrs(), "compiler_builtins") {
+            unsafe {
+                llvm::LLVMRustSetVisibility(lldecl, llvm::Visibility::Hidden);


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

src/bootstrap/bin/rustc.rs

+        let crate_name = args.windows(2)
+            .find(|a| &*a[0] == "--crate-name")
+            .unwrap();
+        let crate_name = &*crate_name[1];


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

src/librustc_trans/trans_item.rs

+        // don't want the symbols to get exported.
+        if linkage != llvm::Linkage::InternalLinkage &&
+           linkage != llvm::Linkage::PrivateLinkage &&
+           attr::contains_name(ccx.tcx().hir.krate_attrs(), "compiler_builtins") {


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

src/test/run-pass/lib-defaults.rs

@@ -8,16 +8,15 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

-// aux-build:clibrary.rs


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

alexcrichton force-pushed the compiler-builtins branch from 9c427f1 to 22d25da Compare 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 8c5aedd Compare June 27, 2017 22:20

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

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

bors merged commit 7e6c9f3 into rust-lang:master Jul 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.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to rust-lang-nursery/compiler-builtins #42899

Switch to rust-lang-nursery/compiler-builtins #42899

alexcrichton commented Jun 25, 2017

rust-highfive commented Jun 25, 2017

alexcrichton commented Jun 25, 2017

alexcrichton commented Jun 25, 2017

nikomatsakis left a comment

nikomatsakis Jun 26, 2017

alexcrichton Jun 26, 2017

nikomatsakis Jun 27, 2017

nagisa Jun 26, 2017

alexcrichton Jun 26, 2017

nagisa Jun 26, 2017

alexcrichton Jun 26, 2017

alexcrichton Jun 26, 2017

nagisa Jun 26, 2017

Mark-Simulacrum commented Jun 27, 2017

alexcrichton commented Jun 27, 2017

aidanhs commented Jun 27, 2017

aidanhs commented Jun 27, 2017 •

edited

Loading

alexcrichton commented Jun 27, 2017

aidanhs commented Jun 27, 2017

alexcrichton commented Jun 30, 2017

bors commented Jun 30, 2017

aidanhs commented Jun 30, 2017

alexcrichton commented Jun 30, 2017

bors commented Jun 30, 2017

bors commented Jul 1, 2017

bors commented Jul 1, 2017

kennytm commented Jul 1, 2017

bors commented Jul 6, 2017

japaric commented Jul 6, 2017

Switch to rust-lang-nursery/compiler-builtins #42899

Switch to rust-lang-nursery/compiler-builtins #42899

Conversation

alexcrichton commented Jun 25, 2017

rust-highfive commented Jun 25, 2017

alexcrichton commented Jun 25, 2017

alexcrichton commented Jun 25, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Jun 27, 2017

alexcrichton commented Jun 27, 2017

aidanhs commented Jun 27, 2017

aidanhs commented Jun 27, 2017 • edited Loading

alexcrichton commented Jun 27, 2017

aidanhs commented Jun 27, 2017

alexcrichton commented Jun 30, 2017

bors commented Jun 30, 2017

aidanhs commented Jun 30, 2017

alexcrichton commented Jun 30, 2017

bors commented Jun 30, 2017

bors commented Jul 1, 2017

bors commented Jul 1, 2017

kennytm commented Jul 1, 2017

bors commented Jul 6, 2017

japaric commented Jul 6, 2017

aidanhs commented Jun 27, 2017 •

edited

Loading