trans: Move rust_try into the compiler #27176

alexcrichton · Jul 21, 2015

This commit moves the IR files in the distribution, rust_try.ll,
rust_try_msvc_64.ll, and rust_try_msvc_32.ll into the compiler from the main
distribution. There's a few reasons for this change:

LLVM changes its IR syntax from time to time, so it's very difficult to
have these files build across many LLVM versions simultaneously. We'll likely
want to retain this ability for quite some time into the future.
The implementation of these files is closely tied to the compiler and runtime
itself, so it makes sense to fold it into a location which can do more
platform-specific checks for various implementation details (such as MSVC 32
vs 64-bit).
This removes LLVM as a build-time dependency of the standard library. This may
end up becoming very useful if we move towards building the standard library
with Cargo.

In the immediate future, however, this commit should restore compatibility with
LLVM 3.5 and 3.6.

rust-highfive · Jul 21, 2015

r? @pcwalton

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

alexcrichton · Jul 21, 2015

r? @brson

cc @rust-lang/compiler (my knowledge of trans is pretty bad and I may have done egregious things)

Aatch · Jul 21, 2015

Aatch reviewed Jul 21, 2015

View changes

Aatch · Jul 21, 2015

src/librustc_trans/trans/intrinsic.rs

+}
+
+// MSVC's definition of the `rust_try` function. The exact implementation here
+// is a little different than the GNU (standard) version below, not ony because


"ony" -> "only"

brson · Jul 21, 2015

@bors r+

bors · Jul 21, 2015

📌 Commit 6046fb7 has been approved by brson

brson · Jul 21, 2015

Awesome patch.

alexcrichton · Jul 21, 2015

@bors: r=brson

bors · Jul 21, 2015

📌 Commit 970a3ca has been approved by brson

nagisa · Jul 21, 2015

nagisa reviewed Jul 21, 2015

View changes

nagisa · Jul 21, 2015

src/librustc_trans/trans/intrinsic.rs

+                                    false);
+
+        // Define the "inner try" shim
+        let rust_try_inner = declare::declare_internal_rust_fn(ccx,


If you are “defining” the shim, declare::define_internal_rust_fn should be used.

nagisa · Jul 21, 2015

nagisa reviewed Jul 21, 2015

View changes

nagisa · Jul 21, 2015

src/librustc_trans/trans/intrinsic.rs

+        });
+
+        // Define the "outer try" shim.
+        let rust_try = declare::declare_internal_rust_fn(ccx, "__rust_try",


nagisa · Jul 21, 2015

nagisa reviewed Jul 21, 2015

View changes

nagisa · Jul 21, 2015

src/rustllvm/RustWrapper.cpp

@@ -120,7 +120,8 @@ extern "C" void LLVMAddDereferenceableCallSiteAttr(LLVMValueRef Instr, unsigned
                                                         idx, B)));
 }

-extern "C" void LLVMAddFunctionAttribute(LLVMValueRef Fn, unsigned index, uint64_t Val) {
+extern "C" void LLVMAddFunctionAttribute(LLVMValueRef Fn, unsigned index,


nit: doesn’t seem to be necessary change.

alexcrichton · Jul 21, 2015

@bors: r=brson c35b2bd

bors · Jul 22, 2015

⌛️ Testing commit c35b2bd with merge 3475c5b...

bors · Jul 22, 2015

💔 Test failed - auto-win-gnu-64-nopt-t

alexcrichton · Jul 22, 2015

@bors: retry

On Tue, Jul 21, 2015 at 9:19 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-gnu-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-nopt-t/builds/766

—
Reply to this email directly or view it on GitHub
#27176 (comment).

bors · Jul 22, 2015

⌛️ Testing commit c35b2bd with merge 25281b1...

bors · Jul 22, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt

vadimcn · Jul 22, 2015

:(
Couldn't we've ran .ll files through cpp (like is done with regular asm) for the same effect?

alexcrichton · Jul 22, 2015

In theory, yes, but that still requires the standard library to have llc available to compile itself, which we need to move away from if we want to build std with Cargo at some point. It also would have been interesting trying to detect the llc version, pass the right variables to the preprocessor, and then piping all that back into llc to create an object.

vadimcn · Jul 22, 2015

In theory, yes, but that still requires the standard library to have llc available to compile itself

I am not sure I understand this point. Didn't rust_trylive in rustrt_native library? Which is effectively a part of the compiler, which already requires LLVM to be around.
How it it different from all other runtime fns in rustrt_native and compiler-rt?

My concern with the new way is that:

Emitting IR via LLVM API takes a lot more work and is way less readable because of all the noise added by API usage.
One cannot override rust_try for a custom target, (like you we able to do for *-pc-windows-msvc) without changing the compiler.

vadimcn · Jul 22, 2015

(sorry for jumping in late - I was not aware of this change until bors told me that my PR collided with this one)

alexcrichton · Jul 22, 2015

I am not sure I understand this point. Didn't rust_trylive in rustrt_native library? Which is effectively a part of the compiler, which already requires LLVM to be around.
How it it different from all other runtime fns in rustrt_native and compiler-rt?

Ah by this I just mean that the tool llc is no longer needed to build the standard library.

The other shims and such in rustrt_native can be built with a normal C compiler and I believe that all the shims in compiler-rt can also be built with a normal C compiler. If you're in theory cross compiling the standard library to a new architecture it's much more likely you've got the right C compiler installed vs llc.

Emitting IR via LLVM API takes a lot more work and is way less readable because of all the noise added by API usage.

I don't disagree with this!

One cannot override rust_try for a custom target, (like you we able to do for *-pc-windows-msvc) without changing the compiler.

I'm not quite sure I follow this. Is this basically saying that it's easier to edit IR than it is to edit the compiler?

sorry for jumping in late - I was not aware of this change until bors told me that my PR collided with this one

No worries!

vadimcn · Jul 23, 2015

I'm not quite sure I follow this. Is this basically saying that it's easier to edit IR than it is to edit the compiler?

I meant that as a library function in rustrt_native, it could have been overridden. Not so much with it as compiler intrinsic.
In principle, rust_try could have been implemented in C++, assembly, or even in C, if the compiler supports suitable extensions. It's just hard to do so portably.

alexcrichton · Jul 23, 2015

How do you override something in a native library? You mean avoiding #[cfg(stage0)]?

The ABI of unwinding is very closely tied to the compiler itself, so even though it could be implemented in various locations it kinda makes sense to me to implement this as an intrinsic so the compiler has complete control over all ABI possibilities.

vadimcn · Jul 23, 2015

How do you override something in a native library?

I meant that when building rustrt_native for a new target, you could write a custom rust_try.

alexcrichton · Jul 23, 2015

That's true, but do you know if that's done often? From what I've seen the rust_try.ll file basically never changes.

eddyb · Jul 24, 2015

Was it known that this would break unwinding entirely on -Z no-landing-pads builds?
This means check-stage1-cfail fails miserably, it need at least the rust_try landing pad to run.

Individual stage1 rustc invocations will abort with fatal runtime error: Could not unwind stack, error = 5 if any errors are found.
And then the compiletest driver will panic! during handling of the unexpected SIGILL in rustc, triggering another fatal runtime error: Could not unwind stack, error = 5 abort, hiding the original rustc one.

eddyb · Jul 24, 2015

Actually, scratch that, it's because the snapshot compiler doesn't have the try intrinsic so this PR uses a noop, breaking unwinding for the stage1 compiler and tests until the next snapshot.

alexcrichton · Jul 24, 2015

@eddyb yes this was known.

rust-highfive assigned pcwalton Jul 21, 2015

rust-highfive assigned brson and unassigned pcwalton Jul 21, 2015

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch 2 times, most recently from 51016bd to 6046fb7 Jul 21, 2015

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch 4 times, most recently from 825860c to 970a3ca Jul 21, 2015

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch from 970a3ca to c35b2bd Jul 21, 2015

bors merged commit c35b2bd into rust-lang:master Jul 22, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Jul 22, 2015

Closed

refactor svh #26629

Merged

SIMD groundwork part 1 #27169

Merged

Implement Win64 eh_personality natively. #27210

Merged

Refactor how closures are represented to expose the types of the upvars #27087

alexcrichton deleted the alexcrichton:fix-stock-llvm branch Jul 22, 2015

arielb1 referenced this pull request Sep 15, 2015
Merged
1.3 relnotes #28408

rust-lang/rust

Join GitHub today

trans: Move rust_try into the compiler #27176

Conversation

alexcrichton commented Jul 21, 2015

rust-highfive assigned pcwalton Jul 21, 2015

This comment has been minimized.

rust-highfive commented Jul 21, 2015

This comment has been minimized.

alexcrichton commented Jul 21, 2015

rust-highfive assigned brson and unassigned pcwalton Jul 21, 2015

Aatch reviewed Jul 21, 2015 View changes

This comment has been minimized.

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch 2 times, most recently from 51016bd to 6046fb7 Jul 21, 2015

This comment has been minimized.

brson commented Jul 21, 2015

This comment has been minimized.

bors commented Jul 21, 2015

This comment has been minimized.

brson commented Jul 21, 2015

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch 4 times, most recently from 825860c to 970a3ca Jul 21, 2015

This comment has been minimized.

alexcrichton commented Jul 21, 2015

This comment has been minimized.

bors commented Jul 21, 2015

nagisa reviewed Jul 21, 2015 View changes

This comment has been minimized.

nagisa reviewed Jul 21, 2015 View changes

This comment has been minimized.

nagisa reviewed Jul 21, 2015 View changes

This comment has been minimized.

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch from 970a3ca to c35b2bd Jul 21, 2015

This comment has been minimized.

alexcrichton commented Jul 21, 2015

bors added a commit that referenced this pull request Jul 22, 2015

This comment has been minimized.

bors commented Jul 22, 2015

This comment has been minimized.

bors commented Jul 22, 2015

This comment has been minimized.

alexcrichton commented Jul 22, 2015

This comment has been minimized.

bors commented Jul 22, 2015

bors added a commit that referenced this pull request Jul 22, 2015

This comment has been minimized.

bors commented Jul 22, 2015

Hide details View details bors merged commit c35b2bd into rust-lang:master Jul 22, 2015 2 checks passed

2 checks passed

This was referenced Jul 22, 2015

alexcrichton deleted the alexcrichton:fix-stock-llvm branch Jul 22, 2015

This comment has been minimized.

vadimcn commented Jul 22, 2015

This comment has been minimized.

alexcrichton commented Jul 22, 2015

This comment has been minimized.

vadimcn commented Jul 22, 2015

This comment has been minimized.

vadimcn commented Jul 22, 2015

This comment has been minimized.

alexcrichton commented Jul 22, 2015

This comment has been minimized.

vadimcn commented Jul 23, 2015

This comment has been minimized.

alexcrichton commented Jul 23, 2015

This comment has been minimized.

vadimcn commented Jul 23, 2015

This comment has been minimized.

alexcrichton commented Jul 23, 2015

This comment has been minimized.

eddyb commented Jul 24, 2015

This comment has been minimized.

eddyb commented Jul 24, 2015

This comment has been minimized.

alexcrichton commented Jul 24, 2015

geofft added a commit to geofft/std-with-cargo that referenced this pull request Aug 31, 2015

geofft added a commit to geofft/std-with-cargo that referenced this pull request Aug 31, 2015

arielb1 referenced this pull request Sep 15, 2015

Aatch reviewed Jul 21, 2015

View changes

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch 2 times, most recently from `51016bd` to `6046fb7` Jul 21, 2015

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch 4 times, most recently from `825860c` to `970a3ca` Jul 21, 2015

nagisa reviewed Jul 21, 2015

View changes

nagisa reviewed Jul 21, 2015

View changes

nagisa reviewed Jul 21, 2015

View changes

alexcrichton force-pushed the alexcrichton:fix-stock-llvm branch from `970a3ca` to `c35b2bd` Jul 21, 2015

bors merged commit `c35b2bd` into rust-lang:master Jul 22, 2015
2 checks passed