Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable TrapUnreachable in LLVM. #45920

Merged
merged 6 commits into from
Nov 16, 2017
Merged

Conversation

sunfishcode
Copy link
Member

This patch enables LLVM's TrapUnreachable flag, which tells it to translate unreachable instructions into hardware trap instructions, rather than allowing control flow to "fall through" into whatever code happens to follow it in memory.

This follows up on #28728 (comment). For example, for @zackw's testcase here, the output function contains a ud2 instead of no code, so it won't "fall through" into whatever happens to be next in memory.

(I'm also working on the problem of LLVM optimizing away infinite loops, but the patch here is useful independently.)

I tested this patch on a few different codebases, and the code size increase ranged from 0.0% to 0.1%.

Enable LLVM's TrapUnreachable flag, which tells it to translate
`unreachable` instructions into hardware trap instructions, rather
than allowing control flow to "fall through" into whatever code
happens to follow it in memory.
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2017
// Tell LLVM to translate `unreachable` into an explicit trap instruction.
// This limits the extent of possible undefined behavior in some cases, as it
// prevents control flow from "falling through" into whatever code happens to
// be layed out next in memory.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "layed" should be "laid"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@zackw
Copy link
Contributor

zackw commented Nov 10, 2017

As the person who originally asked for this, thanks for making sure it got done.

With rustc now emitting "ud2" on unreachable code, a "return nothing"
sequence may take the same number of lines as an "unreachable" sequence
in assembly code. This test is testing that intrinsics::unreachable()
works by testing that it reduces the number of lines in the assembly
code. Fix it by adding a return value, which requires an extra
instruction in the reachable case, which provides the test what it's
looking for.
@Zoxc
Copy link
Contributor

Zoxc commented Nov 11, 2017

I'd like to see this option as a part of TargetOptions and have it be enabled by default.

unsafe {
// Pretend this asm is an exit() syscall.
asm!("" :: "r"(n) :: "volatile");
intrinsics::unreachable()
}
42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment that the 42 is there just so there's some code for LLVM to remove, making the test that the assembly output of exit-unreachable.rs is shorter than exit-ret.rs succeed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added.

@Zoxc Zoxc self-assigned this Nov 11, 2017
The return value in these tests is just being used to generate extra
code so that it can be detected in the test script, which is just
counting lines in the assembly output.
@Zoxc
Copy link
Contributor

Zoxc commented Nov 12, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Nov 12, 2017

@Zoxc: 🔑 Insufficient privileges: Not in reviewers

@Zoxc
Copy link
Contributor

Zoxc commented Nov 12, 2017

I guess I don't have bors permissions yet.

@alexcrichton When will that happen? =P

@Mark-Simulacrum
Copy link
Member

@bors r=Zoxc

@bors
Copy link
Contributor

bors commented Nov 12, 2017

📌 Commit 7b6b764 has been approved by Zoxc

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2017
@bors
Copy link
Contributor

bors commented Nov 15, 2017

⌛ Testing commit 7b6b764 with merge 1f893306a350c62927740ec18e4e75c57c9dc517...

@bors
Copy link
Contributor

bors commented Nov 15, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Nov 15, 2017

Failed to build rustdoc at stage2 in debug build (i686-gnu-nopt) with SIGILL.

[00:46:24] error: process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc -vV` (signal: 4, SIGILL: illegal instruction)
[00:46:24] 
[00:46:24] 
[00:46:24] command did not execute successfully: "/checkout/obj/build/i686-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "i686-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/rustdoc/Cargo.toml"
[00:46:24] expected success, got: exit code: 101

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 15, 2017
@zackw
Copy link
Contributor

zackw commented Nov 15, 2017 via email

@sunfishcode
Copy link
Member Author

Looks like it's just 32-bit x86. I've been able to reproduce the crash now; here's a backtrace:

(gdb) where
#0  0xf7ea6dd0 in __rust_probestack () from /home/rustacean/rust/build/i686-unknown-linux-gnu/stage2/lib/librustc_driver-57160753fe603ca7.so
#1  0xf7db46be in std::sys_common::backtrace::__rust_begin_short_backtrace () from /home/rustacean/rust/build/i686-unknown-linux-gnu/stage2/lib/librustc_driver-57160753fe603ca7.so
#2  0xf7c67488 in __rust_maybe_catch_panic () from /home/rustacean/rust/build/i686-unknown-linux-gnu/stage2/lib/libstd-650e8e2b6b1595ab.so
#3  0xf7e36752 in <F as alloc::boxed::FnBox<A>>::call_box () from /home/rustacean/rust/build/i686-unknown-linux-gnu/stage2/lib/librustc_driver-57160753fe603ca7.so
#4  0xf7c0a635 in std::sys::imp::thread::Thread::new::thread_start () from /home/rustacean/rust/build/i686-unknown-linux-gnu/stage2/lib/libstd-650e8e2b6b1595ab.so
#5  0xf532b2f5 in start_thread (arg=0xf1dffb40) at pthread_create.c:456
#6  0xf7ac76de in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:113

__rust_probestack is a function that contains an inline asm that does a "ret", which is fairly suspicious. I'll investigate further.

@kennytm
Copy link
Member

kennytm commented Nov 15, 2017

cc @alexcrichton (#42816) this PR seems to have conflict with the x86 stack probe.

@sunfishcode
Copy link
Member Author

I've now filed rust-lang/compiler-builtins#207 which makes __rust_probestack more resistant to instruction reordering by the compiler, which fixes the crash.

@sunfishcode
Copy link
Member Author

rust-lang/compiler-builtins#207 is now merged. Can we do a new bors run with an updated compiler-builtins?

@alexcrichton
Copy link
Member

@sunfishcode oh that's actually just a submodule in this repository at src/libcompiler_builtins, to test against an updated version you'll need to update the submodule revision as part of this PR as well

@sunfishcode
Copy link
Member Author

Ok, I've now updated the submodule.

@kennytm
Copy link
Member

kennytm commented Nov 16, 2017

@bors r=Zoxc

@bors
Copy link
Contributor

bors commented Nov 16, 2017

📌 Commit ac48348 has been approved by Zoxc

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2017
@bors
Copy link
Contributor

bors commented Nov 16, 2017

⌛ Testing commit ac48348 with merge 1410d56...

bors added a commit that referenced this pull request Nov 16, 2017
Enable TrapUnreachable in LLVM.

This patch enables LLVM's TrapUnreachable flag, which tells it to translate `unreachable` instructions into hardware trap instructions, rather than allowing control flow to "fall through" into whatever code happens to follow it in memory.

This follows up on #28728 (comment). For example, for @zackw's testcase [here](#42009 (comment)), the output function contains a `ud2` instead of no code, so it won't "fall through" into whatever happens to be next in memory.

(I'm also working on the problem of LLVM optimizing away infinite loops, but the patch here is useful independently.)

I tested this patch on a few different codebases, and the code size increase ranged from 0.0% to 0.1%.
@bors
Copy link
Contributor

bors commented Nov 16, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: Zoxc
Pushing 1410d56 to master...

@bors bors merged commit ac48348 into rust-lang:master Nov 16, 2017
@sunfishcode sunfishcode deleted the trap-on-unreachable branch November 16, 2017 13:50
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 18, 2017
tormol added a commit to tormol/tiny-rust-executable that referenced this pull request Sep 7, 2018
The object file in the rlib has been renamed again,
and the name now includes now gibberish.

The assembly now has an extra instruction due to
rust-lang/rust#45920
but despite this the final executable is actually smaller than before.
(There doesn't appear to be a way to disable the unreachable() trap.)
tormol added a commit to tormol/tiny-rust-executable that referenced this pull request Sep 7, 2018
The object file in the rlib has been renamed again,
and the name now includes now gibberish.

The assembly now has an extra instruction due to
rust-lang/rust#45920
which increases the executable size with two bytes.
(There doesn't appear to be a way to disable the unreachable() trap.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants