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

Make TerminatorKind::Abort an error in all const contexts #77460

Closed

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Oct 2, 2020

I'm not too up-to-date on FFI-unwind stuff, but Abort denotes that panics inside function calls (usually across an FFI boundary) will abort immediately. These semantics are not supported by Miri the const-eval engine AFAICT, and I don't think you can even enable them on the current stable. Forbid them in all const-contexts before that last part changes.

r? @oli-obk
cc @RalfJung (for long-term plan around Abort in Miri)

I'm not too up-to-date on FFI-unwind stuff, but `Abort` denotes that
panics inside function calls (usually across an FFI boundary) will abort
immediately. These semantics are not supported by Miri, and I don't
think you can even enable them on the current stable. Forbid them in all
const-contexts before that last part changes.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2020
@RalfJung
Copy link
Member

RalfJung commented Oct 2, 2020

I'm not too up-to-date on FFI-unwind stuff, but Abort denotes that panics
inside function calls (usually across an FFI boundary) will abort immediately.

I am not sure what you mean by this... how could a terminator have any semantics when it is not even run?

AFAIK Miri implements the abort terminator correctly, and we even have a test for that. If you know of a program that is executed incorrectly under Miri, I'd be curious to learn about that.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Oct 2, 2020

Here's what the code looks like in the const-eval engine:

fn abort(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx, !> {
throw_unsup_format!("aborting execution is not supported")
}

I'll ask in a different way: What's the long-term plan for unwinding/aborting in the const-eval engine? I don't see why #[unwind(abort)] would be problematic for const-eval. Do we want to just allow it in the static checks?

@ecstatic-morse ecstatic-morse changed the title Make TerminatorKind::Abort an error in all contexts Make TerminatorKind::Abort an error in all const contexts Oct 2, 2020
@RalfJung
Copy link
Member

RalfJung commented Oct 3, 2020

I'll ask in a different way: What's the long-term plan for unwinding/aborting in the const-eval engine? I don't see why #[unwind(abort)] would be problematic for const-eval. Do we want to just allow it in the static checks?

AFAIK the plan is that panics immediately abort execution with an error pointing at the panic site. Thus, no unwinding would happen.

It seems reasonable to do something similar for aborts. The error should maybe be made nicer ("unsupported" errors are meant to be prevented by the static checks I think), but other than that I see no problem with just allowing it in the static checks.

EDIT: actually, if no unwinding ever happens, this code is ureachable anyway. So yeah we can probably just allow abort terminators. Maybe with a comment in some suitable place saying that Abort terminators are currently only used for catching panics in unwind(abort) functions, and without unwinding during CTFE, they are thus unreachable.

@ecstatic-morse
Copy link
Contributor Author

Obseleted by #77512

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants