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

rustc: Switch `extern` functions to abort by default on panic #55982

Merged
merged 1 commit into from Dec 13, 2018

Conversation

Projects
None yet
@alexcrichton
Member

alexcrichton commented Nov 15, 2018

This was intended to land way back in 1.24, but it was backed out due to breakage which has long since been fixed. Prior to this PR a crate could panic and unwind past an extern fn boundary, but such behavior is UB. For example, this is undefined behavior:

extern fn foo() {
    panic!();
}

fn main() {
    foo();
}

This PR changes the behavior of generated code to be sound-by-default. If an extern fn is unwound (panicked through) then it immediately aborts the program. Put another way, no extern fn can unwind.

The implementation in this PR is pretty simple because an unstable #[unwind] attribute could already be used to tweak the unwinding behavior. As a result this PR basically just switches the default from #[unwind] to #[unwind(abort)] for all extern fn definitions.

Closes #52652

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 15, 2018

r? @zackmdavis

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

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Nov 15, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 15, 2018

📌 Commit 6ea11fd has been approved by zackmdavis

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Nov 15, 2018

FWIW I still personally feel like changing stable behavior without providing a stabilized way to get back the old behavior is not great, but I'm not going to block this myself, both because I think this is fairly rare and the new behavior seems overall later.

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Nov 15, 2018

Maybe we should do a Crater run for this?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 15, 2018

@bors: r-

Seems fine by me to hold off on a crater run happening!

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 15, 2018

@bors: try

@bors

This comment has been minimized.

Contributor

bors commented Nov 15, 2018

⌛️ Trying commit 6ea11fd with merge 2e026d6...

bors added a commit that referenced this pull request Nov 15, 2018

Auto merge of #55982 - alexcrichton:panic-extern-abort, r=<try>
rustc: Switch `extern` functions to abort by default on panic

This was intended to land way back in 1.24, but it was backed out due to
breakage which has long since been fixed. An unstable `#[unwind]`
attribute can be used to tweak the behavior here, but this is currently
simply switching rustc's internal default to abort-by-default if an
`extern` function panics, making our codegen sound primarily (as
currently you can produce UB with safe code)

Closes #52652
@bors

This comment has been minimized.

Contributor

bors commented Nov 16, 2018

☀️ Test successful - status-travis
State: approved= try=True

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 16, 2018

@craterbot run start=master#6b9b97bd9b704f85f0184f7a213cc4d62bd9654c end=try#2e026d6e5d7621634147ad4c8074f1d964dbb1be mode=build-and-test

@craterbot

This comment has been minimized.

Collaborator

craterbot commented Nov 16, 2018

👌 Experiment pr-55982 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Collaborator

craterbot commented Nov 16, 2018

🚧 Experiment pr-55982 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Collaborator

craterbot commented Nov 18, 2018

🎉 Experiment pr-55982 is completed!
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 20, 2018

From that report there are four legitimate-looking regressions

In testing I've also noticed that the compiler no longer emits nounwind attributes for extern functions. This appears to be a regression in #51041, first showing up in Rust 1.28.0.

I've now pushed a follow-up commit to fix the regression, now applying nounwind on extern function as intended.

