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

[Mem2Reg] Incorrect poison propagation #97702

Closed
nikic opened this issue Jul 4, 2024 · 4 comments · Fixed by #97711
Closed

[Mem2Reg] Incorrect poison propagation #97702

nikic opened this issue Jul 4, 2024 · 4 comments · Fixed by #97711

Comments

@nikic
Copy link
Contributor

nikic commented Jul 4, 2024

; RUN: opt -S -passes=sroa < %a

define i8 @test(i1 %cond) {
start:
  %a = alloca i8, align 1
  br i1 %cond, label %bb1, label %bb2

bb1:
  store i8 poison, ptr %a, align 1
  br label %exit

bb2:
  br label %exit

exit:
  %5 = load i8, ptr %a, align 1
  ret i8 %5
}

Results in:

define i8 @test(i1 %cond) {
  br i1 %cond, label %bb1, label %bb2

bb1:                                              ; preds = %start
  br label %exit

bb2:                                              ; preds = %start
  br label %exit

exit:                                             ; preds = %bb2, %bb1
  ret i8 poison
}

This is incorrect. If %cond is false, then the result must be undef, not poison.

The original test case is more along these lines (not sure yet whether the root cause is always the same):

define i24 @test(i1 %cond) {
start:
  %a = alloca [3 x i8], align 1
  %b = alloca [1 x i8], align 1
  %a.1 = getelementptr inbounds i8, ptr %a, i64 1
  store i8 1, ptr %a, align 1
  store i8 1, ptr %a.1, align 1
  br i1 %cond, label %bb1, label %bb2

bb1:
  %b.1 = getelementptr inbounds i8, ptr %b, i64 1
  %oob = load i8, ptr %b.1, align 1
  %a.2 = getelementptr inbounds i8, ptr %a, i64 2
  store i8 %oob, ptr %a.2, align 1
  br label %exit

bb2:
  br label %exit

exit:
  %5 = load i24, ptr %a, align 1
  ret i24 %5
}
@nikic
Copy link
Contributor Author

nikic commented Jul 4, 2024

It looks like just mem2reg is sufficient for this.

@nikic
Copy link
Contributor Author

nikic commented Jul 4, 2024

Looks like mem2reg calls InstSimplify on phi nodes here:

if (Value *V = simplifyInstruction(PN, SQ)) {

So it looks like we have found the first real-world instance of the #96631 miscompile.

@nikic
Copy link
Contributor Author

nikic commented Jul 4, 2024

After looking a bit closer, my guess in the previous comment wasn't right. The actual root cause is the "single store" optimization in mem2reg, which is not correct if the stored value may be poison.

@nikic
Copy link
Contributor Author

nikic commented Jul 4, 2024

Adding a isGuaranteedNotToBePoison() check to rewriteSingleStoreAlloca() does fix the issue.

nikic added a commit that referenced this issue Jul 4, 2024
nikic added a commit to nikic/llvm-project that referenced this issue Jul 4, 2024
…value

If there is a single store, then loads must either load the stored
value or uninitialized memory (undef). If the stored value may be
poison, then replacing an uninitialized memory load with it would
be incorrect. Fall back to the generic code in that case.

This PR only fixes the case where there is a literal poison store
-- the case where the value is non-trivially poison will be
miscompiled by phi simplification later, see llvm#96631.

Fixes llvm#97702.
@nikic nikic closed this as completed in f58930f Jul 4, 2024
sagebind pushed a commit to sagebind/castaway that referenced this issue Jul 4, 2024
…#25)

Currently some casts from byte primitives to two element tuple lead to
miscompilation on **release** builds on Rust `>=1.70.0`:
- `castaway::cast!(123_u8, (u8, u8))` unexpectedly returns `Ok(...)`
that leads to **UB**.
- `castaway::cast!(false, (bool, u16))` leads to `SIGILL: illegal
instruction` runtime error.

Upstream issues:
- Rust: rust-lang/rust#127286
- LLVM: llvm/llvm-project#97702

I suggest considering adding a safe "workaround" to fix the issue in
this crate without having to wait for the upstream fixes. This way we
will have this fixed in older Rust versions as well.

This PR adds size eq `assert` to `transmute_unchecked`. This workaround
was found while preparing an MRE for an upstream issue.

Checked locally with `cargo test --release` for Rust `1.38`, `1.68.0`,
`1.69.0`, `1.70.0`, `1.71.0`, `1.72.0`, `stable`, `beta`, `nightly`.
Generated assembly for other tests cases for the release build seems the
same (checks and casts are optimized away).

Btw: it might also be a good idea to run tests in `--release` mode as
well since the crate relies heavily on optimizing the casts to
zero-cost.
DianQK pushed a commit to DianQK/llvm-project that referenced this issue Jul 5, 2024
DianQK pushed a commit to DianQK/llvm-project that referenced this issue Jul 5, 2024
…value (llvm#97711)

If there is a single store, then loads must either load the stored value
or uninitialized memory (undef). If the stored value may be poison, then
replacing an uninitialized memory load with it would be incorrect. Fall
back to the generic code in that case.

This PR only fixes the case where there is a literal poison store -- the
case where the value is non-trivially poison will still get miscompiled
by phi simplification later, see llvm#96631.

Fixes llvm#97702.

(cherry picked from commit f58930f)
nikic added a commit to rust-lang/llvm-project that referenced this issue Jul 5, 2024
nikic added a commit to rust-lang/llvm-project that referenced this issue Jul 5, 2024
…value (llvm#97711)

If there is a single store, then loads must either load the stored value
or uninitialized memory (undef). If the stored value may be poison, then
replacing an uninitialized memory load with it would be incorrect. Fall
back to the generic code in that case.

This PR only fixes the case where there is a literal poison store -- the
case where the value is non-trivially poison will still get miscompiled
by phi simplification later, see llvm#96631.

Fixes llvm#97702.

(cherry picked from commit f58930f)
kbluck pushed a commit to kbluck/llvm-project that referenced this issue Jul 6, 2024
kbluck pushed a commit to kbluck/llvm-project that referenced this issue Jul 6, 2024
…value (llvm#97711)

If there is a single store, then loads must either load the stored value
or uninitialized memory (undef). If the stored value may be poison, then
replacing an uninitialized memory load with it would be incorrect. Fall
back to the generic code in that case.

This PR only fixes the case where there is a literal poison store -- the
case where the value is non-trivially poison will still get miscompiled
by phi simplification later, see llvm#96631.

Fixes llvm#97702.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants