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

Miscompilation due to MatchBranchSimplification MIR pass mixing up discriminants #124150

Closed
RalfJung opened this issue Apr 19, 2024 · 13 comments
Closed

Miscompilation due to MatchBranchSimplification MIR pass mixing up discriminants #124150

RalfJung opened this issue Apr 19, 2024 · 13 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Apr 19, 2024 •
edited
Loading

#120614 introduced a miscompilation; turns out this question poked at incorrect logic in the MatchBranchSimplification MIR pass.

#124122 is a WIP patch to fix this (and also contains a testcase that currently gets miscompiled), but as a critical soundness issue this deserves tracking nonetheless.
@RalfJung RalfJung added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Apr 19, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 19, 2024
@RalfJung
Copy link
Member Author

Cc @rust-lang/wg-mir-opt

@DianQK DianQK mentioned this issue Apr 19, 2024
Closed
@workingjubilee workingjubilee added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Apr 19, 2024
@Noratrieb
Copy link
Member

does it make sense to quickly throw up a PR disabling it to leave more time for fixing it properly and thoroughly?

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 19, 2024
@DianQK
Copy link
Member

DianQK commented Apr 19, 2024

does it make sense to quickly throw up a PR disabling it to leave more time for fixing it properly and thoroughly?

I like this: #124122 (comment).
I think that disabling some transforms where miscompilation might occur is feasible in #124122.

@Noratrieb
Copy link
Member

Noratrieb commented Apr 19, 2024 •
edited
Loading

yeah, could you put up a quick PR to disable it, gating it behind unsound MIR opts?

@DianQK
Copy link
Member

DianQK commented Apr 19, 2024

yeah, could you put up a quick PR to disable it, gating it behind unsound MIR opts?

I will do it today. :)

@DianQK
Copy link
Member

DianQK commented Apr 19, 2024

For me, but it's more appropriate to disable unsigned and signed conversions in #124122. But might this require a longer review? I'm not sure, but at least the conversions that are retained should be clear.
@oli-obk Do you have any suggestions?

@DianQK
Copy link
Member

DianQK commented Apr 19, 2024

For me, but it's more appropriate to disable unsigned and signed conversions in #124122.

This is just the usual operation on LLVM: disabling certain conversions, rather than disabling the entire Pass. Of course, compared to LLVM's passes, the passes in MIR-opt are much smaller.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 19, 2024
@DianQK
Copy link
Member

DianQK commented Apr 19, 2024

yeah, could you put up a quick PR to disable it, gating it behind unsound MIR opts?

I've rethought this scenario, and it's appropriate to add unsound opts. Because this pass is mostly all in #120614. :)

@Noratrieb
Copy link
Member

I think disabling the entire pass should have a very small impact, it's not like this is the LLVM instcombine pass :D. So that makes more sense, it's usually how we do it for less important MIR opt passes.

@DianQK DianQK mentioned this issue Apr 19, 2024
Merged
@DianQK
Copy link
Member

DianQK commented Apr 19, 2024

I caused a P-critical issue for the first time. :p

ltratt added a commit to ltratt/yk that referenced this issue Apr 19, 2024
@ltratt
Use a known-working version of Rust nightly.
df056f0 
Rust nightly, as of yesterday, breaks yk. We *think* this is because of
rust-lang/rust#124150 --- if so, hopefully we
can switch back to "nightly nightly" in a day or two.
@ltratt ltratt mentioned this issue Apr 19, 2024
Merged
@RalfJung
Copy link
Member Author

mir-opts work has a pretty high chance of doing that. Unfortunately we haven't yet figured out how to build correct-by-construction optimization passes outside of full-blown theorem provers...

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 19, 2024
@bors
Auto merge of rust-lang#124156 - DianQK:disable-match_branches, r=<try>
e7a5d6f 
Disable MatchBranchSimplification

Due to the miscompilation mentioned in rust-lang#124150, We need to disable MatchBranchSimplification temporarily.

To fully resolve this issue, my plan is:
1. Disable MatchBranchSimplification (this PR).
2. Remove all potentially unclear transforms in rust-lang#124122.
3. Gradually add back the removed transforms (possibly multiple PRs).

r? `@Nilstrieb` or `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 20, 2024
@bors
Auto merge of rust-lang#124156 - DianQK:disable-match_branches, r=Ral…
a61b14d 
…fJung

Disable SimplifyToExp in MatchBranchSimplification

Due to the miscompilation mentioned in rust-lang#124150, We need to disable MatchBranchSimplification temporarily.

To fully resolve this issue, my plan is:
1. Disable SimplifyToExp in MatchBranchSimplification (this PR).
2. Remove all potentially unclear transforms in rust-lang#124122.
3. Gradually add back the removed transforms (possibly multiple PRs).

r? `@Nilstrieb` or `@oli-obk`
@RalfJung
Copy link
Member Author

RalfJung commented Apr 20, 2024 •
edited
Loading

#124156 landed so the soundness bug is fixed by disabling the affected optimization. Thanks @DianQK for the quick fix. :)

@DianQK DianQK mentioned this issue Jul 7, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 3, 2024
@bors
Auto merge of rust-lang#127324 - DianQK:match-br, r=cjgillot
bbf60c8 
Simplify match based on the cast result of `IntToInt`

Continue to complete rust-lang#124150. The condition in rust-lang#120614 is wrong, e.g. `-1i8` cannot be converted to `255i16`. I've rethought the issue and simplified the conditional judgment for a more straightforward approach. The new approach is to check **if the case value after the `IntToInt` conversion equals the target value**.

In different types, `IntToInt` uses different casting methods. This rule is as follows:

- `i8`/`u8` to `i8`/`u8`: do nothing.
- `i8` to `i16`/`u16`: sign extension.
- `u8` to `i16`/`u16`: zero extension.
- `i16`/`u16` to `i8`/`u8`: truncate to the target size.

The previous error was a mix of zext and sext.

r? mir-opt
@compiler-errors compiler-errors added the A-mir-opt Area: MIR optimizations label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@RalfJung @compiler-errors @apiraino @DianQK @workingjubilee @rustbot @Noratrieb