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

Prevent MIRI ICE by bubbling up #100967

Closed
wants to merge 4 commits into from
Closed

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Aug 24, 2022

This fixes MIRI ICE which resulted from this work.

r? @oli-obk

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2022
@ouz-a ouz-a mentioned this pull request Aug 24, 2022
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

@bors try

Does this also fix the ICE in a local build with debug assertions? Or just the new ICE that started when #99383 landed?

@bors
Copy link
Contributor

bors commented Aug 24, 2022

⌛ Trying commit 8b0690a with merge 4ef10657f99de0ed2da88f2ce0114812b9927fbf...

@ouz-a
Copy link
Contributor Author

ouz-a commented Aug 24, 2022

Does this also fix the ICE in a local build with debug assertions? Or just the new ICE that started when #99383 landed?

Only the new one, I would like to fix debug assertion one too but I don't know where to even begin with that.

@RalfJung
Copy link
Member

Fair.^^

@compiler-errors
Copy link
Member

If we end up landing this, I can put up a fix to change the debug assertion in a way that's not as risky as just removing it.

I'm gonna continue to see if we can revert the underlying root cause (the fact we currently don't normalize opaques with escaping bound vars).

@bors
Copy link
Contributor

bors commented Aug 24, 2022

☀️ Try build successful - checks-actions
Build commit: 4ef10657f99de0ed2da88f2ce0114812b9927fbf (4ef10657f99de0ed2da88f2ce0114812b9927fbf)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 25, 2022

If we end up landing this, I can put up a fix to change the debug assertion in a way that's not as risky as just removing it.

I'm gonna continue to see if we can revert the underlying root cause (the fact we currently don't normalize opaques with escaping bound vars).

do you want to keep trying or should we merge this PR now and you can work on a revert of it?

@compiler-errors
Copy link
Member

Working on it in #100980, but I'm not sure how much of a perf improvement is needed to make it worth landing. If @RalfJung really needs this ICE to go away, then we can land this and I can work on top of a revert of this.

@RalfJung
Copy link
Member

Well, a debug assertion that can actually be triggered is always a bug, and makes me worried about which things might break in later parts of the compiler due to the invariant violation. So it should probable at least become a regular assertion to ensure correct execution. And then it's still an ICE, which is a bug -- but it needs a nightly feature to be triggered. I also assume if it can be triggered in Miri there is some way to trigger it with rustc, though I don't know how.

I don't know of any real-life code that runs into this, though.

@oli-obk oli-obk added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 16, 2022
@compiler-errors
Copy link
Member

Superseded by #100980 I think

@oli-obk oli-obk closed this Sep 23, 2022
@ouz-a ouz-a deleted the issue_57961 branch November 13, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants