Skip to content

fix: add VM Execution State #2043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: add VM Execution State #2043

wants to merge 7 commits into from

Conversation

crStiv
Copy link

@crStiv crStiv commented Apr 1, 2025

Add VM Execution State

Description

Added ExecutionState structure and get_execution_state() method to capture current VM state. Helps with debugging and simplifies integration with external tools.
What's included:

  • Structure with key VM parameters (pc, ap, fp, current_step, etc.)
  • Tests to verify functionality
  • Documentation

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added
  • This change requires new documentation.
  • Documentation updated
  • CHANGELOG has been updated.

FrancoGiachetta
FrancoGiachetta previously approved these changes Apr 1, 2025
@FrancoGiachetta
Copy link
Contributor

FrancoGiachetta commented Apr 1, 2025

The changelog needs to be updated.

@FrancoGiachetta FrancoGiachetta dismissed their stale review April 1, 2025 19:38

I made it by accident

@JulianGCalderon
Copy link
Contributor

Hi @crStiv, could you share a bit more information on the usecases of this new structure? Why are those particular fields necessary?

@crStiv
Copy link
Author

crStiv commented Apr 2, 2025

Hi @crStiv, could you share a bit more information on the usecases of this new structure? Why are those particular fields necessary?

@JulianGCalderon
Hey Julian,
About the ExecutionState, I'll try to be brief, so it basically:

  • Makes debugging way easier - you get all VM state in one place
  • Simplifies testing - no need to access VM internals directly
  • Opens up better tooling options - both IDE plugins and standalone tools

I picked these specijfic fields after some trial and error - they're the minimum set that actualy gives you the full picture of what's hapening in the VM. Happy to go deeper if needed.

@crStiv
Copy link
Author

crStiv commented Apr 2, 2025

The changelog needs to be updated.

@FrancoGiachetta got you, changelog is updated now, and deleted comments to the test as well

@FrancoGiachetta
Copy link
Contributor

Is it possible for you to tell us what you are doing that needs this information at some point in the program?

@crStiv
Copy link
Author

crStiv commented Apr 3, 2025

Is it possible for you to tell us what you are doing that needs this information at some point in the program?

@FrancoGiachetta
Before this structure, I had to directly access multiple VM internals, which was messy and fragile. Having a single method that returns a complete snapshot makes the integration much cleaner and less likely to break with future VM changes.
The ExecutionState provides what external tools need without exposing too many implementation details.

@crStiv
Copy link
Author

crStiv commented Apr 28, 2025

@FrancoGiachetta hi, just wonder what's the status of my PR

@FrancoGiachetta
Copy link
Contributor

Hi @crStiv! We've taken a look at this. You can already almost all the information (except for with run_finished attribute, since it's private). Here is an example of how you could create the same struct in you project:

pub struct ExecutionState {
    /// Current program counter (Program Counter)
    pub pc: Relocatable,
    /// Current allocation pointer (Allocation Pointer)
    pub ap: usize,
    /// Current frame pointer (Frame Pointer)
    pub fp: usize,
    /// Current execution step
    pub current_step: usize,
    /// Number of memory segments
    pub memory_segments_count: usize,
    /// Information about active built-ins
    pub active_builtins: Vec<String>,
}

...

let state = ExecutionState {
    pc: vm.get_pc(),
    ap: vm.get_ap().offset,
    fp: vm.get_fp().offset,
    current_step: vm.get_current_step(),
    memory_segments_count: vm.segments.num_segments(),
    active_builtins: vm.get_builtin_runners()
        .iter()
        .map(|runner| runner.name().to_string())
        .collect(),
}

The only attribute you cannot retrieve from the vm would be run_finished. This is a boolean which is used to prevent marking a range memory cells as accessed, which can only be done if the execution has finished. Is it possible to know why do you need to read that attribute?

@crStiv
Copy link
Author

crStiv commented Apr 30, 2025

@FrancoGiachetta Thanks for your feedback! While assembling this data manually is possible, having a dedicated method provides:

  1. Better maintainability - won't break with future VM changes
  2. Safety - access to run_finished is critical for my debugging tool to know when execution is truly complete before analyzing memory

Without run_finished, my tool can't reliably determine when it's safe to perform certain operations, leading to potential issues.

Would you consider keeping this PR or making that attribute public separately?

@FrancoGiachetta
Copy link
Contributor

FrancoGiachetta commented Apr 30, 2025

The thing is that this method would be public, and thus it would increase the size of the api exposed. Any change to this method or the struct would be a breaking. Since you can already get almost all of these attributes by your self, it wouldn't be appropriate. Talking about run_finished, this value is set when you run a program. I'm not getting what your use case is here. Is it, by any change, possible for to tell?

@crStiv
Copy link
Author

crStiv commented May 1, 2025

@FrancoGiachetta Fair point about the API surface. Looking back at it, my use case probably isn't strong enough to justify this addition.

Feel free to close this PR if you don't see value in it. If there's any part worth salvaging or other ways I could help instead, just let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants