interpret StorageLive & StorageDead, and check dead stack slots are not used #176

RalfJung · 2017-06-01T00:57:30Z

(This is work-in-progress, test suite currently fails.)

This changes the stack slots to be of type Option<Value>, where None represents the case of a "dead" stack slot. StorageLive/StorageDead mark slots as live/dead. So this also effectively defines the semantics of these commands. Right now, the semantics are as follows:

When a function is called, miri checks which local variables have StorageLive/StorageDead call on them. The ones that do are initialized as "dead", while the ones that don't are initialized as "live (undef value)".
When a local variable is accessed (read from or written to), and it is dead, that's an error.
When StorageLive is run, and the variable is already live, that's an error. Otherwise, the variable becomes live.
When StorageDead is run, the variable becomes dead. If it was allocated in memory, that memory gets deallocated. Redundant StorageDead are fine.

The third bullet point above is the most questionable. In particular, it makes the test suite fail in loops.rs. Have a look at _4 in factorial_loop in http://play.integer32.com/?gist=08a6b697560aabb9bd8165efcbd8b082&version=nightly. As the program goes around the loop, it runs StorageLive twice without running StorageDead in between, so miri bails out. LLVM is unfortunately unclear on whether this is legal or not. @eddyb mentioned there were some bugs with redundant StorageLive, or maybe I misunderstood?
(Test suite with MIR-libstd fails every test, I am still investigating that.)

Cc @nikomatsakis

Fixes #49.

…ot used

RalfJung · 2017-06-01T00:59:30Z

MIR-libstd fails in flush_buf: https://github.com/rust-lang/rust/blob/2277f4bdcc1195a5f6c9c96d8d1fb32482cbd673/src/libstd/io/buffered.rs#L385

It has a loop, so I assume it's the same issue.

eddyb · 2017-06-01T04:51:46Z

The bugs qere in old trans, where we weren't always consistent about scopes. FWIW you could experiment yourself by using #[link_name = "llvm.lifetime.start"] with FFI to call the intrinsics from Rust code.

RalfJung · 2017-06-01T05:38:01Z

Well, I doubt I will be able to find interesting things by randomly adding such calls. The questions: Do we have hard evidence that redundant StorageLive are harmful? When we talked about this in IRC (#rustc) earlier today, rkruppe quoted LLVM devs saying that they should be fine: https://botbot.me/mozilla/rustc/2017-06-01/?msg=86639833&page=1

eddyb · 2017-06-01T05:41:43Z

LLVM optimizations are relatively easy to trigger: just read & write a variable and see how IR changes with lifetime intrinsics. As for the actual bugs, it's been a while, although @arielb1 might remember better.

oli-obk · 2017-06-01T06:42:41Z

src/eval_context.rs

@@ -452,10 +453,33 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
    ) -> EvalResult<'tcx> {
        ::log_settings::settings().indentation += 1;

+        /// Return the set of locals that have a stroage annotation anywhere


Typo storage

oli-obk · 2017-06-01T06:49:00Z

src/eval_context.rs

        let num_locals = mir.local_decls.len() - 1;
-        let locals = vec![Value::ByVal(PrimVal::Undef); num_locals];
+        let mut locals = Vec::with_capacity(num_locals);
+        for i in 0..num_locals {


Init locals with vec![None; num_locals] and use the loop from the function to change to Some

Actually.. why are you marking these as alive here and not at the storagelive call?

This marks those variables as live that do not have a StorageLive call.

Oh. I didn't see the inversion.

arielb1 · 2017-06-01T15:58:00Z

@RalfJung

I'm not sure whether multiple lifetime.start in a row are valid.

bjorn3 · 2017-06-01T16:28:37Z

src/lvalue.rs

@@ -130,7 +130,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
                Ok(Value::ByRef(ptr))
            }
            Lvalue::Local { frame, local, field } => {
-                Ok(self.stack[frame].get_local(local, field.map(|(i, _)| i)))
+                Ok(self.stack[frame].get_local(local, field.map(|(i, _)| i))?)


Ok( and ?) is redundant

bjorn3 · 2017-06-01T16:29:13Z

src/step.rs

+            StorageLive(ref lvalue) | StorageDead(ref lvalue)=> {
+                let (frame, local) = match self.eval_lvalue(lvalue)? {
+                    Lvalue::Local{ frame, local, field: None } if self.stack.len() == frame+1 => (frame, local),
+                    _ => return Err(EvalError::Unimplemented("Stroage annotations must refer to locals of the topmost stack frame.".to_owned())) // FIXME maybe this should get its own error type


RalfJung · 2017-06-02T01:01:57Z

I fixed the nits you brought up. I also adapted the suggestion of @arielb1 to make StorageLive kill the content of the current stack slot if the slot is already live, rather than erroring out. This is consistent with everything we saw from LLVM so far, so maybe it's good enough? Anyway it makes the test suite pass, even with libstd-MIR. So I feel like this can be merged, and we can still make StorageLive more restrictive later if we decide that is the better semantics.

Finally, I opened a bug in rustc proper about StorageLive in loops: rust-lang/rust#42371.

RalfJung · 2017-06-02T01:04:07Z

Github says "Changes requested", but I think I made all the requested changes... how do I clear that flag?

bjorn3 · 2017-06-02T04:41:59Z

You didnt fix the spell error at step.rs line 133 yet.

RalfJung · 2017-06-02T05:15:59Z

oops sorry, missed that one

interpret StorageLive & StorageDead, and check dead stack slots are n…

e6eaf20

…ot used

oli-obk requested changes Jun 1, 2017

View reviewed changes

bjorn3 suggested changes Jun 1, 2017

View reviewed changes

RalfJung added 2 commits June 1, 2017 11:01

fix some nits

db6ce46

make StorageLive kill the current value of the local

dd7735b

RalfJung changed the title ~~WIP: interpret StorageLive & StorageDead, and check dead stack slots are not used~~ interpret StorageLive & StorageDead, and check dead stack slots are not used Jun 2, 2017

oli-obk approved these changes Jun 2, 2017

View reviewed changes

Fix typo

ec7f1d5

oli-obk merged commit d7d11c1 into rust-lang:master Jun 2, 2017

RalfJung deleted the storage branch June 2, 2017 05:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interpret StorageLive & StorageDead, and check dead stack slots are not used #176

interpret StorageLive & StorageDead, and check dead stack slots are not used #176

RalfJung commented Jun 1, 2017

RalfJung commented Jun 1, 2017

eddyb commented Jun 1, 2017 •

edited

Loading

RalfJung commented Jun 1, 2017

eddyb commented Jun 1, 2017

oli-obk Jun 1, 2017

oli-obk Jun 1, 2017

oli-obk Jun 1, 2017

RalfJung Jun 1, 2017

oli-obk Jun 1, 2017

arielb1 commented Jun 1, 2017

bjorn3 Jun 1, 2017

bjorn3 Jun 1, 2017

RalfJung commented Jun 2, 2017

RalfJung commented Jun 2, 2017

bjorn3 commented Jun 2, 2017

RalfJung commented Jun 2, 2017

interpret StorageLive & StorageDead, and check dead stack slots are not used #176

interpret StorageLive & StorageDead, and check dead stack slots are not used #176

Conversation

RalfJung commented Jun 1, 2017

RalfJung commented Jun 1, 2017

eddyb commented Jun 1, 2017 • edited Loading

RalfJung commented Jun 1, 2017

eddyb commented Jun 1, 2017

oli-obk Jun 1, 2017

Choose a reason for hiding this comment

oli-obk Jun 1, 2017

Choose a reason for hiding this comment

oli-obk Jun 1, 2017

Choose a reason for hiding this comment

RalfJung Jun 1, 2017

Choose a reason for hiding this comment

oli-obk Jun 1, 2017

Choose a reason for hiding this comment

arielb1 commented Jun 1, 2017

bjorn3 Jun 1, 2017

Choose a reason for hiding this comment

bjorn3 Jun 1, 2017

Choose a reason for hiding this comment

RalfJung commented Jun 2, 2017

RalfJung commented Jun 2, 2017

bjorn3 commented Jun 2, 2017

RalfJung commented Jun 2, 2017

eddyb commented Jun 1, 2017 •

edited

Loading