Skip to content

const-eval: disallow unwinding across functions that !fn_can_unwind() #85546

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from May 28, 2021
Merged

const-eval: disallow unwinding across functions that !fn_can_unwind() #85546

merged 15 commits into from May 28, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 21, 2021 •
edited by ghost
Loading

Following rust-lang/miri#1776 (comment), so r? @RalfJung

This PR turns unwind in StackPopCleanup::Goto into a new enum StackPopUnwind, with a NotAllowed variant to indicate that unwinding is not allowed. This variant is chosen based on rustc_middle::ty::layout::fn_can_unwind() in eval_fn_call() when pushing the frame. A check is added in unwind_to_block() to report UB if unwinding happens across a StackPopUnwind::NotAllowed frame.

Tested with Miri HEAD with minor changes and the rust-lang/miri#1776 branch with these changes.

@rust-highfive
Copy link
Contributor

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2021
@RalfJung
Copy link
Member

Tested with Miri HEAD with minor changes

Could you please prepare a Miri PR with just those changes? Then we can fix the toolstate ASAP without having to complete the other, much larger, PR.

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2021
@bors
Copy link
Collaborator

bors commented May 22, 2021

☔ The latest upstream changes (presumably #85514) made this pull request unmergeable. Please resolve the merge conflicts.

@ghost ghost mentioned this pull request May 22, 2021
Merged
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2021
@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2021
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2021
@RalfJung
Copy link
Member

LGTM modulo 2 small final nits. :)

@RalfJung
Copy link
Member

Thanks a lot. :-)
@bors r+

@bors
Copy link
Collaborator

bors commented May 28, 2021

📌 Commit f6348f1 has been approved by RalfJung

@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 May 28, 2021
@bors
Copy link
Collaborator

bors commented May 28, 2021

⌛ Testing commit f6348f1 with merge ce0d64e...

@klensy
Copy link
Contributor

klensy commented May 28, 2021

Not squashed commits (
There should really be some check to check if there any overwritten changes and suggest merging commits.

@bors
Copy link
Collaborator

bors commented May 28, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing ce0d64e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 28, 2021
@bors bors merged commit ce0d64e into rust-lang:master May 28, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 28, 2021
@rust-highfive rust-highfive mentioned this pull request May 28, 2021
Closed
@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #85546!

Tested on commit ce0d64e.
Direct link to PR: #85546

💔 miri on windows: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request May 28, 2021
@rust-highfive
📣 Toolstate changed by rust-lang/rust#85546!
cfde3ba 
Tested on commit rust-lang/rust@ce0d64e.
Direct link to PR: <rust-lang/rust#85546>

💔 miri on windows: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @eddyb @oli-obk @RalfJung).
@ghost ghost deleted the unwind branch May 28, 2021 14:01
bors added a commit to rust-lang/miri that referenced this pull request May 28, 2021
@bors
Auto merge of #1812 - hyd-dev:85546, r=RalfJung
4fa9363 
Fix toolstate for rust-lang/rust#85546

cc rust-lang/rust#85780
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@RalfJung RalfJung

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
5 participants
@rust-highfive @RalfJung @bors @klensy @rustbot