[vm rewrite] Swap to a more step-driven interpreter #19219
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
external-crates/move/crates/move-cli/tests/sandbox_tests/print_stack_trace/args.exp
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
| if !control_flow_instruction(instruction) { | ||
| state.current_frame.pc += 1; | ||
| } |
There was a problem hiding this comment.
Thoughts on instead early returning from the control flow instructions?
Here we could then debug_assert that the state.current_frame.pc hadn't changed yet
There was a problem hiding this comment.
The profiling code would make that quite a bit noisier, which seems more-likely to introduce errors.
| let resolver = fun_ref.get_resolver(link_context, loader); | ||
|
|
||
| let return_values = eval::call_native_with_args( | ||
| None, |
There was a problem hiding this comment.
Does it make sense for this to be None? Especially since it holds VMRuntimeLimitsConfig. Should we instead make an empty state of some sort?
There was a problem hiding this comment.
I had the same initial reaction, though for me looking at the code and try to change that made it more clear why we are doing it.
But all of that (including our NativeContext definition) made me think we really need to review native invocation
There was a problem hiding this comment.
We could pass the limits separately. The only place this was used is in printing the stack trace.
external-crates/move/crates/move-vm-runtime/src/interpreter/state.rs
Outdated
Show resolved
Hide resolved
| pub(crate) fn run( | ||
| function: ArenaPointer<Function>, |
There was a problem hiding this comment.
Lots to think about here for me. At some point in the far future, it would be very interesting to basically call step, or at least "run" here to invoke a function, with a given state, instead of having the custom execution engine we have today for PTBs
external-crates/move/crates/move-cli/tests/sandbox_tests/print_stack_trace/args.exp
Outdated
Show resolved
Hide resolved
| let resolver = fun_ref.get_resolver(link_context, loader); | ||
|
|
||
| let return_values = eval::call_native_with_args( | ||
| None, |
There was a problem hiding this comment.
I had the same initial reaction, though for me looking at the code and try to change that made it more clear why we are doing it.
But all of that (including our NativeContext definition) made me think we really need to review native invocation
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
| let charge_result = gas_meter.charge_simple_instr(SimpleInstruction::Ret); | ||
| profile_close_instr!(gas_meter, format!("{:?}", instruction)); | ||
|
|
||
| partial_error_to_error(state, run_context, charge_result)?; | ||
| let non_ref_vals = state | ||
| .current_frame | ||
| .locals | ||
| .drop_all_values() | ||
| .map(|(_idx, val)| val); | ||
|
|
||
| // TODO: Check if the error location is set correctly. | ||
| gas_meter | ||
| .charge_drop_frame(non_ref_vals.into_iter()) | ||
| .map_err(|e| state.set_location(e))?; |
There was a problem hiding this comment.
this may very well be a "fix all things gas" issue, but I do not believe we charge anything for dropping a frame (lines 162-171) and yet we got calls and check and a TODO....
thedrop_all_values part may actually do something useful (maybe not sure) but...
There was a problem hiding this comment.
This TODO was there in the old code -- not sure what to do about it. Let's revisit gas in the future.
cgswords
force-pushed
the
cgswords/arena_loader_1
branch
from
79f8453
to
d49dc5d
Compare
cgswords
force-pushed
the
cgswords/state_machine_interpreter
branch
from
d3170e0
to
1ca0e7a
Compare
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-vm-runtime/src/interpreter/eval.rs
Outdated
Show resolved
Hide resolved
cgswords
force-pushed
the
cgswords/state_machine_interpreter
branch
from
1ca0e7a
to
7f1ad8c
Compare
Description
This splits up the interpreter to be more step-by-step than the previous loop with control flags approach. It also closes some of the circuit on ArenaPointer usage.
The
move-executionchanges are just due tocargo fmt --all.Test plan
All tests still pass
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.