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

Allow Abort terminators in all const-contexts #77512

Merged
merged 6 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions compiler/rustc_mir/src/transform/check_consts/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,6 @@ pub trait NonConstOp: std::fmt::Debug {
fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> DiagnosticBuilder<'tcx>;
}

#[derive(Debug)]
pub struct Abort;
impl NonConstOp for Abort {
fn status_in_item(&self, ccx: &ConstCx<'_, '_>) -> Status {
mcf_status_in_item(ccx)
}

fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
mcf_build_error(ccx, span, "abort is not stable in const fn")
}
}

#[derive(Debug)]
pub struct FloatingPointOp;
impl NonConstOp for FloatingPointOp {
Expand Down
16 changes: 11 additions & 5 deletions compiler/rustc_mir/src/transform/check_consts/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -434,11 +434,13 @@ impl Visitor<'tcx> for Validator<'mir, 'tcx> {
fn visit_basic_block_data(&mut self, bb: BasicBlock, block: &BasicBlockData<'tcx>) {
trace!("visit_basic_block_data: bb={:?} is_cleanup={:?}", bb, block.is_cleanup);

// Just as the old checker did, we skip const-checking basic blocks on the unwind path.
// These blocks often drop locals that would otherwise be returned from the function.
// We don't const-check basic blocks on the cleanup path since we never unwind during
// const-eval: a panic causes an immediate compile error. In other words, cleanup blocks
// are unreachable during const-eval.
//
// FIXME: This shouldn't be unsound since a panic at compile time will cause a compiler
// error anyway, but maybe we should do more here?
// We can't be more conservative (e.g., by const-checking cleanup blocks anyways) because
// locals that would never be dropped during normal execution are sometimes dropped during
// unwinding, which means backwards-incompatible live-drop errors.
if block.is_cleanup {
return;
}
Expand Down Expand Up @@ -874,12 +876,16 @@ impl Visitor<'tcx> for Validator<'mir, 'tcx> {
}

TerminatorKind::InlineAsm { .. } => self.check_op(ops::InlineAsm),
TerminatorKind::Abort => self.check_op(ops::Abort),

TerminatorKind::GeneratorDrop | TerminatorKind::Yield { .. } => {
self.check_op(ops::Generator(hir::GeneratorKind::Gen))
}

TerminatorKind::Abort => {
// Cleanup blocks are skipped for const checking (see `visit_basic_block_data`).
span_bug!(self.span, "`Abort` terminator outside of cleanup block")
ecstatic-morse marked this conversation as resolved.
Show resolved Hide resolved
}

TerminatorKind::Assert { .. }
| TerminatorKind::FalseEdge { .. }
| TerminatorKind::FalseUnwind { .. }
Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/consts/const-eval/unwind-abort.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#![feature(unwind_attributes, const_panic)]

#[unwind(aborts)]
const fn foo() {
panic!() //~ evaluation of constant value failed
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
}

const _: () = foo(); //~ any use of this value will cause an error
ecstatic-morse marked this conversation as resolved.
Show resolved Hide resolved

fn main() {
let _ = foo();
}
21 changes: 21 additions & 0 deletions src/test/ui/consts/const-eval/unwind-abort.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error[E0080]: evaluation of constant value failed
--> $DIR/unwind-abort.rs:5:5
|
LL | panic!()
| ^^^^^^^^ the evaluated program panicked at 'explicit panic', $DIR/unwind-abort.rs:5:5
|
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: any use of this value will cause an error
--> $DIR/unwind-abort.rs:8:15
|
LL | const _: () = foo();
| --------------^^^^^-
| |
| referenced constant has errors
|
= note: `#[deny(const_err)]` on by default

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
17 changes: 17 additions & 0 deletions src/test/ui/consts/unwind-abort.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// check-pass

#![feature(unwind_attributes, const_panic)]

// `#[unwind(aborts)]` is okay for a `const fn`. We don't unwind in const-eval anyways.
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add two tests? Our test suite is already huge.^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One is check-pass, the other fails to compile.

Copy link
Member

@RalfJung RalfJung Oct 4, 2020

Choose a reason for hiding this comment

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

I can see that, I just do not understand their respective purpose. Specifically, "consts/unwind-abort" does not even run any CTFE, and the other test already checks that the Abort terminator is accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷 I'm happy with the test coverage in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I am trying to understand what unwind-abort.rs tests that the other test does not already cover.

My concern is not too little coverage; my concern is unnecessarily growing the number of UI tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I try to split up passing and failing tests when possible. One is solely a test of const-checking and involves no const-eval, the other is a sanity check to ensure that the CTFE engine doesn't do anything weird when evaluating something with #[unwind(aborts)].

Copy link
Member

Choose a reason for hiding this comment

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

I think this unnecessarily leads to an excessive number of tests in our UI test suite that is already so big that it becomes hard to run the entire thing, but whatever.

Could you add comments to these tests explaining what they are meant to test?

#[unwind(aborts)]
const fn foo() {
panic!()
}

const fn bar() {
foo();
}

fn main() {
bar();
}