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.

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

A regression test for #115145 #118805

Closed
wants to merge 1 commit into from
Closed

A regression test for #115145 #118805

wants to merge 1 commit into from

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Dec 10, 2023 •
edited
Loading

The example here (reduced from the crate deadpool) used to ICE Miri until the fix in #118871

r? @RalfJung

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 10, 2023

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@saethlin
Copy link
Member Author

I swear the tests passed locally, which I thought was odd...

@saethlin
Copy link
Member Author

Oh great this only fails with debug=true

@saethlin saethlin changed the title Remove the interprter's check for project_downcast on unihabited variants Remove the interpreter's check for project_downcast on unihabited variants Dec 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 10, 2023

Could not assign reviewer from: RalfJung.
User(s) RalfJung are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@saethlin saethlin 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 Dec 11, 2023
@RalfJung
Copy link
Member

I assume that diff in the assertion is related to the ICE in the mir-opt test?

That seems to be dataflow_const_prop calling the interpreter on dead code. Maybe ImmTy::offset_ needs to return InterpResult just so that it can return ConstPropNonsense when it is called with uninhabited layout? Obviously there should never be a value of uninhabited type.

Cc @cjgillot , seems like dataflow const prop is unfortunately still doing some things with the interpreter that shouldn't be possible.

@RalfJung RalfJung mentioned this pull request Dec 11, 2023
Merged
@saethlin saethlin force-pushed the project-downcast-uninhabited branch from 81c453d to 3998ee9 Compare December 12, 2023 00:02
@saethlin
Copy link
Member Author

I assume that diff in the assertion is related to the ICE in the mir-opt test?

Yes. I dropped the commit that was making the situation less clear.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung RalfJung mentioned this pull request Dec 12, 2023
Closed
@saethlin saethlin force-pushed the project-downcast-uninhabited branch from 3998ee9 to ace1fa8 Compare December 14, 2023 04:58
@saethlin saethlin changed the title Remove the interpreter's check for project_downcast on unihabited variants A regression test for #115145 Dec 14, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 14, 2023

Could not assign reviewer from: RalfJung.
User(s) RalfJung are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@saethlin
Copy link
Member Author

I've turned this PR into adding a regression test per #115145 (comment)

I agree that keeping the assertion is better than removing it.

@saethlin saethlin linked an issue Dec 14, 2023 that may be closed by this pull request
Miri ICE: InvalidProgram(ConstPropNonsense): uninhabited variant in project_downcast #115145
Closed
@RalfJung
Copy link
Member

Ah sorry only saw this now after I submitted rust-lang/miri#3225 which adds a similar test to Miri directly.

@RalfJung
Copy link
Member

RalfJung commented Dec 14, 2023 •
edited
Loading

I think it'd be good to also have the "futures" test though. Can you add this to tests/pass/async-fn.rs?

That should probably be a Miri PR though.

@saethlin saethlin mentioned this pull request Dec 14, 2023
Merged
@saethlin
Copy link
Member Author

Closing in favor of rust-lang/miri#3226

@saethlin saethlin closed this Dec 14, 2023
@saethlin saethlin deleted the project-downcast-uninhabited branch December 14, 2023 23:25
bors added a commit to rust-lang/miri that referenced this pull request Dec 15, 2023
@bors
Auto merge of #3226 - saethlin:deadpool-test, r=RalfJung
e615426 
Add a regression test for rust#115145

Per rust-lang/rust#118805 (comment)
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Dec 17, 2023
@bors
Auto merge of rust-lang#3226 - saethlin:deadpool-test, r=RalfJung
fcb89b3 
Add a regression test for rust#115145

Per rust-lang#118805 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Miri ICE: InvalidProgram(ConstPropNonsense): uninhabited variant in project_downcast
4 participants
@saethlin @rustbot @rust-log-analyzer @RalfJung