*: Adds toggle command (#2208)

* Adds toggle command

Also adds two rpc2 tests for testing the new functionality

* Removes Debuggers' ToggleBreakpoint method

rpc2's ToggleBreakpoint now calls AmendBreakpoint
Refactors the ClearBreakpoint to avoid a lock.

Documentation/cli/README.md

@@ -32,6 +32,7 @@ Command | Description
 [clearall](#clearall) | Deletes multiple breakpoints.
 [condition](#condition) | Set breakpoint condition.
 [on](#on) | Executes a command when a breakpoint is hit.
+[toggle](#toggle) | Toggles on or off a breakpoint.
 [trace](#trace) | Set tracepoint.
 
 
@@ -531,6 +532,12 @@ Aliases: tr
 Print out info for every traced thread.
 
 
+## toggle
+Toggles on or off a breakpoint.
+
+toggle <breakpoint name or id>
+
+
 ## trace
 Set tracepoint.
 

Documentation/cli/starlark.md

@@ -58,6 +58,7 @@ restart(Position, ResetArgs, NewArgs, Rerecord, Rebuild, NewRedirects) | Equival
 set_expr(Scope, Symbol, Value) | Equivalent to API call [Set](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.Set)
 stacktrace(Id, Depth, Full, Defers, Opts, Cfg) | Equivalent to API call [Stacktrace](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.Stacktrace)
 state(NonBlocking) | Equivalent to API call [State](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.State)
+toggle_breakpoint(Id, Name) | Equivalent to API call [ToggleBreakpoint](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.ToggleBreakpoint)
 dlv_command(command) | Executes the specified command as if typed at the dlv_prompt
 read_file(path) | Reads the file as a string
 write_file(path, contents) | Writes string to a file

_fixtures/testtoggle.go

+package main
+
+import (
+	"fmt"
+)
+
+func lineOne() {
+	fmt.Println("lineOne function")
+}
+
+func lineTwo() {
+	fmt.Println("lineTwo function")
+}
+
+func lineThree() {
+	fmt.Println("lineThree function")
+}
+
+func main() {
+	lineOne()
+	lineTwo()
+	lineThree()
+}

pkg/proc/breakpoints.go

@@ -299,8 +299,8 @@ func (t *Target) SetBreakpoint(addr uint64, kind BreakpointKind, cond ast.Expr)
 	return newBreakpoint, nil
 }
 
-// setBreakpointWithID creates a breakpoint at addr, with the specified logical ID.
-func (t *Target) setBreakpointWithID(id int, addr uint64) (*Breakpoint, error) {
+// SetBreakpointWithID creates a breakpoint at addr, with the specified logical ID.
+func (t *Target) SetBreakpointWithID(id int, addr uint64) (*Breakpoint, error) {
 	bpmap := t.Breakpoints()
 	bp, err := t.SetBreakpoint(addr, UserBreakpoint, nil)
 	if err == nil {

pkg/proc/target.go

@@ -334,7 +334,7 @@ func (t *Target) createUnrecoveredPanicBreakpoint() {
 		panicpcs, err = FindFunctionLocation(t.Process, "runtime.fatalpanic", 0)
 	}
 	if err == nil {
-		bp, err := t.setBreakpointWithID(unrecoveredPanicID, panicpcs[0])
+		bp, err := t.SetBreakpointWithID(unrecoveredPanicID, panicpcs[0])
 		if err == nil {
 			bp.Name = UnrecoveredPanic
 			bp.Variables = []string{"runtime.curg._panic.arg"}
@@ -346,7 +346,7 @@ func (t *Target) createUnrecoveredPanicBreakpoint() {
 func (t *Target) createFatalThrowBreakpoint() {
 	fatalpcs, err := FindFunctionLocation(t.Process, "runtime.fatalthrow", 0)
 	if err == nil {
-		bp, err := t.setBreakpointWithID(fatalThrowID, fatalpcs[0])
+		bp, err := t.SetBreakpointWithID(fatalThrowID, fatalpcs[0])
 		if err == nil {
 			bp.Name = FatalThrow
 		}

pkg/terminal/command.go

@@ -200,6 +200,9 @@ Current limitations:
 	clearall [<linespec>]
 
 If called with the linespec argument it will delete all the breakpoints matching the linespec. If linespec is omitted all breakpoints are deleted.`},
+		{aliases: []string{"toggle"}, group: breakCmds, cmdFn: toggle, helpMsg: `Toggles on or off a breakpoint.
+
+toggle <breakpoint name or id>`},
 		{aliases: []string{"goroutines", "grs"}, group: goroutineCmds, cmdFn: goroutines, helpMsg: `List program goroutines.
 
 	goroutines [-u (default: user location)|-r (runtime location)|-g (go statement location)|-s (start location)] [-t (stack trace)] [-l (labels)]
@@ -1470,6 +1473,24 @@ func clearAll(t *Term, ctx callContext, args string) error {
 	return nil
 }
 
+func toggle(t *Term, ctx callContext, args string) error {
+	if args == "" {
+		return fmt.Errorf("not enough arguments")
+	}
+	id, err := strconv.Atoi(args)
+	var bp *api.Breakpoint
+	if err == nil {
+		bp, err = t.client.ToggleBreakpoint(id)
+	} else {
+		bp, err = t.client.ToggleBreakpointByName(args)
+	}
+	if err != nil {
+		return err
+	}
+	fmt.Printf("%s toggled at %s\n", formatBreakpointName(bp, true), t.formatBreakpointLocation(bp))
+	return nil
+}
+
 // byID sorts breakpoints by ID.
 type byID []*api.Breakpoint
 
@@ -2708,7 +2729,11 @@ func formatBreakpointName(bp *api.Breakpoint, upcase bool) string {
 	if id == "" {
 		id = strconv.Itoa(bp.ID)
 	}
-	return fmt.Sprintf("%s %s", thing, id)
+	state := "(enabled)"
+	if bp.Disabled {
+		state = "(disabled)"
+	}
+	return fmt.Sprintf("%s %s %s", thing, id, state)
 }
 
 func (t *Term) formatBreakpointLocation(bp *api.Breakpoint) string {

pkg/terminal/starbind/starlark_mapping.go

@@ -1359,5 +1359,43 @@ func (env *Env) starlarkPredeclare() starlark.StringDict {
 		}
 		return env.interfaceToStarlarkValue(rpcRet), nil
 	})
+	r["toggle_breakpoint"] = starlark.NewBuiltin("toggle_breakpoint", func(thread *starlark.Thread, _ *starlark.Builtin, args starlark.Tuple, kwargs []starlark.Tuple) (starlark.Value, error) {
+		if err := isCancelled(thread); err != nil {
+			return starlark.None, decorateError(thread, err)
+		}
+		var rpcArgs rpc2.ToggleBreakpointIn
+		var rpcRet rpc2.ToggleBreakpointOut
+		if len(args) > 0 && args[0] != starlark.None {
+			err := unmarshalStarlarkValue(args[0], &rpcArgs.Id, "Id")
+			if err != nil {
+				return starlark.None, decorateError(thread, err)
+			}
+		}
+		if len(args) > 1 && args[1] != starlark.None {
+			err := unmarshalStarlarkValue(args[1], &rpcArgs.Name, "Name")
+			if err != nil {
+				return starlark.None, decorateError(thread, err)
+			}
+		}
+		for _, kv := range kwargs {
+			var err error
+			switch kv[0].(starlark.String) {
+			case "Id":
+				err = unmarshalStarlarkValue(kv[1], &rpcArgs.Id, "Id")
+			case "Name":
+				err = unmarshalStarlarkValue(kv[1], &rpcArgs.Name, "Name")
+			default:
+				err = fmt.Errorf("unknown argument %q", kv[0])
+			}
+			if err != nil {
+				return starlark.None, decorateError(thread, err)
+			}
+		}
+		err := env.ctx.Client().CallAPI("ToggleBreakpoint", &rpcArgs, &rpcRet)
+		if err != nil {
+			return starlark.None, err
+		}
+		return env.interfaceToStarlarkValue(rpcRet), nil
+	})
 	return r
 }

service/api/types.go

@@ -87,6 +87,8 @@ type Breakpoint struct {
 	HitCount map[string]uint64 `json:"hitCount"`
 	// number of times a breakpoint has been reached
 	TotalHitCount uint64 `json:"totalHitCount"`
+	// Disabled flag, signifying the state of the breakpoint
+	Disabled bool `json:"disabled"`
 }
 
 // ValidBreakpointName returns an error if

service/client.go

@@ -72,6 +72,10 @@ type Client interface {
 	ClearBreakpoint(id int) (*api.Breakpoint, error)
 	// ClearBreakpointByName deletes a breakpoint by name
 	ClearBreakpointByName(name string) (*api.Breakpoint, error)
+	// ToggleBreakpoint toggles on or off a breakpoint by ID.
+	ToggleBreakpoint(id int) (*api.Breakpoint, error)
+	// ToggleBreakpointByName toggles on or off a breakpoint by name.
+	ToggleBreakpointByName(name string) (*api.Breakpoint, error)
 	// Allows user to update an existing breakpoint for example to change the information
 	// retrieved when the breakpoint is hit or to change, add or remove the break condition
 	AmendBreakpoint(*api.Breakpoint) error

service/debugger/debugger.go

@@ -70,6 +70,10 @@ type Debugger struct {
 	recordMutex   sync.Mutex
 
 	dumpState proc.DumpState
+	// Debugger keeps a map of disabled breakpoints
+	// so lower layers like proc doesn't need to deal
+	// with them
+	disabledBreakpoints map[int]*api.Breakpoint
 }
 
 type ExecuteKind int
@@ -198,6 +202,9 @@ func New(config *Config, processArgs []string) (*Debugger, error) {
 			return nil, err
 		}
 	}
+
+	d.disabledBreakpoints = make(map[int]*api.Breakpoint)
+
 	return d, nil
 }
 
@@ -502,7 +509,9 @@ func (d *Debugger) Restart(rerecord bool, pos string, resetArgs bool, newArgs []
 	}
 
 	discarded := []api.DiscardedBreakpoint{}
-	for _, oldBp := range api.ConvertBreakpoints(d.breakpoints()) {
+	breakpoints := api.ConvertBreakpoints(d.breakpoints())
+	d.target = p
+	for _, oldBp := range breakpoints {
 		if oldBp.ID < 0 {
 			continue
 		}
@@ -512,7 +521,7 @@ func (d *Debugger) Restart(rerecord bool, pos string, resetArgs bool, newArgs []
 				discarded = append(discarded, api.DiscardedBreakpoint{Breakpoint: oldBp, Reason: err.Error()})
 				continue
 			}
-			createLogicalBreakpoint(p, addrs, oldBp)
+			createLogicalBreakpoint(d, addrs, oldBp)
 		} else {
 			// Avoid setting a breakpoint based on address when rebuilding
 			if rebuild {
@@ -527,7 +536,6 @@ func (d *Debugger) Restart(rerecord bool, pos string, resetArgs bool, newArgs []
 			}
 		}
 	}
-	d.target = p
 	return discarded, nil
 }
 
@@ -613,7 +621,7 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoin
 		if err = api.ValidBreakpointName(requestedBp.Name); err != nil {
 			return nil, err
 		}
-		if d.findBreakpointByName(requestedBp.Name) != nil {
+		if (d.findBreakpointByName(requestedBp.Name) != nil) || (d.findDisabledBreakpointByName(requestedBp.Name) != nil) {
 			return nil, errors.New("breakpoint name already exists")
 		}
 	}
@@ -646,7 +654,7 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoin
 		return nil, err
 	}
 
-	createdBp, err := createLogicalBreakpoint(d.target, addrs, requestedBp)
+	createdBp, err := createLogicalBreakpoint(d, addrs, requestedBp)
 	if err != nil {
 		return nil, err
 	}
@@ -656,7 +664,13 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoin
 
 // createLogicalBreakpoint creates one physical breakpoint for each address
 // in addrs and associates all of them with the same logical breakpoint.
-func createLogicalBreakpoint(p *proc.Target, addrs []uint64, requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
+func createLogicalBreakpoint(d *Debugger, addrs []uint64, requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
+	p := d.target
+
+	if dbp, ok := d.disabledBreakpoints[requestedBp.ID]; ok {
+		return dbp, proc.BreakpointExistsError{File: dbp.File, Line: dbp.Line, Addr: dbp.Addr}
+	}
+
 	bps := make([]*proc.Breakpoint, len(addrs))
 	var err error
 	for i := range addrs {
@@ -687,6 +701,7 @@ func createLogicalBreakpoint(p *proc.Target, addrs []uint64, requestedBp *api.Br
 		}
 		return nil, err
 	}
+
 	createdBp := api.ConvertBreakpoints(bps)
 	return createdBp[0], nil // we created a single logical breakpoint, the slice here will always have len == 1
 }
@@ -697,22 +712,39 @@ func isBreakpointExistsErr(err error) bool {
 }
 
 // AmendBreakpoint will update the breakpoint with the matching ID.
+// It also enables or disables the breakpoint.
 func (d *Debugger) AmendBreakpoint(amend *api.Breakpoint) error {
 	d.targetMutex.Lock()
 	defer d.targetMutex.Unlock()
 
 	originals := d.findBreakpoint(amend.ID)
-	if originals == nil {
+	_, disabled := d.disabledBreakpoints[amend.ID]
+	if originals == nil && !disabled {
 		return fmt.Errorf("no breakpoint with ID %d", amend.ID)
 	}
 	if err := api.ValidBreakpointName(amend.Name); err != nil {
 		return err
 	}
+	if !amend.Disabled && disabled { // enable the breakpoint
+		bp, err := d.target.SetBreakpointWithID(amend.ID, amend.Addr)
+		if err != nil {
+			return err
+		}
+		copyBreakpointInfo(bp, amend)
+		delete(d.disabledBreakpoints, amend.ID)
+	}
+	if amend.Disabled && !disabled { // disable the breakpoint
+		if _, err := d.clearBreakpoint(amend); err != nil {
+			return err
+		}
+		d.disabledBreakpoints[amend.ID] = amend
+	}
 	for _, original := range originals {
 		if err := copyBreakpointInfo(original, amend); err != nil {
 			return err
 		}
 	}
+
 	return nil
 }
 
@@ -744,6 +776,15 @@ func copyBreakpointInfo(bp *proc.Breakpoint, requested *api.Breakpoint) (err err
 func (d *Debugger) ClearBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
 	d.targetMutex.Lock()
 	defer d.targetMutex.Unlock()
+	return d.clearBreakpoint(requestedBp)
+}
+
+// clearBreakpoint clears a breakpoint, we can consume this function to avoid locking a goroutine
+func (d *Debugger) clearBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
+	if bp, ok := d.disabledBreakpoints[requestedBp.ID]; ok {
+		delete(d.disabledBreakpoints, bp.ID)
+		return bp, nil
+	}
 
 	var bps []*proc.Breakpoint
 	var errs []error
@@ -796,7 +837,14 @@ func (d *Debugger) ClearBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint
 func (d *Debugger) Breakpoints() []*api.Breakpoint {
 	d.targetMutex.Lock()
 	defer d.targetMutex.Unlock()
-	return api.ConvertBreakpoints(d.breakpoints())
+
+	bps := api.ConvertBreakpoints(d.breakpoints())
+
+	for _, bp := range d.disabledBreakpoints {
+		bps = append(bps, bp)
+	}
+
+	return bps
 }
 
 func (d *Debugger) breakpoints() []*proc.Breakpoint {
@@ -815,6 +863,7 @@ func (d *Debugger) FindBreakpoint(id int) *api.Breakpoint {
 	d.targetMutex.Lock()
 	defer d.targetMutex.Unlock()
 	bps := api.ConvertBreakpoints(d.findBreakpoint(id))
+	bps = append(bps, d.findDisabledBreakpoint(id)...)
 	if len(bps) <= 0 {
 		return nil
 	}
@@ -831,11 +880,26 @@ func (d *Debugger) findBreakpoint(id int) []*proc.Breakpoint {
 	return bps
 }
 
+func (d *Debugger) findDisabledBreakpoint(id int) []*api.Breakpoint {
+	var bps []*api.Breakpoint
+	for _, dbp := range d.disabledBreakpoints {
+		if dbp.ID == id {
+			bps = append(bps, dbp)
+		}
+	}
+	return bps
+}
+
 // FindBreakpointByName returns the breakpoint specified by 'name'
 func (d *Debugger) FindBreakpointByName(name string) *api.Breakpoint {
 	d.targetMutex.Lock()
 	defer d.targetMutex.Unlock()
-	return d.findBreakpointByName(name)
+
+	bp := d.findBreakpointByName(name)
+	if bp == nil {
+		bp = d.findDisabledBreakpointByName(name)
+	}
+	return bp
 }
 
 func (d *Debugger) findBreakpointByName(name string) *api.Breakpoint {
@@ -853,6 +917,15 @@ func (d *Debugger) findBreakpointByName(name string) *api.Breakpoint {
 	return r[0] // there can only be one logical breakpoint with the same name
 }
 
+func (d *Debugger) findDisabledBreakpointByName(name string) *api.Breakpoint {
+	for _, dbp := range d.disabledBreakpoints {
+		if dbp.Name == name {
+			return dbp
+		}
+	}
+	return nil
+}
+
 // Threads returns the threads of the target process.
 func (d *Debugger) Threads() ([]proc.Thread, error) {
 	d.targetMutex.Lock()

service/rpc2/client.go

@@ -250,6 +250,18 @@ func (c *RPCClient) ClearBreakpointByName(name string) (*api.Breakpoint, error)
 	return out.Breakpoint, err
 }
 
+func (c *RPCClient) ToggleBreakpoint(id int) (*api.Breakpoint, error) {
+	var out ToggleBreakpointOut
+	err := c.call("ToggleBreakpoint", ToggleBreakpointIn{id, ""}, &out)
+	return out.Breakpoint, err
+}
+
+func (c *RPCClient) ToggleBreakpointByName(name string) (*api.Breakpoint, error) {
+	var out ToggleBreakpointOut
+	err := c.call("ToggleBreakpoint", ToggleBreakpointIn{0, name}, &out)
+	return out.Breakpoint, err
+}
+
 func (c *RPCClient) AmendBreakpoint(bp *api.Breakpoint) error {
 	out := new(AmendBreakpointOut)
 	err := c.call("AmendBreakpoint", AmendBreakpointIn{*bp}, out)

service/rpc2/server.go

@@ -294,6 +294,38 @@ func (s *RPCServer) ClearBreakpoint(arg ClearBreakpointIn, out *ClearBreakpointO
 	return nil
 }
 
+type ToggleBreakpointIn struct {
+	Id   int
+	Name string
+}
+
+type ToggleBreakpointOut struct {
+	Breakpoint *api.Breakpoint
+}
+
+// ToggleBreakpoint toggles on or off a breakpoint by Name (if Name is not an
+// empty string) or by ID.
+func (s *RPCServer) ToggleBreakpoint(arg ToggleBreakpointIn, out *ToggleBreakpointOut) error {
+	var bp *api.Breakpoint
+	if arg.Name != "" {
+		bp = s.debugger.FindBreakpointByName(arg.Name)
+		if bp == nil {
+			return fmt.Errorf("no breakpoint with name %s", arg.Name)
+		}
+	} else {
+		bp = s.debugger.FindBreakpoint(arg.Id)
+		if bp == nil {
+			return fmt.Errorf("no breakpoint with id %d", arg.Id)
+		}
+	}
+	bp.Disabled = !bp.Disabled
+	if err := s.debugger.AmendBreakpoint(bp); err != nil {
+		return err
+	}
+	out.Breakpoint = bp
+	return nil
+}
+
 type AmendBreakpointIn struct {
 	Breakpoint api.Breakpoint
 }

service/test/integration2_test.go

@@ -507,6 +507,113 @@ func TestClientServer_clearBreakpoint(t *testing.T) {
 	})
 }
 
+func TestClientServer_toggleBreakpoint(t *testing.T) {
+	withTestClient2("testtoggle", t, func(c service.Client) {
+		toggle := func(bp *api.Breakpoint) {
+			dbp, err := c.ToggleBreakpoint(bp.ID)
+			if err != nil {
+				t.Fatalf("Unexpected error: %v", err)
+			}
+			if dbp.ID != bp.ID {
+				t.Fatalf("The IDs don't match")
+			}
+		}
+
+		// This one is toggled twice
+		bp1, err := c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.lineOne", Tracepoint: true})
+		if err != nil {
+			t.Fatalf("Unexpected error: %v\n", err)
+		}
+
+		toggle(bp1)
+		toggle(bp1)
+
+		// This one is toggled once
+		bp2, err := c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.lineTwo", Tracepoint: true})
+		if err != nil {
+			t.Fatalf("Unexpected error: %v\n", err)
+		}
+
+		toggle(bp2)
+
+		// This one is never toggled
+		bp3, err := c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.lineThree", Tracepoint: true})
+		if err != nil {
+			t.Fatalf("Unexpected error: %v\n", err)
+		}
+
+		if e, a := 3, countBreakpoints(t, c); e != a {
+			t.Fatalf("Expected breakpoint count %d, got %d", e, a)
+		}
+
+		enableCount := 0
+		disabledCount := 0
+
+		contChan := c.Continue()
+		for state := range contChan {
+			if state.CurrentThread != nil && state.CurrentThread.Breakpoint != nil {
+				switch state.CurrentThread.Breakpoint.ID {
+				case bp1.ID, bp3.ID:
+					enableCount++
+				case bp2.ID:
+					disabledCount++
+				}
+
+				t.Logf("%v", state)
+			}
+			if state.Exited {
+				continue
+			}
+			if state.Err != nil {
+				t.Fatalf("Unexpected error during continue: %v\n", state.Err)
+			}
+		}
+
+		if enableCount != 2 {
+			t.Fatalf("Wrong number of enabled hits: %d\n", enableCount)
+		}
+
+		if disabledCount != 0 {
+			t.Fatalf("A disabled breakpoint was hit: %d\n", disabledCount)
+		}
+	})
+}
+
+func TestClientServer_toggleAmendedBreakpoint(t *testing.T) {
+	withTestClient2("testtoggle", t, func(c service.Client) {
+		toggle := func(bp *api.Breakpoint) {
+			dbp, err := c.ToggleBreakpoint(bp.ID)
+			if err != nil {
+				t.Fatalf("Unexpected error: %v", err)
+			}
+			if dbp.ID != bp.ID {
+				t.Fatalf("The IDs don't match")
+			}
+		}
+
+		// This one is toggled twice
+		bp, err := c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.lineOne", Tracepoint: true})
+		if err != nil {
+			t.Fatalf("Unexpected error: %v\n", err)
+		}
+		bp.Cond = "n == 7"
+		assertNoError(c.AmendBreakpoint(bp), t, "AmendBreakpoint() 1")
+
+		// Toggle off.
+		toggle(bp)
+		// Toggle on.
+		toggle(bp)
+
+		amended, err := c.GetBreakpoint(bp.ID)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if amended.Cond == "" {
+			t.Fatal("breakpoint amendedments not preserved after toggle")
+		}
+	})
+}
+
 func TestClientServer_switchThread(t *testing.T) {
 	protest.AllowRecording(t)
 	withTestClient2("testnextprog", t, func(c service.Client) {