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

Miscompile with -Copt-level=1 after update to LLVM 17 #115681

Closed
djkoloski opened this issue Sep 8, 2023 · 5 comments · Fixed by #115959
Closed

Miscompile with -Copt-level=1 after update to LLVM 17 #115681

djkoloski opened this issue Sep 8, 2023 · 5 comments · Fixed by #115959
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@djkoloski
Copy link
Contributor

djkoloski commented Sep 8, 2023 •
edited
Loading

Minimized from code in the Fuchsia tree:

fn main() {
    enum Bits {
        None = 0x00,
        Low = 0x40,
        High = 0x80,
        Both = 0xC0,
    }

    let value = Box::new(0x40u8);
    let mut out = Box::new(0u8);

    let bits = match *value {
        0x00 => Bits::None,
        0x40 => Bits::Low,
        0x80 => Bits::High,
        0xC0 => Bits::Both,
        _ => return,
    };

    match bits {
        Bits::None | Bits::Low => {
            *out = 1;
        }
        _ => (),
    }

    assert_eq!(*out, 1);
}

This issue was introduced in the nightly-2023-08-09 build, and so it is likely to be a result of the upgrade to LLVM 17 (#114048).

$ RUSTFLAGS="-Copt-level=1" cargo  nightly-2032-08-09 run
thread 'main' panicked at src/main.rs:27:5:
assertion `left == right` failed
  left: 0
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
$ RUSTFLAGS="-Copt-level=1" cargo  nightly-2032-08-08 run
<passes>
@djkoloski djkoloski added the C-bug Category: This is a bug. label Sep 8, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 8, 2023
@djkoloski djkoloski changed the title Missing branch in ASAN builds on nightly Miscompile with -Copt-level=1 on nightly Sep 11, 2023
@djkoloski djkoloski changed the title Miscompile with -Copt-level=1 on nightly Miscompile with -Copt-level=1 after update to LLVM 17 Sep 11, 2023
@djkoloski
Copy link
Contributor Author

I have bisected the LLVM pass to the instcombine pass on main. I'm trying to get a diff of before/after so I can start tracking down the exact location of the incorrect optimization.

@tmiasko tmiasko added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Sep 11, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 11, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 11, 2023
@tmiasko tmiasko added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Sep 11, 2023
@djkoloski
Copy link
Contributor Author

The optimization transforms this switch lookup which appears to correspond to the match:

switch.lookup:                                    ; preds = %"_ZN5alloc5boxed12Box$LT$T$GT$3new17h3cb0a1b6f587dddeE.exit4"
  %switch.idx.mult = mul nsw i8 %8, 64, !dbg !682
  %switch.offset = add nsw i8 %switch.idx.mult, -128, !dbg !682
  call void @llvm.dbg.value(metadata i64 poison, metadata !531, metadata !DIExpression()), !dbg !719
  switch i8 %switch.offset, label �10 [
    i8 0, label �9
    i8 64, label �9
  ], !dbg !720

with only the calculation of %switch.idx.mult and %switch.offset changing from:

%switch.idx.mult = mul nsw i8 %8, 64, !dbg !682
%switch.offset = add nsw i8 %switch.idx.mult, -128, !dbg !682

to:

%switch.idx.mult = shl nsw i8 %8, 6, !dbg !682
%switch.offset = or i8 %switch.idx.mult, -128, !dbg !682

with the rest of the IR remaining unchanged.

The transform of %switch.idx.mult appears to be okay (mul by 64 -> shl by 6). The transform of %switch.offset introduces the issue because although the add instruction is marked as nsw, %switch.idx.mult is equal to 192 which is negative in the signed representation and so subtracting 128 does cause signed underflow.

@tmandry tmandry added P-critical Critical priority A-codegen Area: Code generation WG-llvm Working group: LLVM backend code generation and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. WG-llvm Working group: LLVM backend code generation labels Sep 11, 2023
@djkoloski
Copy link
Contributor Author

The IR output from rustc without any optimization passes outputs is:

switch i8 %8, label �3 [
  i8 0, label �4
  i8 64, label �5
  i8 -128, label �6
  i8 -64, label �7
], !dbg !1471

Which appears correct to me. This suggests that the add nsw is being introduced in a later optimization pass. I'll continue bisecting when I get a chance.

@djkoloski
Copy link
Contributor Author

djkoloski commented Sep 11, 2023 •
edited
Loading

Looks like this add nsw instruction is being generated by a simplifycfg<switch-to-lookup> pass that's run before the instcombine pass.

Before simplifycfg:

switch i8 %8, label �3 [
  i8 2, label �8
  i8 3, label �5
  i8 0, label �6
  i8 1, label �7
], !dbg !682

Which is correct. So the real errant pass is simplifycfg<switch-to-lookup>, it just doesn't cause incorrect x86 to be emitted until a later pass.

@djkoloski
Copy link
Contributor Author

Looks like this issue is known and a fix is in progress upstream at llvm/llvm-project#65882.

This was referenced Sep 14, 2023
Closed
Closed
@nikic nikic mentioned this issue Sep 19, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2023
@bors
Auto merge of rust-lang#115959 - nikic:update-llvm-13, r=cuviper
047fac0 
Update to LLVM 17.0.0

This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2023
@bors
Auto merge of rust-lang#115959 - nikic:update-llvm-13, r=cuviper
e2faa4a 
Update to LLVM 17.0.0

This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 20, 2023
@bors
Auto merge of rust-lang#115959 - nikic:update-llvm-13, r=cuviper
793d5ea 
Update to LLVM 17.0.0

This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.
@bors bors closed this as completed in 531830c Sep 20, 2023
cuviper pushed a commit to cuviper/rust that referenced this issue Sep 22, 2023
@nikic @cuviper
Update to LLVM 17.0.0
fff45e9 
This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.

(cherry picked from commit 531830c)
xobs pushed a commit to betrusted-io/rust that referenced this issue Oct 3, 2023
@nikic @xobs
Update to LLVM 17.0.0
3f7547a 
This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.

(cherry picked from commit 531830c)
plietar pushed a commit to plietar/rust that referenced this issue Oct 18, 2023
@nikic @MabezDev
Update to LLVM 17.0.0
fa21521 
This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.

(cherry picked from commit 531830c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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 a pull request may close this issue.

Update to LLVM 17.0.0 nikic/rust
5 participants
@tmandry @djkoloski @saethlin @rustbot @tmiasko