service: fix breakpoint IDs after Restart with disabled breakpoints (#2425)

When restarting we must take care of setting breakpoint IDs correctly
so that enabled breakpoints do not end up having the same ID as a
disabled breakpoint, also we must make sure that breakpoints created
after restart will not get an ID already used by a disabled breakpoint.

pkg/proc/target.go

@@ -359,3 +359,8 @@ func (t *Target) createFatalThrowBreakpoint() {
 func (t *Target) CurrentThread() Thread {
 	return t.currentThread
 }
+
+// SetNextBreakpointID sets the breakpoint ID of the next breakpoint
+func (t *Target) SetNextBreakpointID(id int) {
+	t.Breakpoints().breakpointIDCounter = id
+}

service/debugger/debugger.go

@@ -509,23 +509,27 @@ func (d *Debugger) Restart(rerecord bool, pos string, resetArgs bool, newArgs []
 	discarded := []api.DiscardedBreakpoint{}
 	breakpoints := api.ConvertBreakpoints(d.breakpoints())
 	d.target = p
+	maxID := 0
 	for _, oldBp := range breakpoints {
 		if oldBp.ID < 0 {
 			continue
 		}
+		if oldBp.ID > maxID {
+			maxID = oldBp.ID
+		}
 		if len(oldBp.File) > 0 {
 			addrs, err := proc.FindFileLocation(p, oldBp.File, oldBp.Line)
 			if err != nil {
 				discarded = append(discarded, api.DiscardedBreakpoint{Breakpoint: oldBp, Reason: err.Error()})
 				continue
 			}
-			createLogicalBreakpoint(d, addrs, oldBp)
+			createLogicalBreakpoint(d, addrs, oldBp, oldBp.ID)
 		} else {
 			// Avoid setting a breakpoint based on address when rebuilding
 			if rebuild {
 				continue
 			}
-			newBp, err := p.SetBreakpoint(oldBp.Addr, proc.UserBreakpoint, nil)
+			newBp, err := p.SetBreakpointWithID(oldBp.ID, oldBp.Addr)
 			if err != nil {
 				return nil, err
 			}
@@ -534,6 +538,12 @@ func (d *Debugger) Restart(rerecord bool, pos string, resetArgs bool, newArgs []
 			}
 		}
 	}
+	for _, bp := range d.disabledBreakpoints {
+		if bp.ID > maxID {
+			maxID = bp.ID
+		}
+	}
+	d.target.SetNextBreakpointID(maxID)
 	return discarded, nil
 }
 
@@ -652,7 +662,7 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoin
 		return nil, err
 	}
 
-	createdBp, err := createLogicalBreakpoint(d, addrs, requestedBp)
+	createdBp, err := createLogicalBreakpoint(d, addrs, requestedBp, 0)
 	if err != nil {
 		return nil, err
 	}
@@ -662,7 +672,7 @@ 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(d *Debugger, addrs []uint64, requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
+func createLogicalBreakpoint(d *Debugger, addrs []uint64, requestedBp *api.Breakpoint, id int) (*api.Breakpoint, error) {
 	p := d.target
 
 	if dbp, ok := d.disabledBreakpoints[requestedBp.ID]; ok {
@@ -672,7 +682,11 @@ func createLogicalBreakpoint(d *Debugger, addrs []uint64, requestedBp *api.Break
 	bps := make([]*proc.Breakpoint, len(addrs))
 	var err error
 	for i := range addrs {
+		if id > 0 {
+			bps[i], err = p.SetBreakpointWithID(id, addrs[i])
+		} else {
 			bps[i], err = p.SetBreakpoint(addrs[i], proc.UserBreakpoint, nil)
+		}
 		if err != nil {
 			break
 		}
@@ -871,7 +885,7 @@ func (d *Debugger) FindBreakpoint(id int) *api.Breakpoint {
 func (d *Debugger) findBreakpoint(id int) []*proc.Breakpoint {
 	var bps []*proc.Breakpoint
 	for _, bp := range d.target.Breakpoints().M {
-		if bp.LogicalID == id {
+		if bp.IsUser() && bp.LogicalID == id {
 			bps = append(bps, bp)
 		}
 	}

service/test/integration2_test.go

@@ -2277,3 +2277,34 @@ func TestDetachLeaveRunning(t *testing.T) {
 	defer server.Stop()
 	assertNoError(client.Detach(false), t, "Detach")
 }
+
+func assertNoDuplicateBreakpoints(t *testing.T, c service.Client) {
+	t.Helper()
+	bps, _ := c.ListBreakpoints()
+	seen := make(map[int]bool)
+	for _, bp := range bps {
+		t.Logf("%#v\n", bp)
+		if seen[bp.ID] {
+			t.Fatalf("duplicate breakpoint ID %d", bp.ID)
+		}
+		seen[bp.ID] = true
+	}
+}
+
+func TestToggleBreakpointRestart(t *testing.T) {
+	// Checks that breakpoints IDs do not overlap after Restart if there are disabled breakpoints.
+	withTestClient2("testtoggle", t, func(c service.Client) {
+		bp1, err := c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.main", Line: 1, Name: "firstbreakpoint"})
+		assertNoError(err, t, "CreateBreakpoint 1")
+		_, err = c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.main", Line: 2, Name: "secondbreakpoint"})
+		assertNoError(err, t, "CreateBreakpoint 2")
+		_, err = c.ToggleBreakpoint(bp1.ID)
+		assertNoError(err, t, "ToggleBreakpoint")
+		_, err = c.Restart(false)
+		assertNoError(err, t, "Restart")
+		assertNoDuplicateBreakpoints(t, c)
+		_, err = c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.main", Line: 3, Name: "thirdbreakpoint"})
+		assertNoError(err, t, "CreateBreakpoint 3")
+		assertNoDuplicateBreakpoints(t, c)
+	})
+}