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

ConstProp misoptimises pointer-typed enum field #118328

Closed
cbeuw opened this issue Nov 26, 2023 · 9 comments · Fixed by #118426
Closed

ConstProp misoptimises pointer-typed enum field #118328

cbeuw opened this issue Nov 26, 2023 · 9 comments · Fixed by #118426
Assignees
Labels
A-const-prop Area: Constant propagation A-mir-opt Area: MIR optimizations I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cbeuw
Copy link
Contributor

cbeuw commented Nov 26, 2023

Fuzzer generated MIR, reduced, and UB-free under Miri (for real this time 😅)

#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;

#[custom_mir(dialect = "runtime", phase = "initial")]
fn fn4() {
    mir! {
    let _1: isize;
    let _12: Adt55;
    let unit: ();
    {
    _12 = Adt55::Variant1 { fld0: 0, fld1: 0};
    SetDiscriminant(_12, 0);
    place!(Field::<*mut isize>(Variant(_12, 0), 0)) = core::ptr::addr_of_mut!(_1);
    Call(unit = fn19(Field::<*mut isize>(Variant(_12, 0), 0)), bb11, UnwindUnreachable())
    }
    bb11 = {
    Return()
    }
    }
}
#[custom_mir(dialect = "runtime", phase = "initial")]
pub fn fn19(mut _1: *mut isize) {
    mir! {
    {
    (*_1) = 1;
    Return()
    }
    }
}
pub fn main() {
    fn4();
    println!("here");
}
#[derive(Debug, Copy, Clone)]
pub enum Adt55 {
    Variant0 { fld0: *mut isize },
    Variant1 { fld0: u8, fld1: u64 },
}

Segfaults with ConstProp enabled:

$ rustc -Zmir-opt-level=0 -Copt-level=0 -Zmir-enable-passes=+ConstProp repro.rs &&
 ./repro
Segmentation fault (core dumped)

Field::<*mut isize>(Variant(_12, 0), 0)), which is a valid pointer, somehow got propagated as 0:

// MIR for `fn4` before ConstProp

fn fn4() -> () {
    let mut _0: ();
    let mut _1: isize;
    let mut _2: Adt55;
    let mut _3: ();

    bb0: {
        _2 = Adt55::Variant1 { fld0: const 0_u8, fld1: const 0_u64 };
        discriminant(_2) = 0;
        ((_2 as variant#0).0: *mut isize) = &raw mut _1;
        _3 = fn19(((_2 as variant#0).0: *mut isize)) -> [return: bb1, unwind unreachable];
    }

    bb1: {
        return;
    }
}
// MIR for `fn4` after ConstProp

fn fn4() -> () {
    let mut _0: ();
    let mut _1: isize;
    let mut _2: Adt55;
    let mut _3: ();

    bb0: {
        _2 = Adt55::Variant1 { fld0: const 0_u8, fld1: const 0_u64 };
        discriminant(_2) = 0;
        ((_2 as variant#0).0: *mut isize) = &raw mut _1;
        _3 = fn19(const {0x0 as *mut isize}) -> [return: bb1, unwind unreachable];
    }

    bb1: {
        return;
    }
}
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 26, 2023
@Noratrieb Noratrieb added 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 A-mir-opt Area: MIR optimizations A-const-prop Area: Constant propagation and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 26, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 26, 2023
@workingjubilee
Copy link
Member

workingjubilee commented Nov 27, 2023

Field::<*mut isize>(Variant(_12, 0), 0)), which is a valid pointer, somehow got propagated as 0:

That seems undesirable.

Is there a surface Rust program that can produce this MIR? Asking primarily for prioritization purposes.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 27, 2023
@tmiasko
Copy link
Contributor

tmiasko commented Nov 27, 2023

Minimized test case:

#![feature(custom_mir, core_intrinsics, inline_const)]
extern crate core;
use core::intrinsics::mir::*;

#[custom_mir(dialect = "runtime", phase = "initial")]
fn size_of<T>() -> usize {
    mir! {
        let a : usize;
        {
            a = 0;
            a = const { std::mem::size_of::<T>() };
            RET = a;
            Return()
        }
    }
}

fn main() {
    assert_eq!(size_of::<u32>(), std::mem::size_of::<u32>());
}
$ rustc    a.rs && ./a
$ rustc -O a.rs && ./a
thread 'main' panicked at a.rs:19:5:
assertion `left == right` failed
  left: 0
 right: 4
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@apiraino apiraino added P-high High priority and removed P-medium Medium priority labels Nov 28, 2023
@lqd
Copy link
Member

lqd commented Nov 28, 2023

Here's an example using stable surface syntax (playground).

#![allow(unused_assignments)]

struct SizeOfConst<T>(std::marker::PhantomData<T>);
impl<T> SizeOfConst<T> {
    const SIZE: usize = std::mem::size_of::<T>();
}

fn size_of<T>() -> usize {
    let mut a = 0;
    a = SizeOfConst::<T>::SIZE;
    a
}

fn main() {
    assert_eq!(size_of::<u32>(), std::mem::size_of::<u32>());
}

Seems to have been introduced in 1.70, with #108872 in particular.

@Urgau
Copy link
Member

Urgau commented Nov 28, 2023

Using @lqd minimization it seems to be a LLVM miscompilation as Alive2 complains about the optimizations applied.

Nevermind seems like I was wrong.

define noundef i64 @src() {
  %a = alloca i64, align 8
  store i64 0, ptr %a, align 8
  store i64 4, ptr %a, align 8
  %_0 = load i64, ptr %a, align 8
  ret i64 %_0
}

define noundef i64 @tgt() {
  ret i64 0
}

@aDotInTheVoid
Copy link
Member

It's not an LLVM miscompilation, the code is already ret i64 0 by the time it goes to LLVM. https://godbolt.org/z/r1W13h1sY.

With rustc +nightly ./lol.rs -Z dump-mir=size_of -O

--- ./mir_dump/lol.size_of.005-015.ConstProp.before.mir 2023-11-28 14:38:07.519258465 +0000
+++ ./mir_dump/lol.size_of.005-015.ConstProp.after.mir  2023-11-28 14:38:07.519258465 +0000
@@ -1,4 +1,4 @@
-// MIR for `size_of` before ConstProp
+// MIR for `size_of` after ConstProp

 fn size_of() -> usize {
     let mut _0: usize;
@@ -11,7 +11,7 @@
         StorageLive(_1);
         _1 = const 0_usize;
         _1 = const _;
-        _0 = _1;
+        _0 = const 0_usize;
         StorageDead(_1);
         return;
     }

So it's defiantly happening in MIR ConstProp

rust-cloud-vms bot pushed a commit to aDotInTheVoid/rust that referenced this issue Nov 28, 2023
@aDotInTheVoid
Copy link
Member

The problem seems to be that when running on the statement _1 = const _;, the frames arn't updated, so when it runs on _0 = _1;, it thinks 0x0 is their for local _1.

│ │ ├─┐rustc_mir_transform::const_prop::visit_statement statement=_1 = const _, location=bb0[2]
│ │ │ ├─  0ms TRACE rustc_mir_transform::const_prop initial frame, frame=[LocalState { value: Live(Immediate(Uninit)), ty: None }, LocalState { value: Live(Immediate(Scalar(0x0000000000000000))), ty: Some(usize) }]
│ │ │ ├─┐rustc_mir_transform::const_prop::visit_assign place=_1, rvalue=const _, location=bb0[2]
│ │ │ │ ├─┐rustc_mir_transform::const_prop::visit_operand operand=const _, location=bb0[2]
│ │ │ │ │ ├─  0ms TRACE rustc_mir_transform::const_prop about to do it
│ │ │ │ ├─┘
│ │ │ │ ├─┐rustc_mir_transform::const_prop::check_rvalue rvalue=const _
│ │ │ │ ├─┘
│ │ │ ├─┘
│ │ │ ├─  0ms TRACE rustc_mir_transform::const_prop final frame, frame=[LocalState { value: Live(Immediate(Uninit)), ty: None }, LocalState { value: Live(Immediate(Scalar(0x0000000000000000))), ty: Some(usize) }]
│ │ ├─┘

./x build library --keep-stage 1 && RUSTC_LOG=rustc_mir_transform::const_prop=trace,rustc_const_eval::interpret=trace ./build/host/stage1/bin/rustc ./tests/mir-opt/const_prop/issue_118328.rs -O 2> log.txt, on aDotInTheVoid@aef5074 to add logging and test.

(full log)

@aDotInTheVoid
Copy link
Member

@rustbot claim

@bors bors closed this as completed in 434232f Nov 29, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 29, 2023
Rollup merge of rust-lang#118426 - aDotInTheVoid:const-wat, r=compiler-errors,cjgillot

ConstProp: Correctly remove const if unknown value assigned to it.

Closes rust-lang#118328

The problematic sequence of MIR is:

```rust
          _1 = const 0_usize;
          _1 = const _; // This is an associated constant we can't know before monomorphization.
          _0 = _1;
```

1. When `ConstProp::visit_assign` happens on `_1 = const 0_usize;`, it records that `0x0usize` is the value for `_1`.
2. Next `visit_assign` happens on `_1 = const _;`. Because the rvalue `.has_param()`, it can't be const evaled.
3. Finaly, `visit_assign` happens on `_0 = _1;`. Here it would think the value of `_1` was `0x0usize` from step 1.

The solution is to remove consts when checking the RValue fails, as they may have contained values that should now be invalidated, as that local was overwritten.

This should probably be back-ported to beta. Stable is more iffy, as it's gone unidentified since 1.70, so I only think it's worthwhile if there's another reason for a 1.74.1 release anyway.
kjetilkjeka pushed a commit to kjetilkjeka/rust that referenced this issue Nov 30, 2023
cuviper pushed a commit to cuviper/rust that referenced this issue Dec 1, 2023
(cherry picked from commit b1a6cf4)
@RalfJung
Copy link
Member

RalfJung commented Dec 2, 2023

Cc @cjgillot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-prop Area: Constant propagation A-mir-opt Area: MIR optimizations I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants