[WIP] Add capability to jump to location in order skip execution of some instructions #1211
Conversation
This currently is a more or less work in progress, it depends on the feature set we want to support this feature. In my opinion, this could be flagged as an experimental feature and if the user has an issue then it's their fault. Maybe it's not a 100% good statement to have. There are cases where this could be useful to have and a user that understands how to use it, will be able to use it to the full extent. I've tested this only on Windows, I'll let the integration test do the job on the other systems. Cross function jumping works as well, but only in the toy example below, I have no clue how the runtime will be affected by this. Fixes go-delve#1045
dlsniper
changed the title
|
I added the |
| @@ -128,6 +128,11 @@ See also: "help on", "help cond" and "help clear"`}, | |||
| {aliases: []string{"step-instruction", "si"}, cmdFn: c.stepInstruction, helpMsg: "Single step a single cpu instruction."}, | |||
| {aliases: []string{"next", "n"}, cmdFn: c.next, helpMsg: "Step over to next source line."}, | |||
| {aliases: []string{"stepout"}, cmdFn: c.stepout, helpMsg: "Step out of the current function."}, | |||
| {aliases: []string{"jump", "j"}, cmdFn: executionPoint, helpMsg: `Jump execution to a new location. This will NOT execute the lines that are skipped. | |||
|
|
|||
| break <linespec> | |||
There was a problem hiding this comment.
You need to run go run scripts/gen-cli-docs.go to regenerate the command line documentation. I wouldn't worry about the test failure on appveyor, I think it's spurious.
In my opinion, this could be flagged as an experimental feature and if the user has an issue then it's their fault
Agreed.
Cross function jumping works as well, but only in the toy example below, I have no clue how the runtime will be affected by this.
I think we should at least check that this isn't done since the check is really easy, just compare d.target.CurrentThread().Location().Fn with the output of d.target.BinInfo().PCToFunc(addr).
| func breakpoint(t *Term, ctx callContext, args string) error { | ||
| return setBreakpoint(t, ctx, false, args) | ||
| } | ||
|
|
||
| func executionPoint(t *Term, ctx callContext, args string) error { | ||
| return setExecutionPoint(t, ctx, args) |
There was a problem hiding this comment.
I think setExecutionPoint should just be inlined here, since it's not called anywhere else.
| @@ -1063,10 +1068,49 @@ func setBreakpoint(t *Term, ctx callContext, tracepoint bool, argstr string) err | |||
| return nil | |||
| } | |||
|
|
|||
| func setExecutionPoint(t *Term, ctx callContext, argstr string) error { | |||
| args := strings.SplitN(argstr, " ", 2) | |||
There was a problem hiding this comment.
locspecs can contain spaces the better thing to do is to use all the argstr as locspec and check that it's != ""
| requestedEp := &api.ExecutionPoint{} | ||
| locspec := "" | ||
| if len(args) != 1 { | ||
| return fmt.Errorf("address required") |
There was a problem hiding this comment.
Other commands return errors.New("not enough arguments") in this situation, this should to for uniformity's sake.
| if len(args) != 1 { | ||
| return fmt.Errorf("address required") | ||
| } else { | ||
| locspec = argstr |
There was a problem hiding this comment.
The if above returns, this doesn't need to be inside an else body.
|
|
||
| locs, err := t.client.FindLocation(ctx.Scope, locspec) | ||
| if err != nil { | ||
| locspec = argstr |
There was a problem hiding this comment.
I don't understand what's going on.
| return err | ||
| } | ||
| } | ||
| for _, loc := range locs { |
There was a problem hiding this comment.
Jumping multiple times is idempotent to just doing the last jump, this should either reject the case where there are multiple locations, jump to the first returned location or jump to the last returned location (I think for now the appropriate thing is rejecting).
| @@ -73,6 +73,16 @@ type Breakpoint struct { | |||
| TotalHitCount uint64 `json:"totalHitCount"` | |||
| } | |||
|
|
|||
| // ExecutionPoint addresses a location at which process execution will jump to. | |||
| type ExecutionPoint struct { | |||
There was a problem hiding this comment.
This type is basically the same thing as an api.Location. Given that it's constructed starting from an api.Location I think we should pass in the api.Location directly.
Alternatively just pass the address.
| d.processMutex.Lock() | ||
| defer d.processMutex.Unlock() | ||
|
|
||
| if len(requestedExecPoint.File) > 0 { |
There was a problem hiding this comment.
I know this is what we are doing for breakpoints, but that's because of backwards compatibility. Given that the intended use of this API is to always call FindLocations first I would lean towards not repeating the sins of the past.
|
@dlsniper any chance you'll be able to pick this up soon? |
|
Closing this due to lack of response. @dlsniper please rebase and reopen when you have the bandwidth to continue working on this. |
This currently is a more or less work in progress, it depends on the feature set we want to support this feature. In my opinion, this could be flagged as an experimental feature and if the user has an issue then it's their fault. Maybe it's not a 100% good statement to have. There are cases where this could be useful to have and a user that understands how to use it, will be able to use it to the full extent.
I've tested this only on Windows, I'll let the integration test do the job on the other systems.
Cross function jumping works as well, but only in the toy example below, I have no clue how the runtime will be affected by this.
Fixes #1045