bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Auto merge of #113569 - RalfJung:miri, r=oli-obk

Browse source 
miri: protect Move() function arguments during the call

This gives `Move` operands a meaning specific to function calls:
- for the duration of the call, the place the operand comes from is protected, making all read and write accesses insta-UB.
- the contents of that place are reset to `Uninit`, so looking at them again after the function returns, we cannot observe their contents

Turns out we can replace the existing "retag return place" hack with the exact same sort of protection on the return place, which is nicely symmetric.

Fixes https://github.com/rust-lang/rust/issues/112564
Fixes https://github.com/rust-lang/miri/issues/2927

This starts with a Miri rustc-push, since we'd otherwise conflict with a PR that recently landed in Miri.
(The "miri tree borrows" commit is an unrelated cleanup I noticed while doing the PR. I can remove it if you prefer.)
r? `@oli-obk`
This commit is contained in:
bors 2023-07-12 10:19:42 +00:00
commit 136dab6614
37 changed files with 747 additions and 161 deletions
Download patch file Download diff file Expand all files Collapse all files

14
compiler/rustc_middle/src/mir/syntax.rs
View file

@ -1050,10 +1050,6 @@ pub type PlaceElem<'tcx> = ProjectionElem<Local, Ty<'tcx>>;
/// there may be other effects: if the type has a validity constraint loading the place might be UB
/// if the validity constraint is not met.
///
/// **Needs clarification:** Ralf proposes that loading a place not have side-effects.
/// This is what is implemented in miri today. Are these the semantics we want for MIR? Is this
/// something we can even decide without knowing more about Rust's memory model?
///
/// **Needs clarification:** Is loading a place that has its variant index set well-formed? Miri
/// currently implements it, but it seems like this may be something to check against in the
/// validator.
@ -1071,6 +1067,16 @@ pub enum Operand<'tcx> {
/// in [UCG#188]. You should not emit MIR that may attempt a subsequent second load of this
/// place without first re-initializing it.
///
/// **Needs clarification:** The operational impact of `Move` is unclear. Currently (both in
/// Miri and codegen) it has no effect at all unless it appears in an argument to `Call`; for
/// `Call` it allows the argument to be passed to the callee "in-place", i.e. the callee might
/// just get a reference to this place instead of a full copy. Miri implements this with a
/// combination of aliasing model "protectors" and putting `uninit` into the place. Ralf
/// proposes that we don't want these semantics for `Move` in regular assignments, because
/// loading a place should not have side-effects, and the aliasing model "protectors" are
/// inherently tied to a function call. Are these the semantics we want for MIR? Is this
/// something we can even decide without knowing more about Rust's memory model?
///
/// [UCG#188]: https://github.com/rust-lang/unsafe-code-guidelines/issues/188
Move(Place<'tcx>),