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

Abort if a promoted fails to be const evaluable and its runtime checks didn't trigger #52571

Merged
merged 5 commits into from
Jul 23, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 20, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2018
@nagisa
Copy link
Member

nagisa commented Jul 20, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Jul 20, 2018

📌 Commit 7064f69 has been approved by nagisa

@bors bors 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 Jul 20, 2018
@nagisa nagisa added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2018
@nagisa
Copy link
Member

nagisa commented Jul 20, 2018

A test would be great still. Either a codegen or run-pass that self-invokes is fine.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 20, 2018

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jul 20, 2018

📌 Commit 92fad0f has been approved by nagisa

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 20, 2018

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jul 20, 2018

📌 Commit 297ad72 has been approved by nagisa

@kennytm
Copy link
Member

kennytm commented Jul 21, 2018

@bors r-

You cannot spawn a process on wasm32. Please // ignore-wasm the test case.

[00:56:18] ---- [run-pass] run-pass/invalid_const_promotion.rs stdout ----
[00:56:18] 
[00:56:18] error: test run failed!
[00:56:18] status: exit code: 101
[00:56:18] command: "/node-v9.2.0-linux-x64/bin/node" "/checkout/src/etc/wasm32-shim.js" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/invalid_const_promotion/a.wasm"
[00:56:18] stdout:
[00:56:18] ------------------------------------------
[00:56:18] 
[00:56:18] ------------------------------------------
[00:56:18] stderr:
[00:56:18] ------------------------------------------
[00:56:18] RuntimeError: unreachable
[00:56:18]     at __rust_start_panic (wasm-function[131]:1)
[00:56:18]     at rust_panic.llvm.9502800419846304078 (wasm-function[127]:30)
[00:56:18]     at std::panicking::rust_panic_with_hook::h0d5a137ad2df1a11 (wasm-function[122]:441)
[00:56:18]     at std::panicking::continue_panic_fmt::hf8718f064fd9e77b (wasm-function[120]:120)
[00:56:18]     at rust_begin_unwind (wasm-function[119]:3)
[00:56:18]     at core::panicking::panic_fmt::hb53c33b6d4981cfb (wasm-function[228]:70)
[00:56:18]     at core::result::unwrap_failed::h0ceaf4312c8cc139 (wasm-function[9]:130)
[00:56:18]     at _$LT$core..result..Result$LT$T$C$$u20$E$GT$$GT$::unwrap::h68dfea1c9fcddfb1 (wasm-function[10]:34)
[00:56:18]     at invalid_const_promotion::main::h0e4b68e8564f7e57 (wasm-function[12]:179)
[00:56:18]     at std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::hceea268ccf57703c (wasm-function[5]:17)
[00:56:18]     at std::sys_common::backtrace::__rust_begin_short_backtrace::ha35b6b1b512e8e7b (wasm-function[57]:8)
[00:56:18]     at std::panicking::try::do_call::h6e356fa82a1037a1 (.llvm.9502800419846304078) (wasm-function[118]:20)
[00:56:18]     at __rust_maybe_catch_panic (wasm-function[130]:5)
[00:56:18]     at std::rt::lang_start_internal::h21c3cbc5f89250ba (wasm-function[58]:104)
[00:56:18]     at main (wasm-function[13]:33)
[00:56:18]     at Object.<anonymous> (/checkout/src/etc/wasm32-shim.js:136:20)
[00:56:18]     at Module._compile (module.js:641:30)
[00:56:18]     at Object.Module._extensions..js (module.js:652:10)
[00:56:18]     at Module.load (module.js:560:32)
[00:56:18]     at tryModuleLoad (module.js:503:12)
[00:56:18] 
[00:56:18] ------------------------------------------
[00:56:18] 
[00:56:18] thread '[run-pass] run-pass/invalid_const_promotion.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3137:9
[00:56:18] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:56:18] 
[00:56:18] 
[00:56:18] failures:
[00:56:18]     [run-pass] run-pass/invalid_const_promotion.rs

@bors bors 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 Jul 21, 2018
@RalfJung
Copy link
Member

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jul 21, 2018

📌 Commit 555a7b4 has been approved by nagisa

@bors bors 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 Jul 21, 2018
.stdout(Stdio::piped())
.stdin(Stdio::piped())
.arg("test").output().unwrap();
assert!(!p.status.success());
Copy link
Member

Choose a reason for hiding this comment

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

This test would also pass if the program SEGFAULTs (like some variants of this did) instead of SIGILLs... but I am not sure how to test for this properly, so whatever.^^

Copy link
Member

Choose a reason for hiding this comment

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

#[cfg(unix)]
fn check_status(status: std::process::ExitStatus)
{
use libc;
use std::os::unix::process::ExitStatusExt;
assert!(status.signal() == Some(libc::SIGSEGV)
|| status.signal() == Some(libc::SIGBUS));
}
#[cfg(not(unix))]
fn check_status(status: std::process::ExitStatus)
{
assert!(!status.success());
}

Copy link
Member

Choose a reason for hiding this comment

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

So...? That's how this test could be improved, is what you are saying?

Copy link
Member

Choose a reason for hiding this comment

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

I mean we already have copy-pastable code that does the exact checking necessary here. We should avoid writing non-trivial tests by hand when there's already a good example.

// option. This file may not be copied, modified, or distributed
// except according to those terms.

// ignore-wasm
Copy link
Member

Choose a reason for hiding this comment

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

Sorry it should be // ignore-wasm32 😭 (or maybe // ignore-emscripten to include asm.js too, I'm not sure if asm.js can run a program)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 22, 2018

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jul 22, 2018

📌 Commit 233a6e1 has been approved by nagisa

@bors
Copy link
Contributor

bors commented Jul 23, 2018

⌛ Testing commit 233a6e1 with merge 210d61f...

bors added a commit that referenced this pull request Jul 23, 2018
Abort if a promoted fails to be const evaluable and its runtime checks didn't trigger

r? @eddyb

cc @RalfJung @nagisa

cc #49760
@bors
Copy link
Contributor

bors commented Jul 23, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 210d61f to master...

@bors bors merged commit 233a6e1 into rust-lang:master Jul 23, 2018
bors added a commit that referenced this pull request Jul 24, 2018
[beta] Abort instead of UB if promotion fails

original PR: #52571 (not beta approved yet)

r? @nikomatsakis

cc @RalfJung
@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 26, 2018
@oli-obk oli-obk deleted the promotion_abort branch July 26, 2018 14:50
@pnkfelix
Copy link
Member

landed in beta in #52624

@pnkfelix pnkfelix removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 26, 2018
moshg pushed a commit to moshg/rust-std-ja that referenced this pull request Apr 4, 2020
[beta] Abort instead of UB if promotion fails

original PR: rust-lang/rust#52571 (not beta approved yet)

r? @nikomatsakis

cc @RalfJung
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants