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

MIR building optimizes away some place expressions around "_" patterns even with -Zmir-opt-level=0 #99180

Closed
RalfJung opened this issue Jul 12, 2022 · 5 comments · Fixed by #103208
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

Consider the following code -- the comments say it all:

fn main() {
    let ptr = 32 as *const u8;
    
    // This is UB, and Miri finds it:
    //unsafe { let _ptr = std::ptr::addr_of!(*ptr); }
    
    // So this should also be UB, but Miri says nothing:
    unsafe { let _ = *ptr; }
    
    // And same for this:
    unsafe {
        match *ptr { _ => {} }
    }
}

Here is the mir-opt-level=0 MIR for this:

fn main() -> () {
    let mut _0: ();                      // return place in scope 0 at place.rs:1:11: 1:11
    let _1: *const u8;                   // in scope 0 at place.rs:2:9: 2:12
    let _2: ();                          // in scope 0 at place.rs:8:5: 8:29
    scope 1 {
        debug ptr => _1;                 // in scope 1 at place.rs:2:9: 2:12
        scope 2 {
            scope 3 {
            }
        }
        scope 4 {
        }
    }

    bb0: {
        StorageLive(_1);                 // scope 0 at place.rs:2:9: 2:12
        _1 = const 32_usize as *const u8 (PointerFromExposedAddress); // scope 0 at place.rs:2:15: 2:30
        StorageLive(_2);                 // scope 1 at place.rs:8:5: 8:29
        _2 = const ();                   // scope 2 at place.rs:8:5: 8:29
        StorageDead(_2);                 // scope 1 at place.rs:8:28: 8:29
        _0 = const ();                   // scope 4 at place.rs:12:27: 12:29
        StorageDead(_1);                 // scope 0 at place.rs:14:1: 14:2
        return;                          // scope 0 at place.rs:14:2: 14:2
    }
}

Currently, MIR building "loses" the entire place expression somewhere, even though that place expression could cause UB. IMO that is a bug. Can we change MIR building to preserve the place expressions?

We could have a EvalPlace MIR statement that just computes a place expression and then does nothing with the result. (To be clear, there should be no place-to-value conversion, i.e., no actual load from the *ptr place. But the place expression itself should be evaluated.) We do have FakeRead, which works on a place, but (a) its docs talk about "inspecting constants and discriminant values" so that is doing more than just evaluating the place expression, and (b) it is "disallowed after drop elaboration".

Cc @rust-lang/wg-mir-opt in lieu of a "MIR building wg".
Also I think this is related to THIR unsafety checking; MIR unsafety checking does not even complain about some of these *ptr because they get erased before reaching MIR, but in THIR they would still be visible.
Also related: #79735 rust-lang/unsafe-code-guidelines#261

@SkiFire13
Copy link
Contributor

    // So this should also be UB, but Miri says nothing:
    unsafe { let _ = *ptr; }

This doesn't even need the unsafe block, see #80059

@RalfJung
Copy link
Member Author

Yeah that's what I mentioned in the last paragraph about THIR unsafety checking.

@jyn514 jyn514 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 C-bug Category: This is a bug. labels Apr 15, 2023
@RalfJung
Copy link
Member Author

This got fixed by #102256.

@RalfJung
Copy link
Member Author

This required -Zmir-keep-place-mention though. The set of flags Miri needs to set keeps growing.^^

@RalfJung
Copy link
Member Author

Also turns out the match version is still lost. That's probably a separate issue though. For now we track it at rust-lang/miri#2360.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 27, 2023
…RalfJung

Allow partially moved values in match

This PR attempts to unify the behaviour between `let _ = PLACE`, `let _: TY = PLACE;` and `match PLACE { _ => {} }`.
The logical conclusion is that the `match` version should not check for uninitialised places nor check that borrows are still live.

The `match PLACE {}` case is handled by keeping a `FakeRead` in the unreachable fallback case to verify that `PLACE` has a legal value.

Schematically, `match PLACE { arms }` in surface rust becomes in MIR:
```rust
PlaceMention(PLACE)
match PLACE {
  // Decision tree for the explicit arms
  arms,
  // An extra fallback arm
  _ => {
    FakeRead(ForMatchedPlace, PLACE);
    unreachable
  }
}
```

`match *borrow { _ => {} }` continues to check that `*borrow` is live, but does not read the value.
`match *borrow {}` both checks that `*borrow` is live, and fake-reads the value.

Continuation of ~rust-lang#102256 ~rust-lang#104844

Fixes rust-lang#99180 rust-lang#53114
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Oct 28, 2023
Allow partially moved values in match

This PR attempts to unify the behaviour between `let _ = PLACE`, `let _: TY = PLACE;` and `match PLACE { _ => {} }`.
The logical conclusion is that the `match` version should not check for uninitialised places nor check that borrows are still live.

The `match PLACE {}` case is handled by keeping a `FakeRead` in the unreachable fallback case to verify that `PLACE` has a legal value.

Schematically, `match PLACE { arms }` in surface rust becomes in MIR:
```rust
PlaceMention(PLACE)
match PLACE {
  // Decision tree for the explicit arms
  arms,
  // An extra fallback arm
  _ => {
    FakeRead(ForMatchedPlace, PLACE);
    unreachable
  }
}
```

`match *borrow { _ => {} }` continues to check that `*borrow` is live, but does not read the value.
`match *borrow {}` both checks that `*borrow` is live, and fake-reads the value.

Continuation of ~rust-lang/rust#102256 ~rust-lang/rust#104844

Fixes rust-lang/rust#99180 rust-lang/rust#53114
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. 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.

3 participants