VM 2 Tracing #20848
VM 2 Tracing #20848
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
cgswords
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
cgswords
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| vtables: &'vm_cache VMDispatchTables, | ||
| vm_config: Arc<VMConfig>, | ||
| extensions: &'native mut NativeContextExtensions<'native_lifetimes>, | ||
| // TODO: consider making this `Option<&mut VMTracer<'_>>` and passing it like that everywhere? | ||
| tracer: &'tracer mut Option<VMTracer<'trace_builder>>, |
There was a problem hiding this comment.
See comment lower down, but I kinda like the way this turned out at the entry points so I'd be in favor of keeping it as-is here.
| @@ -161,9 +181,17 @@ fn step( | |||
|
|
|||
| profile_close_frame!( | |||
| gas_meter, | |||
| arena::to_ref(current_frame.function).pretty_string() | |||
| state.call_stack.current_frame.function().pretty_string() | |||
There was a problem hiding this comment.
I think this is a bug in the current tracer as well, but I think above you'll need to do something like
let err = gas_meter.charge_drop_frame(...).map_err(|e| ..);
profile_close_frame!( ...);
trace(..., err.as_ref().err());
err?;Otherwise if the charge for the drop frame causes an OOG you wont see it in the trace.
| @@ -75,3 +87,5 @@ macro_rules! set_err_info { | |||
| } | |||
|
|
|||
| pub(crate) use set_err_info; | |||
|
|
|||
| use super::tracing::tracer::VMTracer; | |||
There was a problem hiding this comment.
Seems a weird place for this use (and the above one too). Move to top of the file?
| @@ -163,6 +209,7 @@ impl<'extensions> MoveVM<'extensions> { | |||
| function_name: &IdentStr, | |||
| type_arguments: Vec<Type>, | |||
| serialized_args: Vec<impl Borrow<[u8]>>, | |||
| tracer: Option<&mut MoveTraceBuilder>, | |||
There was a problem hiding this comment.
I like this. I think this assuages the comment higher about whether we wanted &mut Option<VMTracer> or Option<&mut VMTracer> -- I think I like it the way it is here.
| } | ||
|
|
||
| /// Similar to execute_entry_function, but it bypasses visibility checks and accepts a tracer | ||
| pub fn execute_function_bypass_visibility_with_tracer_if_enabled( |
There was a problem hiding this comment.
Not for this PR, but thoughts on eventually rolling this into one function execute_function_bypass_visibility?
Description
Add tracing to VM rewrite
Test plan
Old 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.