Encapsulate interpreter stack(s) a bit better

[saethlin]

This is what I was referring to in rust-lang/miri#2647 (comment). We no longer hand out a &mut Vec<Frame> anywhere, so we get control over exactly how and where frames are added to or removed from the stack. All the better-not-be-inconsistent state can be put in a small module that is easy to reason about.

r? @RalfJung

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[oli-obk]

Instead of adding the additional API to the machine, we could also create a Stack trait and assoc type and return it via stack and stack_mut. Not sure if the complexity is worth it, but the API complexity of Machine containing 5 stack_xxx functions is also not great.

[RalfJung]

Yeah the machine trait is growing quite big. Having a Stack trait could help share code between the const_eval and const_prop instances. Not sure if that's worth it though, I am fine either way.

[RalfJung]

These should all have doc comments. (In particular the last two need to explain which frame they return.)

[RalfJung]

This seems to give out mutable access to basically the entire thing; in which sense is this 'encapsulated'?

[saethlin]

This seems to give out mutable access to basically the entire thing; in which sense is this 'encapsulated'?

Yeah, I wasn't thinking about this thoroughly enough until this comment. After poking around for a few days I'm abandoning encapsulation as an approach here.

Originally what stuck out at me was that this interface hands out a &mut Vec<Frame> which would permit all sorts of inserts and removals. That API is probably excessively powerful anyway, but what I was focusing on is that the stack could be arbitrarily resized. Restricting all resizing to a push and pop permits us to insert logic after actually adding the frame to the Vec, fixing up some caching logic for what frames are local.

But IMO what kills this idea is the way the interpreter uses frame_mut() to access a number of fields of the topmost frame. So long as that function exists and can return a &mut Frame, external code can just replace the whole Frame and so we can't know if after any call the topmost Frame's Instance has changed.

I can imagine a few solutions to this:

• Replace all of the code that calls frame_mut() with a specific API that just modifies the one field of interest
• Return a proxy object from frame_mut() instead which wraps references to only the fields from Frame that we want to permit modification of (i.e. not the Instance)
• Replace frame_mut() with a closure-based API, and add logic to repair our caching after the closure

IMO all of these exceed the amount of complexity I'm willing to add in order to avoid the duplicate state consistency issue pointed out in rust-lang/miri#2647 (comment).

[RalfJung]

Would it work to make the instance field private?

[saethlin]

I don't think so, because frame_mut() permits replacing the whole Frame.