@alexcrichton alexcrichton force-pushed the alexcrichton:panic-extern-abort branch from 6ea11fd to 95310d6 Nov 20, 2018

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 20, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:2312b483:start=1542755280460092720,finish=1542755281902444787,duration=1442352067
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:17:49]    Compiling rustc_codegen_ssa v0.0.0 (/checkout/src/librustc_codegen_ssa)
[00:18:03] error[E0061]: this function takes 2 parameters but 1 parameter was supplied
[00:18:03]    --> librustc_codegen_llvm/attributes.rs:199:5
[00:18:03]     |
[00:18:03] 64  |   fn unwind(val: &'ll Value, can_unwind: bool) {
[00:18:03]     |   -------------------------------------------- defined here
[00:18:03] ...
[00:18:03] 199 | /     unwind(if cx.tcx.sess.panic_strategy() != PanicStrategy::Unwind {
[00:18:03] 200 | |         // In panic=abort mode we assume nothing can unwind anywhere, so
[00:18:03] 201 | |         // optimize based on this!
[00:18:03] ...   |
[00:18:03] 228 | |         true
[00:18:03] 229 | |     });
[00:18:03]     | |______^ expected 2 parameters
---
145100 ./obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib
145096 ./obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu
145092 ./obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib
134664 ./obj/build/bootstrap/debug/incremental/bootstrap-zemjd6kcyh2u
134660 ./obj/build/bootstrap/debug/incremental/bootstrap-zemjd6kcyh2u/s-f6v028rh0j-1jh577i-2q1xp1m83q6vz
107888 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
104700 ./src/tools/lldb
93748 ./src/tools/clang/test
89976 ./src/llvm-emscripten/test/CodeGen

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@alexcrichton alexcrichton force-pushed the alexcrichton:panic-extern-abort branch from 95310d6 to 07aafac Nov 20, 2018

bors added a commit that referenced this pull request Dec 12, 2018

Auto merge of #55982 - alexcrichton:panic-extern-abort, r=zackmdavis
rustc: Switch `extern` functions to abort by default on panic

This was intended to land way back in 1.24, but it was backed out due to
breakage which has long since been fixed. An unstable `#[unwind]`
attribute can be used to tweak the behavior here, but this is currently
simply switching rustc's internal default to abort-by-default if an
`extern` function panics, making our codegen sound primarily (as
currently you can produce UB with safe code)

Closes #52652
@bors

This comment has been minimized.

Contributor

bors commented Dec 12, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 12, 2018

The job asmjs of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:32:46] ---- [run-pass] run-pass/extern/extern-call-deep.rs stdout ----
[01:32:46] 
[01:32:46] error: test run failed!
[01:32:46] status: exit code: 1
[01:32:46] command: "/emsdk-portable/node/8.9.1_64bit/bin/node" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js"
[01:32:46] ------------------------------------------
[01:32:46] n = 1000
[01:32:46] n = 999
[01:32:46] n = 998
---
[01:32:46] 
[01:32:46] ------------------------------------------
[01:32:46] stderr:
[01:32:46] ------------------------------------------
[01:32:46] exception thrown: RangeError: Maximum call stack size exceeded,RangeError: Maximum call stack size exceeded
[01:32:46]     at invoke_ii (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:5375:5)
[01:32:46]     at Array.__ZN16extern_call_deep2cb17h3838fa28def83edcE (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:5830:8)
[01:32:46]     at _rust_dbg_call (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:6029:35)
[01:32:46]     at Array.__ZN16extern_call_deep5count17h896e346009afe526E (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:5807:8)
[01:32:46]     at Object.dynCall_ii (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:38191:38)
[01:32:47]     at invoke_ii (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:5373:32)
[01:32:47]     at Array.__ZN16extern_call_deep2cb17h3838fa28def83edcE (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:5830:8)
[01:32:47]     at _rust_dbg_call (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:6029:35)
[01:32:47]     at Array.__ZN16extern_call_deep5count17h896e346009afe526E (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:5807:8)
[01:32:47]     at Object.dynCall_ii (/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/extern/extern-call-deep/a.js:38191:38)
[01:32:47] ------------------------------------------
[01:32:47] 
[01:32:47] thread '[run-pass] run-pass/extern/extern-call-deep.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3252:9
[01:32:47] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[01:32:47] test result: FAILED. 2749 passed; 1 failed; 176 ignored; 0 measured; 0 filtered out
[01:32:47] 
[01:32:47] 
[01:32:47] 
[01:32:47] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/asmjs-unknown-emscripten/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-asmjs-unknown-emscripten" "--mode" "run-pass" "--target" "asmjs-unknown-emscripten" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--nodejs" "/emsdk-portable/node/8.9.1_64bit/bin/node" "--host-rustcflags" "-Crpath -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -Zunstable-options  -Lnative=/checkout/obj/build/asmjs-unknown-emscripten/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--llvm-version" "8.0.0svn\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:32:47] 
[01:32:47] 
[01:32:47] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target asmjs-unknown-emscripten src/test/run-pass src/test/run-fail src/libstd src/liballoc src/libcore
[01:32:47] Build completed unsuccessfully in 1:28:56
---
travis_time:end:180eb0f8:start=1544602639696426370,finish=1544602639703648833,duration=7222463
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0a4ef683
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1619df90
travis_time:start:1619df90
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0bca5bd0
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rustc: Switch `extern` functions to abort by default on panic
This was intended to land way back in 1.24, but it was backed out due to
breakage which has long since been fixed. An unstable `#[unwind]`
attribute can be used to tweak the behavior here, but this is currently
simply switching rustc's internal default to abort-by-default if an
`extern` function panics, making our codegen sound primarily (as
currently you can produce UB with safe code)

