migrate move_vm_stack to TraceState#18
migrate move_vm_stack to TraceState#18lbr77 wants to merge 16 commits intoBitsLabSec:v1.65.2-pendingfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates from using move_vm_stack::Stack directly to a new TraceState abstraction that tracks the operand stack, local variables, and global state during Move VM execution. The migration involves creating a new TraceState struct that maintains VM state by processing trace events, along with supporting infrastructure for instruction-level tracing through a new event.rs module.
Changes:
- Introduced
TraceStateto replace directmove_vm_stack::Stackusage for tracking VM execution state - Added
event.rsmodule withNotifierTracerandTraceNotifiertrait for enhanced trace event handling - Updated all oracle implementations and tracer infrastructure to use
TraceStateinstead ofStackparameter
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/movy-replay/src/tracer/trace.rs | New file defining TraceState struct that reconstructs VM state from trace events |
| crates/movy-replay/src/tracer/oracle.rs | Updated SuiGeneralOracle::event signature to accept &TraceState instead of Option<&Stack> |
| crates/movy-replay/src/tracer/op.rs | Added TryFrom implementations for converting SerializableMoveValue and TraceValue to Magic |
| crates/movy-replay/src/tracer/mod.rs | Added trace module export |
| crates/movy-replay/src/tracer/fuzz.rs | Refactored to use TraceState for stack operations, split instruction handling into separate method |
| crates/movy-replay/src/tracer/concolic.rs | Migrated from move_vm_stack::Stack to TraceState, updated helper functions for stack access |
| crates/movy-replay/src/tracer/tree.rs | Inlined move_vm_stack::Stack import in function signature |
| crates/movy-replay/src/lib.rs | Added event module export |
| crates/movy-replay/src/event.rs | New file with NotifierTracer, TraceNotifier, and module loading infrastructure |
| crates/movy-fuzz/src/oracles/sui/*.rs | Updated all oracle event methods to use &TraceState parameter |
| crates/movy-fuzz/src/executor.rs | Integrated NotifierTracer with FuzzModuleProvider for module-aware tracing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //! state (operand stack, locals, and globals) at each point in time during execution. | ||
| //! | ||
| //! The memory tracer then emits `External` events with the current VM state for every instruction, | ||
| //! and open/close frame event that is has built up. |
There was a problem hiding this comment.
Typo in the documentation. "is has" should be "it has".
| //! and open/close frame event that is has built up. | |
| //! and open/close frame event that it has built up. |
| // if self.stack.len() != stack.len() && stack.is_empty() { | ||
| // self.stack.clear(); | ||
| // } |
There was a problem hiding this comment.
Commented-out code should be removed if it is no longer needed. If this code is still relevant for future development, consider documenting why it's kept with a more detailed comment explaining its purpose and when it might be re-enabled.
| // if self.stack.len() != stack.len() && stack.is_empty() { | |
| // self.stack.clear(); | |
| // } |
| // self.module_stack = Some((address, name)); | ||
| self.module_stack.push((address, name)); | ||
| if let Err(e) = self.notifier.notify_event(event) { | ||
| log::error!("NotifierTracer: failed to notify OpenFrame: {:?}", e); | ||
| } | ||
| } | ||
|
|
||
| TraceEvent::CloseFrame { | ||
| frame_id: _, | ||
| return_: _, | ||
| gas_left: _, | ||
| } => { | ||
| // self.module_stack = None; |
There was a problem hiding this comment.
Commented-out code should be removed if it is no longer needed. If these comments document the evolution from Option to Vec for module_stack, they should be removed now that the implementation is complete.
| module_provider: P, | ||
| module_stack: Vec<(AccountAddress, String)>, | ||
| module_cache: BTreeMap<(AccountAddress, String), CompiledModule>, | ||
| _phantom: PhantomData<P>, |
There was a problem hiding this comment.
The _phantom field of type PhantomData<P> is unnecessary because the generic parameter P is already used in the module_provider field. This PhantomData can be removed.
| .event(event, trace_state, symbol_stack, current_function.clone(), state)? | ||
| .into_iter() | ||
| .chain( | ||
| self.1 | ||
| .event(event, stack, symbol_stack, current_function, state)?, | ||
| .event(event, trace_state, symbol_stack, current_function, state)? |
There was a problem hiding this comment.
The current_function parameter is cloned on line 95 when passed to the first oracle, but passed by reference to the second oracle on line 99. This inconsistency could be unintentional. Both calls should use the same approach - either both clone or both use references.
| }; | ||
|
|
There was a problem hiding this comment.
Duplicate lines detected. Lines 244-245 are identical. This appears to be a copy-paste error - only one check for the tracing level is needed.
| for ev in events.iter() { | ||
| if let Some((st, ev)) = state.fuzz_state().decode_sui_event(ev)? { |
There was a problem hiding this comment.
Duplicate lines detected. Lines 248-249 are identical. This appears to be a copy-paste error - only one tracing::debug! call is needed.
| ); | ||
| } else { |
There was a problem hiding this comment.
Duplicate lines detected. Lines 256-257 are identical. This appears to be a copy-paste error - only one tracing::debug! call is needed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Manually merged in 96dbba1 |
2 participants
based on branch v1.65.2