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 in "assert_*" intrinsics #1222

Closed
RalfJung opened this issue Mar 11, 2020 · 5 comments · Fixed by #1239
Closed

Abort in "assert_*" intrinsics #1222

RalfJung opened this issue Mar 11, 2020 · 5 comments · Fixed by #1239
Labels
A-shims Area: This affects the external function shims C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@RalfJung
Copy link
Member

These three intrinsics should, IMO, abort instead of panic. The main reason we make them panic in rustc is that we want a good backtrace, and doing that from codegen is hard. But causing a panic is easy thanks to libcore having panic support, and std hooking into it.

But in Miri, we can show great backtraces on abort! So, there is no reason to use panics IMO. So I propose that we extend this with an abort message:

Abort,

and then cause an abort from the intrinsic.

@elichai that they would look into this.

@RalfJung RalfJung added A-shims Area: This affects the external function shims C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Mar 11, 2020
@eddyb
Copy link
Member

eddyb commented Mar 12, 2020

Might make more sense to name the intrinsics e.g assert_inhabited instead of panic_if_uninhabited.
That would also make aborting instead of panicking in miri less odd-looking.

Centril added a commit to Centril/rust that referenced this issue Mar 13, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this issue Mar 13, 2020
@elichai
Copy link
Contributor

elichai commented Mar 15, 2020

Had a medical procedure, so sadly I'll only be able to work on it in a few days

@RalfJung
Copy link
Member Author

RalfJung commented Mar 15, 2020

No worries, this is not urgent at all. :) Get well soon!

@eddyb eddyb changed the title Abort in "panic_if_*" intrinsics Abort in "assert_*" intrinsics Mar 15, 2020
@elichai
Copy link
Contributor

elichai commented Mar 18, 2020

How should I modify these tests to capture the aborts?
Should I make a separate .rs file for each? :/
Should I write the code into a new .rs file, and compile+run it in a new process? something else?

(maybe I can "patch" the abort function so I can catch it? but it might defeat the purpose)

We could also fork, but that will be unix specific

Another option would be to hook on SIGABRT and loop there, and launch a separate thread per function so we can catch all aborts like that without the process being terminated something like: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4363a89d68c089f122fd06cd37598a75

just remembered miri doesn't support threading yet

@RalfJung
Copy link
Member Author

Just remove these things from the catch test. And then add one or two new compile-fail tests instead. See here for an example of another test that aborts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-shims Area: This affects the external function shims C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants