terminal: improve 'on' command (#2556)

* terminal: improve 'on' command Adds the ability to edit the list of commands executed after stopping on a breakpoint, as well as converting a breakpoint into a tracepoint and vice versa. Prior to this it was possible to add commands to a breakpoint but removing commands or changing a breakpoint into a tracepoint, or vice versa, could only be done by removing and recreating the breakpoint.
2021-07-22 19:16:42 +02:00 · 2021-07-22 19:16:42 +02:00 · 8989073548
commit 8989073548
parent b7d8edcdaf
3 changed files with 240 additions and 46 deletions
--- a/Documentation/cli/README.md
+++ b/Documentation/cli/README.md
@ -464,9 +464,18 @@ Aliases: n
 ## on
 Executes a command when a breakpoint is hit.

-	on <breakpoint name or id> <command>.
+	on <breakpoint name or id> <command>
+	on <breakpoint name or id> -edit
+	

-Supported commands: print, stack and goroutine)
+Supported commands: print, stack, goroutine, trace and cond. 
+To convert a breakpoint into a tracepoint use:
+	
+	on <breakpoint name or id> trace
+
+The command 'on <bp> cond <cond-arguments>' is equivalent to 'cond <bp> <cond-arguments>'.
+
+The command 'on x -edit' can be used to edit the list of commands executed when the breakpoint is hit.


 ## print
--- a/pkg/terminal/command.go
+++ b/pkg/terminal/command.go
@ -10,6 +10,7 @@ import (
 	"go/parser"
 	"go/scanner"
 	"io"
+	"io/ioutil"
 	"math"
 	"os"
 	"os/exec"
@ -123,7 +124,7 @@ Type "help" followed by the name of a command for more information about it.`},
 See $GOPATH/src/github.com/go-delve/delve/Documentation/cli/locspec.md for the syntax of linespec.

 See also: "help on", "help cond" and "help clear"`},
-		{aliases: []string{"trace", "t"}, group: breakCmds, cmdFn: tracepoint, helpMsg: `Set tracepoint.
+		{aliases: []string{"trace", "t"}, group: breakCmds, cmdFn: tracepoint, allowedPrefixes: onPrefix, helpMsg: `Set tracepoint.

 	trace [name] <linespec>

@ -425,10 +426,19 @@ If no argument is specified the function being executed in the selected stack fr
 	-l <locspec>		disassembles the specified function`},
 		{aliases: []string{"on"}, group: breakCmds, cmdFn: c.onCmd, helpMsg: `Executes a command when a breakpoint is hit.

-	on <breakpoint name or id> <command>.
+	on <breakpoint name or id> <command>
+	on <breakpoint name or id> -edit
+	

-Supported commands: print, stack and goroutine)`},
-		{aliases: []string{"condition", "cond"}, group: breakCmds, cmdFn: conditionCmd, helpMsg: `Set breakpoint condition.
+Supported commands: print, stack, goroutine, trace and cond. 
+To convert a breakpoint into a tracepoint use:
+	
+	on <breakpoint name or id> trace
+
+The command 'on <bp> cond <cond-arguments>' is equivalent to 'cond <bp> <cond-arguments>'.
+
+The command 'on x -edit' can be used to edit the list of commands executed when the breakpoint is hit.`},
+		{aliases: []string{"condition", "cond"}, group: breakCmds, cmdFn: conditionCmd, allowedPrefixes: onPrefix, helpMsg: `Set breakpoint condition.

 	condition <breakpoint name or id> <boolean expression>.
 	condition -hitcount <breakpoint name or id> <operator> <argument>
@ -1700,36 +1710,8 @@ func breakpoints(t *Term, ctx callContext, args string) error {
 	for _, bp := range breakPoints {
 		fmt.Printf("%s at %v (%d)\n", formatBreakpointName(bp, true), t.formatBreakpointLocation(bp), bp.TotalHitCount)

-		var attrs []string
-		if bp.Cond != "" {
-			attrs = append(attrs, fmt.Sprintf("\tcond %s", bp.Cond))
-		}
-		if bp.HitCond != "" {
-			attrs = append(attrs, fmt.Sprintf("\tcond -hitcount %s", bp.HitCond))
-		}
-		if bp.Stacktrace > 0 {
-			attrs = append(attrs, fmt.Sprintf("\tstack %d", bp.Stacktrace))
-		}
-		if bp.Goroutine {
-			attrs = append(attrs, "\tgoroutine")
-		}
-		if bp.LoadArgs != nil {
-			if *(bp.LoadArgs) == longLoadConfig {
-				attrs = append(attrs, "\targs -v")
-			} else {
-				attrs = append(attrs, "\targs")
-			}
-		}
-		if bp.LoadLocals != nil {
-			if *(bp.LoadLocals) == longLoadConfig {
-				attrs = append(attrs, "\tlocals -v")
-			} else {
-				attrs = append(attrs, "\tlocals")
-			}
-		}
-		for i := range bp.Variables {
-			attrs = append(attrs, fmt.Sprintf("\tprint %s", bp.Variables[i]))
-		}
+		attrs := formatBreakpointAttrs("\t", bp, false)
+
 		if len(attrs) > 0 {
 			fmt.Printf("%s\n", strings.Join(attrs, "\n"))
 		}
@ -1737,6 +1719,43 @@ func breakpoints(t *Term, ctx callContext, args string) error {
 	return nil
 }

+func formatBreakpointAttrs(prefix string, bp *api.Breakpoint, includeTrace bool) []string {
+	var attrs []string
+	if bp.Cond != "" {
+		attrs = append(attrs, fmt.Sprintf("%scond %s", prefix, bp.Cond))
+	}
+	if bp.HitCond != "" {
+		attrs = append(attrs, fmt.Sprintf("%scond -hitcount %s", prefix, bp.HitCond))
+	}
+	if bp.Stacktrace > 0 {
+		attrs = append(attrs, fmt.Sprintf("%sstack %d", prefix, bp.Stacktrace))
+	}
+	if bp.Goroutine {
+		attrs = append(attrs, fmt.Sprintf("%sgoroutine", prefix))
+	}
+	if bp.LoadArgs != nil {
+		if *(bp.LoadArgs) == longLoadConfig {
+			attrs = append(attrs, fmt.Sprintf("%sargs -v", prefix))
+		} else {
+			attrs = append(attrs, fmt.Sprintf("%sargs", prefix))
+		}
+	}
+	if bp.LoadLocals != nil {
+		if *(bp.LoadLocals) == longLoadConfig {
+			attrs = append(attrs, fmt.Sprintf("%slocals -v", prefix))
+		} else {
+			attrs = append(attrs, fmt.Sprintf("%slocals", prefix))
+		}
+	}
+	for i := range bp.Variables {
+		attrs = append(attrs, fmt.Sprintf("%sprint %s", prefix, bp.Variables[i]))
+	}
+	if includeTrace && bp.Tracepoint {
+		attrs = append(attrs, fmt.Sprintf("%strace", prefix))
+	}
+	return attrs
+}
+
 func setBreakpoint(t *Term, ctx callContext, tracepoint bool, argstr string) ([]*api.Breakpoint, error) {
 	args := split2PartsBySpace(argstr)

@ -1829,16 +1848,18 @@ func breakpoint(t *Term, ctx callContext, args string) error {
 }

 func tracepoint(t *Term, ctx callContext, args string) error {
+	if ctx.Prefix == onPrefix {
+		if args != "" {
+			return errors.New("too many arguments to trace")
+		}
+		ctx.Breakpoint.Tracepoint = true
+		return nil
+	}
 	_, err := setBreakpoint(t, ctx, true, args)
 	return err
 }

-func edit(t *Term, ctx callContext, args string) error {
-	file, lineno, _, err := getLocation(t, ctx, args, false)
-	if err != nil {
-		return err
-	}
-
+func runEditor(args ...string) error {
 	var editor string
 	if editor = os.Getenv("DELVE_EDITOR"); editor == "" {
 		if editor = os.Getenv("EDITOR"); editor == "" {
@ -1846,13 +1867,21 @@ func edit(t *Term, ctx callContext, args string) error {
 		}
 	}

-	cmd := exec.Command(editor, fmt.Sprintf("+%d", lineno), file)
+	cmd := exec.Command(editor, args...)
 	cmd.Stdin = os.Stdin
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	return cmd.Run()
 }

+func edit(t *Term, ctx callContext, args string) error {
+	file, lineno, _, err := getLocation(t, ctx, args, false)
+	if err != nil {
+		return err
+	}
+	return runEditor(fmt.Sprintf("+%d", lineno), file)
+}
+
 func watchpoint(t *Term, ctx callContext, args string) error {
 	v := strings.SplitN(args, " ", 2)
 	if len(v) != 2 {
@ -2766,13 +2795,69 @@ func (c *Commands) onCmd(t *Term, ctx callContext, argstr string) error {

 	ctx.Prefix = onPrefix
 	ctx.Breakpoint = bp
-	err = c.CallWithContext(args[1], t, ctx)
-	if err != nil {
-		return err
+
+	if args[1] == "-edit" {
+		f, err := ioutil.TempFile("", "dlv-on-cmd-")
+		if err != nil {
+			return err
+		}
+		defer func() {
+			_ = os.Remove(f.Name())
+		}()
+		attrs := formatBreakpointAttrs("", ctx.Breakpoint, true)
+		_, err = f.Write([]byte(strings.Join(attrs, "\n")))
+		if err != nil {
+			return err
+		}
+		err = f.Close()
+		if err != nil {
+			return err
+		}
+
+		err = runEditor(f.Name())
+		if err != nil {
+			return err
+		}
+
+		fin, err := os.Open(f.Name())
+		if err != nil {
+			return err
+		}
+		defer fin.Close()
+
+		err = c.parseBreakpointAttrs(t, ctx, fin)
+		if err != nil {
+			return err
+		}
+	} else {
+		err = c.CallWithContext(args[1], t, ctx)
+		if err != nil {
+			return err
+		}
 	}
 	return t.client.AmendBreakpoint(ctx.Breakpoint)
 }

+func (c *Commands) parseBreakpointAttrs(t *Term, ctx callContext, r io.Reader) error {
+	ctx.Breakpoint.Tracepoint = false
+	ctx.Breakpoint.Goroutine = false
+	ctx.Breakpoint.Stacktrace = 0
+	ctx.Breakpoint.Variables = ctx.Breakpoint.Variables[:0]
+	ctx.Breakpoint.Cond = ""
+	ctx.Breakpoint.HitCond = ""
+
+	scan := bufio.NewScanner(r)
+	lineno := 0
+	for scan.Scan() {
+		lineno++
+		err := c.CallWithContext(scan.Text(), t, ctx)
+		if err != nil {
+			fmt.Printf("%d: %s\n", lineno, err.Error())
+		}
+	}
+	return scan.Err()
+}
+
 func conditionCmd(t *Term, ctx callContext, argstr string) error {
 	args := split2PartsBySpace(argstr)

@ -2782,10 +2867,17 @@ func conditionCmd(t *Term, ctx callContext, argstr string) error {

 	if args[0] == "-hitcount" {
 		// hitcount breakpoint
+
+		if ctx.Prefix == onPrefix {
+			ctx.Breakpoint.HitCond = args[1]
+			return nil
+		}
+
 		args = split2PartsBySpace(args[1])
 		if len(args) < 2 {
 			return fmt.Errorf("not enough arguments")
 		}
+
 		bp, err := getBreakpointByIDOrName(t, args[0])
 		if err != nil {
 			return err
@ -2796,6 +2888,11 @@ func conditionCmd(t *Term, ctx callContext, argstr string) error {
 		return t.client.AmendBreakpoint(bp)
 	}

+	if ctx.Prefix == onPrefix {
+		ctx.Breakpoint.Cond = argstr
+		return nil
+	}
+
 	bp, err := getBreakpointByIDOrName(t, args[0])
 	if err != nil {
 		return err
--- a/pkg/terminal/command_test.go
+++ b/pkg/terminal/command_test.go
@ -8,6 +8,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"reflect"
 	"regexp"
 	"runtime"
 	"strconv"
@ -1192,3 +1193,90 @@ func TestHitCondBreakpoint(t *testing.T) {
 		}
 	})
 }
+
+func TestBreakpointEditing(t *testing.T) {
+	term := &FakeTerminal{
+		t:    t,
+		Term: New(nil, &config.Config{}),
+	}
+	_ = term
+
+	var testCases = []struct {
+		inBp    *api.Breakpoint
+		inBpStr string
+		edit    string
+		outBp   *api.Breakpoint
+	}{
+		{ // tracepoint -> breakpoint
+			&api.Breakpoint{Tracepoint: true},
+			"trace",
+			"",
+			&api.Breakpoint{}},
+		{ // breakpoint -> tracepoint
+			&api.Breakpoint{Variables: []string{"a"}},
+			"print a",
+			"print a\ntrace",
+			&api.Breakpoint{Tracepoint: true, Variables: []string{"a"}}},
+		{ // add print var
+			&api.Breakpoint{Variables: []string{"a"}},
+			"print a",
+			"print b\nprint a\n",
+			&api.Breakpoint{Variables: []string{"b", "a"}}},
+		{ // add goroutine flag
+			&api.Breakpoint{},
+			"",
+			"goroutine",
+			&api.Breakpoint{Goroutine: true}},
+		{ // remove goroutine flag
+			&api.Breakpoint{Goroutine: true},
+			"goroutine",
+			"",
+			&api.Breakpoint{}},
+		{ // add stack directive
+			&api.Breakpoint{},
+			"",
+			"stack 10",
+			&api.Breakpoint{Stacktrace: 10}},
+		{ // remove stack directive
+			&api.Breakpoint{Stacktrace: 20},
+			"stack 20",
+			"print a",
+			&api.Breakpoint{Variables: []string{"a"}}},
+		{ // add condition
+			&api.Breakpoint{Variables: []string{"a"}},
+			"print a",
+			"print a\ncond a < b",
+			&api.Breakpoint{Variables: []string{"a"}, Cond: "a < b"}},
+		{ // remove condition
+			&api.Breakpoint{Cond: "a < b"},
+			"cond a < b",
+			"",
+			&api.Breakpoint{}},
+		{ // change condition
+			&api.Breakpoint{Cond: "a < b"},
+			"cond a < b",
+			"cond a < 5",
+			&api.Breakpoint{Cond: "a < 5"}},
+		{ // change hitcount condition
+			&api.Breakpoint{HitCond: "% 2"},
+			"cond -hitcount % 2",
+			"cond -hitcount = 2",
+			&api.Breakpoint{HitCond: "= 2"}},
+	}
+
+	for _, tc := range testCases {
+		bp := *tc.inBp
+		bpStr := strings.Join(formatBreakpointAttrs("", &bp, true), "\n")
+		if bpStr != tc.inBpStr {
+			t.Errorf("Expected %q got %q for:\n%#v", tc.inBpStr, bpStr, tc.inBp)
+		}
+		ctx := callContext{Prefix: onPrefix, Scope: api.EvalScope{GoroutineID: -1, Frame: 0, DeferredCall: 0}, Breakpoint: &bp}
+		err := term.cmds.parseBreakpointAttrs(nil, ctx, strings.NewReader(tc.edit))
+		if err != nil {
+			t.Errorf("Unexpected error during edit %q", tc.edit)
+		}
+		if !reflect.DeepEqual(bp, *tc.outBp) {
+			t.Errorf("mismatch after edit\nexpected: %#v\ngot: %#v", tc.outBp, bp)
+		}
+	}
+}