vm: add RemoveBreakPoint support #3674

ixje · 2024-11-12T09:32:29Z

Problem

#3673

Solution

add function

AnnaShaleva

LGTM.

AnnaShaleva · 2024-11-12T10:20:37Z

pkg/vm/vm.go

@@ -275,6 +275,14 @@ func (v *VM) AddBreakPoint(n int) {
 	ctx.sc.breakPoints = append(ctx.sc.breakPoints, n)
 }

+// RemoveBreakPoint removes the breakpoint in the current context.
+func (v *VM) RemoveBreakPoint(n int) {


BTW, can you also add a support of respective VM CLI command? Similar to

neo-go/cli/vm/cli.go

Line 104 in 66fbcb2

Name: "break",

If not, then let's move it to a separate issue.

should I also add a list breakpoints?

To me it's a useful feature. It may be supported as a separate command and may be even as an extension of (v *VM) PrintOps (some special symbol may be added to every output line that has a breakpoint).

But keep in mind that step command is also handled with the help of breakpoints:

neo-go/cli/vm/cli.go

Line 1191 in 66fbcb2

v.AddBreakPointRel(n)

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

Another note is that we now actually have a set of breakpoint-related commands, hence we may group them under a single breakpoints command as a set of subcommands, something like: breakpoints break, breakpoints remove and breakpoints list:

var commands = []*cli.Command{ { Name: "breakpoints", Subcommands: []*cli.Command{ { Name: "break", ... }, { Name: "remove", ... }, { Name: "list", ... }, }, },

But it's an incompatible change. @roman-khimov, what do you think?

I think man gdb.

Well, I had to be more clear: what do you think about incompatibility of this change, can we afford this?

Regarding gdb: it has the same concept as proposed above: gdb breakpoints has a subset of breakpoint-related commands including break, clear, delete. Although I haven't find something similar to list to display all breakpoints.

ib lists breakpoints in gdb

incompatibility of this change

If you really want to debug something with this CLI you won't be happy typing "breakpoints break" and alike, these:

break, brea, bre, br, b -- Set breakpoint at specified location. delete, del, d -- Delete all or some breakpoints.

are all available at the top level in GDB. Also, good commands start with verbs.

Strict compatibility is not critical for this CLI (and can be done with some transition period), but likely we can extend it in a compatible way now.

OK, let's then agree on compatible extension and use delete and ib for breakpoints removal and listing correspondingly.

AnnaShaleva · 2024-11-12T10:22:53Z

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 · 2024-11-12T10:25:22Z

Codecov Report

Attention: Patch coverage is 27.77778% with 13 lines in your changes missing coverage. Please review.

Project coverage is 83.06%. Comparing base (66fbcb2) to head (d91a6d2).

Files with missing lines	Patch %	Lines
cli/vm/cli.go	0.00%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3674      +/-   ##
==========================================
- Coverage   83.08%   83.06%   -0.03%     
==========================================
  Files         334      334              
  Lines       46590    46608      +18     
==========================================
+ Hits        38710    38714       +4     
- Misses       6310     6325      +15     
+ Partials     1570     1569       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cli/vm/cli.go

AnnaShaleva · 2024-11-12T13:20:18Z

pkg/vm/vm.go

@@ -275,6 +275,14 @@ func (v *VM) AddBreakPoint(n int) {
 	ctx.sc.breakPoints = append(ctx.sc.breakPoints, n)
 }

+// RemoveBreakPoint removes the breakpoint in the current context.
+func (v *VM) RemoveBreakPoint(n int) {


To me it's a useful feature. It may be supported as a separate command and may be even as an extension of (v *VM) PrintOps (some special symbol may be added to every output line that has a breakpoint).

But keep in mind that step command is also handled with the help of breakpoints:

neo-go/cli/vm/cli.go

Line 1191 in 66fbcb2

v.AddBreakPointRel(n)

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

AnnaShaleva · 2024-11-12T13:25:16Z

pkg/vm/vm.go

@@ -275,6 +275,14 @@ func (v *VM) AddBreakPoint(n int) {
 	ctx.sc.breakPoints = append(ctx.sc.breakPoints, n)
 }

+// RemoveBreakPoint removes the breakpoint in the current context.
+func (v *VM) RemoveBreakPoint(n int) {


Another note is that we now actually have a set of breakpoint-related commands, hence we may group them under a single breakpoints command as a set of subcommands, something like: breakpoints break, breakpoints remove and breakpoints list:

var commands = []*cli.Command{ { Name: "breakpoints", Subcommands: []*cli.Command{ { Name: "break", ... }, { Name: "remove", ... }, { Name: "list", ... }, }, },

But it's an incompatible change. @roman-khimov, what do you think?

cli/vm/cli.go

Some extensions need to be reviewed.

ixje · 2024-11-12T15:31:18Z

Slightly off-topic but relevant for me. Are you open to exposing the following?

neo-go/pkg/vm/context.go

Line 33 in dda2caf

static slot

and

neo-go/pkg/vm/context.go

Lines 68 to 69 in dda2caf

    
           local     slot 
        
           arguments slot

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 · 2024-11-13T08:33:12Z

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 #3675

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

ixje · 2024-11-13T12:59:52Z

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 🙏

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

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
on the consumer side read the value of e.g. arguments, then run another JSON parser instance on the value.

AnnaShaleva · 2024-11-13T13:36:31Z

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.

Signed-off-by: ixje <erik@coz.io>

ixje · 2024-11-13T14:10:13Z

I can't seem to reply to #3674 (comment) 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
#3676). I expect the solution to not use a break point, therefore it won't show up.

ixje requested review from AnnaShaleva and roman-khimov as code owners November 12, 2024 09:32

AnnaShaleva previously approved these changes Nov 12, 2024

View reviewed changes

AnnaShaleva reviewed Nov 12, 2024

View reviewed changes

ixje force-pushed the remove-breakpoint branch from 7f7b264 to d91a6d2 Compare November 12, 2024 11:17

AnnaShaleva reviewed Nov 12, 2024

View reviewed changes

vm: add RemoveBreakPoint support

31706a7

Signed-off-by: ixje <erik@coz.io>

ixje force-pushed the remove-breakpoint branch from d91a6d2 to 31706a7 Compare November 13, 2024 13:52

ixje mentioned this pull request Nov 13, 2024

vm: make slots public #3677

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: add RemoveBreakPoint support #3674

vm: add RemoveBreakPoint support #3674

ixje commented Nov 12, 2024

AnnaShaleva left a comment

AnnaShaleva Nov 12, 2024

ixje Nov 12, 2024

AnnaShaleva Nov 12, 2024

AnnaShaleva Nov 12, 2024 •

edited

Loading

roman-khimov Nov 12, 2024

AnnaShaleva Nov 12, 2024

ixje Nov 12, 2024

roman-khimov Nov 12, 2024

AnnaShaleva Nov 13, 2024

AnnaShaleva commented Nov 12, 2024

codecov bot commented Nov 12, 2024 •

edited

Loading

AnnaShaleva Nov 12, 2024

AnnaShaleva Nov 12, 2024 •

edited

Loading

ixje commented Nov 12, 2024

AnnaShaleva commented Nov 13, 2024

ixje commented Nov 13, 2024

AnnaShaleva commented Nov 13, 2024

ixje commented Nov 13, 2024

vm: add RemoveBreakPoint support #3674

Are you sure you want to change the base?

vm: add RemoveBreakPoint support #3674

Conversation

ixje commented Nov 12, 2024

Problem

Solution

AnnaShaleva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva commented Nov 12, 2024

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AnnaShaleva Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

ixje commented Nov 12, 2024

AnnaShaleva commented Nov 13, 2024

ixje commented Nov 13, 2024

AnnaShaleva commented Nov 13, 2024

ixje commented Nov 13, 2024

AnnaShaleva Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

AnnaShaleva Nov 12, 2024 •

edited

Loading