squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Rollup merge of #56100 - RalfJung:visiting-generators, r=oli-obk

Browse source 
generator fields are not necessarily initialized

Looking at the MIR we generate for generators, I think we deliberately leave fields of the generator uninitialized in ways that would be illegal if this was a normal struct (or rather, one would have to use `MaybeUninit`). Consider [this example](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=417b4a2950421b726dd7b307e9ee3bec):
```rust
#![feature(generators, generator_trait)]

fn main() {
    let generator = || {
        let mut x = Box::new(5);
        {
            let y = &mut *x;
            *y = 5;
            yield *y;
            *y = 10;
        }
        *x
    };
    let _gen = generator;
}
```

It generates the MIR
```
fn main() -> (){
    let mut _0: ();                      // return place
    scope 1 {
        scope 3 {
        }
        scope 4 {
            let _2: [generator@src/main.rs:4:21: 13:6 for<'r> {std::boxed::Box<i32>, i32, &'r mut i32, ()}]; // "_gen" in scope 4 at src/main.rs:14:9: 14:13
        }
    }
    scope 2 {
        let _1: [generator@src/main.rs:4:21: 13:6 for<'r> {std::boxed::Box<i32>, i32, &'r mut i32, ()}]; // "generator" in scope 2 at src/main.rs:4:9: 4:18
    }

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 0 at src/main.rs:4:9: 4:18
        (_1.0: u32) = const 0u32;        // bb0[1]: scope 0 at src/main.rs:4:21: 13:6
                                         // ty::Const
                                         // + ty: u32
                                         // + val: Scalar(Bits { size: 4, bits: 0 })
                                         // mir::Constant
                                         // + span: src/main.rs:4:21: 13:6
                                         // + ty: u32
                                         // + literal: Const { ty: u32, val: Scalar(Bits { size: 4, bits: 0 }) }
        StorageLive(_2);                 // bb0[2]: scope 1 at src/main.rs:14:9: 14:13
        _2 = move _1;                    // bb0[3]: scope 1 at src/main.rs:14:16: 14:25
        drop(_2) -> bb1;                 // bb0[4]: scope 1 at src/main.rs:15:1: 15:2
    }

    bb1: {
        StorageDead(_2);                 // bb1[0]: scope 1 at src/main.rs:15:1: 15:2
        StorageDead(_1);                 // bb1[1]: scope 0 at src/main.rs:15:1: 15:2
        return;                          // bb1[2]: scope 0 at src/main.rs:15:2: 15:2
    }
}
```
Notice how we only initialize the first field of `_1` (even though it contains a `Box`!), and then assign it to `_2`. This violates the rule "on assignment, all data must satisfy the validity invariant", and hence miri complains about this code.

What this PR effectively does is to change the validity invariant for generators such that it says nothing about the fields of the generator. We behave as if every field of the generator was wrapped in a `MaybeUninit`.

r? @oli-obk

Cc @nikomatsakis @eddyb @cramertj @withoutboats @Zoxc
This commit is contained in:
Pietro Albini 2018-11-25 17:05:02 +01:00 committed by GitHub
commit 45e5a856a6
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
1 changed files with 42 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

54
src/librustc_mir/interpret/visitor.rs
View file

@ -158,7 +158,9 @@ macro_rules! make_value_visitor {
) -> EvalResult<'tcx> {
self.walk_aggregate(v, fields)
}
/// Called each time we recurse down to a field, passing in old and new value.
/// Called each time we recurse down to a field of a "product-like" aggregate
/// (structs, tuples, arrays and the like, but not enums), passing in old and new value.
/// This gives the visitor the chance to track the stack of nested fields that
/// we are descending through.
#[inline(always)]
@ -171,6 +173,19 @@ macro_rules! make_value_visitor {
self.visit_value(new_val)
}
/// Called for recursing into the field of a generator. These are not known to be
/// initialized, so we treat them like unions.
#[inline(always)]
fn visit_generator_field(
&mut self,
_old_val: Self::V,
_field: usize,
new_val: Self::V,
) -> EvalResult<'tcx> {
self.visit_union(new_val)
}
/// Called when recursing into an enum variant.
#[inline(always)]
fn visit_variant(
&mut self,
@ -291,17 +306,33 @@ macro_rules! make_value_visitor {
// use that as an unambiguous signal for detecting primitives. Make sure
// we did not miss any primitive.
debug_assert!(fields > 0);
self.visit_union(v)?;
self.visit_union(v)
},
layout::FieldPlacement::Arbitrary { ref offsets, .. } => {
// FIXME: We collect in a vec because otherwise there are lifetime errors:
// Projecting to a field needs (mutable!) access to `ecx`.
let fields: Vec<EvalResult<'tcx, Self::V>> =
(0..offsets.len()).map(|i| {
v.project_field(self.ecx(), i as u64)
})
.collect();
self.visit_aggregate(v, fields.into_iter())?;
// Special handling needed for generators: All but the first field
// (which is the state) are actually implicitly `MaybeUninit`, i.e.,
// they may or may not be initialized, so we cannot visit them.
match v.layout().ty.sty {
ty::Generator(..) => {
let field = v.project_field(self.ecx(), 0)?;
self.visit_aggregate(v, std::iter::once(Ok(field)))?;
for i in 1..offsets.len() {
let field = v.project_field(self.ecx(), i as u64)?;
self.visit_generator_field(v, i, field)?;
}
Ok(())
}
_ => {
// FIXME: We collect in a vec because otherwise there are lifetime
// errors: Projecting to a field needs access to `ecx`.
let fields: Vec<EvalResult<'tcx, Self::V>> =
(0..offsets.len()).map(|i| {
v.project_field(self.ecx(), i as u64)
})
.collect();
self.visit_aggregate(v, fields.into_iter())
}
}
},
layout::FieldPlacement::Array { .. } => {
// Let's get an mplace first.
@ -317,10 +348,9 @@ macro_rules! make_value_visitor {
.map(|f| f.and_then(|f| {
Ok(Value::from_mem_place(f))
}));
self.visit_aggregate(v, iter)?;
self.visit_aggregate(v, iter)
}
}
Ok(())
}
}
}