nspcc-dev / neo-go

Go Node and SDK for the Neo blockchain
MIT License
123 stars 79 forks source link

vm: add RemoveBreakPoint support #3674

Closed ixje closed 3 days ago

ixje commented 5 days ago

Problem

https://github.com/nspcc-dev/neo-go/issues/3673

Solution

add function

AnnaShaleva commented 5 days ago

Contribution guidelines workflow is failing, could you please add a signed-off-by line to the commit message? Ref. https://github.com/nspcc-dev/.github/blob/master/git.md#commit-messages.

codecov[bot] commented 5 days ago

Codecov Report

Attention: Patch coverage is 65.38462% with 9 lines in your changes missing coverage. Please review.

Project coverage is 83.14%. Comparing base (66fbcb2) to head (d8ea410). Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
cli/vm/cli.go 57.14% 6 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3674 +/- ## ========================================== + Coverage 83.08% 83.14% +0.06% ========================================== Files 334 334 Lines 46590 46599 +9 ========================================== + Hits 38710 38746 +36 + Misses 6310 6279 -31 - Partials 1570 1574 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ixje commented 5 days ago

Slightly off-topic but relevant for me. Are you open to exposing the following? https://github.com/nspcc-dev/neo-go/blob/dda2cafdf8f7a30905bb0b8516b41ec20dce2c21/pkg/vm/context.go#L33 and https://github.com/nspcc-dev/neo-go/blob/dda2cafdf8f7a30905bb0b8516b41ec20dce2c21/pkg/vm/context.go#L68-L69

because if not then I'll have to fork anyway and we can skip this PR and #3675 to save you some time. I need access to these slots or I can't display stack frame information. I'm aware of the dumpSlot() output but that has some issues and I'd prefer to transfer the information to the IDE in a binary format

AnnaShaleva commented 4 days ago

Are you open to exposing the following?

If an external application needs it, then I don't have objections. But (c *Context) Dump*Slot methods exist because vm.slot structure is unexported and I think we should not export it because it's an internal VM structure. Hence, I'd suggest to make a set of func (c *Context) *SlotBytes() []byte helpers in a separate commit that will serialize slots using binary representation instead of JSON.

but that has some issues

Could you please clarify, which issues? Because I'd expect dumpSlot to output a proper JSON that may be reused by other applications.

we can skip this PR and https://github.com/nspcc-dev/neo-go/issues/3675

Let's finalize it, I consider this PR useful.

ixje commented 4 days ago

vm.slot structure is unexported and I think we should not export it because it's an internal VM structure.

I understand this is sensitive data that shouldn't be changed, but how are they different from the current Estack() and Istack() methods? Those equally give you enough power to wreck the VM state.

Hence, I'd suggest to make a set of func (c *Context) *SlotBytes() []byte helpers in a separate commit that will serialize slots using binary representation instead of JSON.

I'll consume whatever is made available :pray:

Could you please clarify, which issues? Because I'd expect dumpSlot to output a proper JSON that may be reused by other applications.

Assume the following

type DebugContext {
   StackFrames []StackFrame  `json:"stackFrames"`
}

type StackFrame struct {
    Arguments string `json:"arguments"`
    Statics   string `json:"statics"`
    Locals    string `json:"locals"`
    NextIP    int    `json:"nextIp"`
    CurIP     int    `json:"curIp"`
}
...
    for i, ctx := range ic.VM.Istack() {
        frames[i] = StackFrame{
            Arguments: ctx.DumpArgumentsSlot(),
            Statics:   ctx.DumpStaticSlot(),
            Locals:    ctx.DumpLocalSlot(),
            NextIP:    ctx.NextIP(),
            CurIP:     ctx.IP(),
        }
    }

marshalling this gives for example

{"stackFrames":[{"arguments":"[\n    {\n        \"type\": \"Integer\",\n        \"value\": \"1\"\n    }\n]","statics":"[]","locals":"[\n    null,\n    null\n]","nextIp":3,"curIp":0}]}

1 problem here is that arguments (as are statics & locals) is a string not an array. So to get it into a usable format I have 2 choices

  1. on the neo-go side, unmarshal the dumpSlot returned string into a slice of some new type. Then when the whole DebugContext is marshaled arguments will be a proper array
  2. on the consumer side read the value of e.g. arguments, then run another JSON parser instance on the value.
AnnaShaleva commented 4 days ago

but how are they different from the current Estack() and Istack() methods?

From that point you're right, so to be honest I don't have any strong objections against of creating a set of func (c *Context) *Slot() Slot methods. Let's implement this.

ixje commented 4 days ago

I can't seem to reply to https://github.com/nspcc-dev/neo-go/pull/3674#discussion_r1838090081 so I'll do it here

Hence, the output of list breakpoints contains not only user-defined-breakpoints. This fact should be mentioned in the command doc.

I did not add a mention because I believe the current implementation of step n is flawed (see https://github.com/nspcc-dev/neo-go/issues/3676). I expect the solution to not use a break point, therefore it won't show up.

ixje commented 3 days ago

if this ain't the right format then I give up

AnnaShaleva commented 3 days ago

if this ain't the right format

It's the right one!

AnnaShaleva commented 3 days ago

BTW, please also add Close #3673. to the commit message to auto-close related GH issue after merge.