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

-Zmir-opt-level miscompiles code with panic #131195

Closed
shao-hua-li opened this issue Oct 3, 2024 · 6 comments · Fixed by #131201 · May be fixed by #131203
Closed

-Zmir-opt-level miscompiles code with panic #131195

shao-hua-li opened this issue Oct 3, 2024 · 6 comments · Fixed by #131201 · May be fixed by #131203
Assignees
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. I-miscompile Issue: Correct Rust code lowers to incorrect machine code 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

@shao-hua-li
Copy link

shao-hua-li commented Oct 3, 2024

I tried this code:

pub fn myfunc() -> i32 {
    let mut c :i32 = 1;
    c = 1 ;
    if !c != 0 {
        return 1;
    }
    panic!("Reached end of non-void function");
}

pub fn main() {
    let e = myfunc();
    println!("e={}", e);
}

I expected to see this happen: always panic

Instead, this happened: only -Zmir-opt-level=2 panic

For mir-opt-level 0, 1 and 3, they all normally return

% rustc -Awarnings -Zmir-opt-level=2 test.rs && ./test
thread 'main' panicked at test.rs:7:5:
Reached end of non-void function
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
%
% rustc -Awarnings -Zmir-opt-level=0 test.rs && ./test
e=1

Meta

rustc --version --verbose:

rustc 1.83.0-nightly (18b1161ec 2024-10-02)
binary: rustc
commit-hash: 18b1161ec9eeab8927f91405bca0ddf59a4a26c9
commit-date: 2024-10-02
host: x86_64-unknown-linux-gnu
release: 1.83.0-nightly
LLVM version: 19.1.0
@shao-hua-li shao-hua-li added the C-bug Category: This is a bug. label Oct 3, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 3, 2024
@jieyouxu jieyouxu added the A-mir-opt Area: MIR optimizations label Oct 3, 2024
@clubby789
Copy link
Contributor

!1 equals -2, so I think the panic case is incorrect?

@shao-hua-li
Copy link
Author

!1 equals -2, so I think the panic case is incorrect?

Yeah, I think so.

@clubby789
Copy link
Contributor

@@ -1,4 +1,4 @@
-// MIR for `myfunc` before JumpThreading
+// MIR for `myfunc` after JumpThreading
 
 fn myfunc() -> i32 {
     let mut _0: i32;
@@ -17,7 +17,7 @@ fn myfunc() -> i32 {
         _4 = copy _1;
         _3 = Not(move _4);
         _2 = Ne(move _3, const 0_i32);
-        switchInt(move _2) -> [0: bb2, otherwise: bb1];
+        goto -> bb2;
     }
 
     bb1: {

Looks like it's JumpThreading that incorrectly removes the condition

@clubby789 clubby789 added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 3, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 3, 2024
@shao-hua-li
Copy link
Author

shao-hua-li commented Oct 3, 2024

I managed to create an example without panic, just wrong value

pub fn myfunc() -> i32 {
    let mut a: i32 = 0;
    a = 1;
    if !a != 0 {
        return 1
    }
    return 0;
}

pub fn main() {
    let mut e = myfunc();
    println!("e={}", e);
}
% rustc -Awarnings test.rs -Zmir-opt-level=0 && ./test
e=1
% rustc -Awarnings test.rs -Zmir-opt-level=2 && ./test
e=0

@clubby789
Copy link
Contributor

found 8 bors merge commits in the specified range
  commit[0] 2024-02-10: Auto merge of #119614 - RalfJung:const-refs-to-static, r=oli-obk
  commit[1] 2024-02-10: Auto merge of #117206 - cjgillot:jump-threading-default, r=tmiasko
  commit[2] 2024-02-11: Auto merge of #120232 - c272:json-buildstd, r=Mark-Simulacrum
  commit[3] 2024-02-11: Auto merge of #120405 - cjgillot:gvn-pointer, r=oli-obk
  commit[4] 2024-02-11: Auto merge of #120914 - ehuss:downgrade-xcode, r=Mark-Simulacrum
  commit[5] 2024-02-11: Auto merge of #120920 - matthiaskrgr:rollup-jsryr8e, r=matthiaskrgr
  commit[6] 2024-02-11: Auto merge of #120903 - matthiaskrgr:rollup-tmsuzth, r=matthiaskrgr
  commit[7] 2024-02-11: Auto merge of #120803 - onur-ozkan:fix-compilation-cache, r=Mark-Simulacrum

Most likely #117206

@jieyouxu jieyouxu added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 3, 2024
@compiler-errors
Copy link
Member

ha, I found the bug. We're optimizing UnOp::Not incorrectly in the jump threading code. Right now it's only written to handle bool correctly. I'll put up a fix 🤔

@rustbot claim

@oli-obk oli-obk added the I-miscompile Issue: Correct Rust code lowers to incorrect machine code label Oct 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 3, 2024
…piler-errors

JumpThreading: fix bitwise not on non-booleans

Fixes rust-lang#131195

Alternative to rust-lang#131201
@bors bors closed this as completed in 11ee3a8 Oct 4, 2024
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 7, 2024
@lqd lqd added the P-critical Critical priority label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. I-miscompile Issue: Correct Rust code lowers to incorrect machine code 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
8 participants