Skip to content

interpret: fix vtable check debug assertion #99607

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 1 commit into from
Jul 23, 2022
Merged

interpret: fix vtable check debug assertion #99607

merged 1 commit into from
Jul 23, 2022

Conversation

RalfJung
Copy link
Member

Fixes #99605
Thanks to @eddyb for suggesting the fix!

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 22, 2022
@rust-highfive
Copy link
Contributor

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@RalfJung RalfJung marked this pull request as ready for review July 22, 2022 14:37
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

eddyb
eddyb approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

LGTM/r=me - only one thing I wanted to ask before then though, is this testable on this side, or only in the miri repo?

For the record, I believe a minimal test would look something like this:

(Box::new(|| {}) as Box<dyn FnOnce()>)()

(just checked and there is indeed a ::{shim:vtable#0} generated for such code)

@RalfJung
Copy link
Member Author

is this testable on this side, or only in the miri repo?

Yeah it can be tested in a rustc checkout (with debug assertions enabled) via ./x.py test src/tools/miri --stage 1. (Stage 0 will also work again soon.)

@RalfJung
Copy link
Member Author

@bors r=eddyb

@bors
Copy link
Collaborator

bors commented Jul 22, 2022

📌 Commit 19e29e9 has been approved by eddyb

It is now in the queue for this repository.

@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 22, 2022
@eddyb
Copy link
Member

eddyb commented Jul 22, 2022

is this testable on this side, or only in the miri repo?

Yeah it can be tested in a rustc checkout (with debug assertions enabled)

Sorry, I meant whether it's possible to add a test in rust-lang/rust at all, for this situation.
But I suppose that would only be the case if CTFE could cause a virtual call?

@RalfJung
Copy link
Member Author

Ah I see. Yeah I don't think CTFE can reach this... well, it could with -Zunleash-the-miri-inside-of-you I guess?

@bors r-

@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 22, 2022
@RalfJung RalfJung mentioned this pull request Jul 22, 2022
Merged
@RalfJung
Copy link
Member Author

No that won't work, it still dynamically checks that we only call const fn.

@bors r=eddyb

@bors
Copy link
Collaborator

bors commented Jul 22, 2022

📌 Commit 19e29e9 has been approved by eddyb

It is now in the queue for this repository.

@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 22, 2022
@RalfJung RalfJung mentioned this pull request Jul 22, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 23, 2022
@bors
Auto merge of rust-lang#99623 - RalfJung:rollup-0h066kc, r=RalfJung
8aedb9c 
Rollup of 3 pull requests

Successful merges:

 - rust-lang#99588 (Update books)
 - rust-lang#99602 (cargotest: do not run quickcheck tests in xsv)
 - rust-lang#99607 (interpret: fix vtable check debug assertion)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 25de227 into rust-lang:master Jul 23, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 23, 2022
@RalfJung RalfJung deleted the vtable-check branch July 23, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@cjgillot cjgillot

Labels
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
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.

Miri tests fail with debug assertions enabled
6 participants
@RalfJung @rust-highfive @rustbot @bors @eddyb @cjgillot