Closes #52652

@alexcrichton alexcrichton force-pushed the alexcrichton:panic-extern-abort branch from c3fc942 to 1091eee Dec 12, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 12, 2018

@bors: r=zackmdavis

@bors

This comment has been minimized.

Contributor

bors commented Dec 12, 2018

📌 Commit 1091eee has been approved by zackmdavis

@bors

This comment has been minimized.

Contributor

bors commented Dec 12, 2018

⌛️ Testing commit 1091eee with merge 5316104...

bors added a commit that referenced this pull request Dec 12, 2018

Auto merge of #55982 - alexcrichton:panic-extern-abort, r=zackmdavis
rustc: Switch `extern` functions to abort by default on panic

This was intended to land way back in 1.24, but it was backed out due to
breakage which has long since been fixed. An unstable `#[unwind]`
attribute can be used to tweak the behavior here, but this is currently
simply switching rustc's internal default to abort-by-default if an
`extern` function panics, making our codegen sound primarily (as
currently you can produce UB with safe code)

Closes #52652
@bors

This comment has been minimized.

Contributor

bors commented Dec 12, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: zackmdavis
Pushing 5316104 to master...

@bors

This comment has been minimized.

Contributor

bors commented Dec 12, 2018

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 12, 2018

@bors: retry

@bors

This comment has been minimized.

Contributor

bors commented Dec 13, 2018

⌛️ Testing commit 1091eee with merge 2f35a10...

bors added a commit that referenced this pull request Dec 13, 2018

Auto merge of #55982 - alexcrichton:panic-extern-abort, r=zackmdavis
rustc: Switch `extern` functions to abort by default on panic

This was intended to land way back in 1.24, but it was backed out due to
breakage which has long since been fixed. An unstable `#[unwind]`
attribute can be used to tweak the behavior here, but this is currently
simply switching rustc's internal default to abort-by-default if an
`extern` function panics, making our codegen sound primarily (as
currently you can produce UB with safe code)

Closes #52652
@bors

This comment has been minimized.

Contributor

bors commented Dec 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: zackmdavis
Pushing 2f35a10 to master...

@bors bors merged commit 1091eee into rust-lang:master Dec 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Dec 13, 2018

but this is currently simply switching rustc's internal default to abort-by-default

This sounds like the default is unchanged, and this PR doesn’t do anything without #[unwind]. But that’s not what the test case changes suggest. @alexcrichton Could you clarify the PR description, for the eventual release notes?

@alexcrichton alexcrichton deleted the alexcrichton:panic-extern-abort branch Dec 13, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 13, 2018

Sure! @SimonSapin does this clear things up?


This was intended to land way back in 1.24, but it was backed out due to breakage which has long since been fixed. Prior to this PR a crate could panic and unwind past an extern fn boundary, but such behavior is UB. For example, this is undefined behavior:

extern fn foo() {
    panic!();
}

fn main() {
    foo();
}

This PR changes the behavior of generated code to be sound-by-default. If an extern fn is unwound (panicked through) then it immediately aborts the program. Put another way, no extern fn can unwind.

The implementation in this PR is pretty simple because an unstable #[unwind] attribute could already be used to tweak the unwinding behavior. As a result this PR basically just switches the default from #[unwind] to #[unwind(abort)] for all extern fn definitions.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Dec 13, 2018

Yes this is better, thanks! I don’t know if this example is actually UB though. I though what was undefined was unwinding from Rust into a call stack of another language.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 13, 2018

You can explore the UB through the IR of a program like this:

#![crate_type = "lib"]      
                            
#[inline(never)]            
extern fn foo() {           
    panic!("x");            
}                           
                            
struct A;                   
                            
impl Drop for A {           
    fn drop(&mut self) {    
        extern { fn foo(); }
        unsafe { foo(); }   
    }                       
}                           
                            
pub fn bar() {              
    let _x = A;             
    foo();                  
}                           

The compiler places the nounwind LLVM attribute on the function foo, but it actually ends up unwinding. According to LLVM:

If the function does raise an exception, its runtime behavior is undefined

Note that recent compilers have a bug where they don't place nounwind (fixed in this PR), but 1.25.0 for example placed the nounwind attribute.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Dec 13, 2018

Ok I see. I didn’t know about nounwind. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment