proc: do not assign temporary breakpoint IDs (#2650)

Internal breakpoints do not need IDs and assigning them from a counter
separate from the user ID counter can be a cause of confusion.
If a user breakpoint is overlayed on top of a pre-existing internal
breakpoint the temporary ID will be surfaced as if it was a user ID,
possibly conflicting with another user ID.
If a temporary breakpoint is overlayed on top of a pre-existing user
breakpoint and the user breakpoint is first deleted and then
re-created, the user ID will be resurrected along with the breakpoint,
instead of allocating a fresh one.

This change removes internal breakpoint IDs entirely, only user
breakpoints receive an ID.

pkg/proc/breakpoints.go

@@ -27,6 +27,8 @@ const (
 
 	unrecoveredPanicID = -1
 	fatalThrowID       = -2
+
+	NoLogicalID = -1000 // Logical breakpoint ID for breakpoints internal breakpoints.
 )
 
 // Breakpoint represents a physical breakpoint. Stores information on the break
@@ -41,7 +43,6 @@ type Breakpoint struct {
 	Addr         uint64 // Address breakpoint is set for.
 	OriginalData []byte // If software breakpoint, the data we replace with breakpoint instruction.
 	Name         string // User defined name of the breakpoint
-	LogicalID    int    // ID of the logical breakpoint that owns this physical breakpoint
 
 	WatchExpr     string
 	WatchType     WatchType
@@ -74,6 +75,8 @@ type Breaklet struct {
 	// stepping).
 	Kind BreakpointKind
 
+	LogicalID int // ID of the logical breakpoint that owns this physical breakpoint
+
 	// Cond: if not nil the breakpoint will be triggered only if evaluating Cond returns true
 	Cond ast.Expr
 
@@ -175,7 +178,16 @@ func (wtype WatchType) withSize(sz uint8) WatchType {
 var ErrHWBreakUnsupported = errors.New("hardware breakpoints not implemented")
 
 func (bp *Breakpoint) String() string {
-	return fmt.Sprintf("Breakpoint %d at %#v %s:%d", bp.LogicalID, bp.Addr, bp.File, bp.Line)
+	return fmt.Sprintf("Breakpoint %d at %#v %s:%d", bp.LogicalID(), bp.Addr, bp.File, bp.Line)
+}
+
+func (bp *Breakpoint) LogicalID() int {
+	for _, breaklet := range bp.Breaklets {
+		if breaklet.Kind == UserBreakpoint {
+			return breaklet.LogicalID
+		}
+	}
+	return NoLogicalID
 }
 
 // VerboseDescr returns a string describing parts of the breakpoint struct
@@ -449,7 +461,6 @@ type BreakpointMap struct {
 	WatchOutOfScope []*Breakpoint
 
 	breakpointIDCounter int
-	internalBreakpointIDCounter int
 }
 
 // NewBreakpointMap creates a new BreakpointMap.
@@ -610,11 +621,22 @@ func (t *Target) setBreakpointInternal(addr uint64, kind BreakpointKind, wtype W
 	newBreaklet := &Breaklet{Kind: kind, Cond: cond}
 	if kind == UserBreakpoint {
 		newBreaklet.HitCount = map[int]uint64{}
+		bpmap.breakpointIDCounter++
+		newBreaklet.LogicalID = bpmap.breakpointIDCounter
 	}
 	if bp, ok := bpmap.M[addr]; ok {
 		if !bp.canOverlap(kind) {
 			return bp, BreakpointExistsError{bp.File, bp.Line, bp.Addr}
 		}
+		if kind == UserBreakpoint {
+			bp.Tracepoint = false
+			bp.TraceReturn = false
+			bp.Goroutine = false
+			bp.Stacktrace = 0
+			bp.Variables = nil
+			bp.LoadArgs = nil
+			bp.LoadLocals = nil
+		}
 		bp.Breaklets = append(bp.Breaklets, newBreaklet)
 		return bp, nil
 	}
@@ -655,14 +677,6 @@ func (t *Target) setBreakpointInternal(addr uint64, kind BreakpointKind, wtype W
 		return nil, err
 	}
 
-	if kind != UserBreakpoint {
-		bpmap.internalBreakpointIDCounter++
-		newBreakpoint.LogicalID = bpmap.internalBreakpointIDCounter
-	} else {
-		bpmap.breakpointIDCounter++
-		newBreakpoint.LogicalID = bpmap.breakpointIDCounter
-	}
-
 	newBreakpoint.Breaklets = append(newBreakpoint.Breaklets, newBreaklet)
 
 	bpmap.M[addr] = newBreakpoint
@@ -675,8 +689,13 @@ func (t *Target) SetBreakpointWithID(id int, addr uint64) (*Breakpoint, error) {
 	bpmap := t.Breakpoints()
 	bp, err := t.SetBreakpoint(addr, UserBreakpoint, nil)
 	if err == nil {
-		bp.LogicalID = id
+		for _, breaklet := range bp.Breaklets {
+			if breaklet.Kind == UserBreakpoint {
+				breaklet.LogicalID = id
 				bpmap.breakpointIDCounter--
+				break
+			}
+		}
 	}
 	return bp, err
 }
@@ -693,13 +712,13 @@ func (bp *Breakpoint) canOverlap(kind BreakpointKind) bool {
 }
 
 // ClearBreakpoint clears the breakpoint at addr.
-func (t *Target) ClearBreakpoint(addr uint64) (*Breakpoint, error) {
+func (t *Target) ClearBreakpoint(addr uint64) error {
 	if valid, err := t.Valid(); !valid {
-		return nil, err
+		return err
 	}
 	bp, ok := t.Breakpoints().M[addr]
 	if !ok {
-		return nil, NoBreakpointError{Addr: addr}
+		return NoBreakpointError{Addr: addr}
 	}
 
 	for i := range bp.Breaklets {
@@ -710,18 +729,18 @@ func (t *Target) ClearBreakpoint(addr uint64) (*Breakpoint, error) {
 
 	_, err := t.finishClearBreakpoint(bp)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	if bp.WatchExpr != "" && bp.watchStackOff != 0 {
 		// stack watchpoint, must remove all its WatchOutOfScopeBreakpoints/StackResizeBreakpoints
 		err := t.clearStackWatchBreakpoints(bp)
 		if err != nil {
-			return bp, err
+			return err
 		}
 	}
 
-	return bp, nil
+	return nil
 }
 
 // ClearSteppingBreakpoints removes all stepping breakpoints from the map,

pkg/proc/gdbserial/rr_test.go

@@ -251,7 +251,7 @@ func TestCheckpoints(t *testing.T) {
 
 		// Delete breakpoint, move back to checkpoint then next twice and check
 		// output of 'when' again
-		_, err = p.ClearBreakpoint(bp.Addr)
+		err = p.ClearBreakpoint(bp.Addr)
 		assertNoError(err, t, "ClearBreakpoint")
 		p.Restart(fmt.Sprintf("c%d", cpid))
 		g, _ = proc.FindGoroutine(p, 1)
@@ -283,7 +283,7 @@ func TestIssue1376(t *testing.T) {
 	withTestRecording("continuetestprog", t, func(p *proc.Target, fixture protest.Fixture) {
 		bp := setFunctionBreakpoint(p, t, "main.main")
 		assertNoError(p.Continue(), t, "Continue (forward)")
-		_, err := p.ClearBreakpoint(bp.Addr)
+		err := p.ClearBreakpoint(bp.Addr)
 		assertNoError(err, t, "ClearBreakpoint")
 		assertNoError(p.ChangeDirection(proc.Backward), t, "Switching to backward direction")
 		assertNoError(p.Continue(), t, "Continue (backward)")

pkg/proc/proc_test.go

@@ -360,7 +360,7 @@ func TestClearBreakpointBreakpoint(t *testing.T) {
 	withTestProcess("testprog", t, func(p *proc.Target, fixture protest.Fixture) {
 		bp := setFunctionBreakpoint(p, t, "main.sleepytime")
 
-		_, err := p.ClearBreakpoint(bp.Addr)
+		err := p.ClearBreakpoint(bp.Addr)
 		assertNoError(err, t, "ClearBreakpoint()")
 
 		data, err := dataAtAddr(p.Memory(), bp.Addr)
@@ -384,7 +384,7 @@ type nextTest struct {
 func countBreakpoints(p *proc.Target) int {
 	bpcount := 0
 	for _, bp := range p.Breakpoints().M {
-		if bp.LogicalID >= 0 {
+		if bp.LogicalID() >= 0 {
 			bpcount++
 		}
 	}
@@ -473,7 +473,7 @@ func testseq2Args(wd string, args []string, buildFlags protest.BuildFlags, t *te
 					if traceTestseq2 {
 						t.Log("clearing initial breakpoint")
 					}
-					_, err := p.ClearBreakpoint(bp.Addr)
+					err := p.ClearBreakpoint(bp.Addr)
 					assertNoError(err, t, "ClearBreakpoint() returned an error")
 				}
 			case contReverseNext:
@@ -503,7 +503,7 @@ func testseq2Args(wd string, args []string, buildFlags protest.BuildFlags, t *te
 					t.Log("continue")
 				}
 				assertNoError(p.Continue(), t, "Continue() returned an error")
-				_, err := p.ClearBreakpoint(bp.Addr)
+				err := p.ClearBreakpoint(bp.Addr)
 				assertNoError(err, t, "ClearBreakpoint() returned an error")
 			}
 
@@ -596,7 +596,7 @@ func TestNextConcurrent(t *testing.T) {
 		f, ln := currentLineNumber(p, t)
 		initV := evalVariable(p, t, "n")
 		initVval, _ := constant.Int64Val(initV.Value)
-		_, err := p.ClearBreakpoint(bp.Addr)
+		err := p.ClearBreakpoint(bp.Addr)
 		assertNoError(err, t, "ClearBreakpoint()")
 		for _, tc := range testcases {
 			g, err := proc.GetG(p.CurrentThread())
@@ -1086,11 +1086,11 @@ func TestContinueMulti(t *testing.T) {
 			}
 			assertNoError(err, t, "Continue()")
 
-			if bp := p.CurrentThread().Breakpoint(); bp.LogicalID == bp1.LogicalID {
+			if bp := p.CurrentThread().Breakpoint(); bp.LogicalID() == bp1.LogicalID() {
 				mainCount++
 			}
 
-			if bp := p.CurrentThread().Breakpoint(); bp.LogicalID == bp2.LogicalID {
+			if bp := p.CurrentThread().Breakpoint(); bp.LogicalID() == bp2.LogicalID() {
 				sayhiCount++
 			}
 		}
@@ -2415,12 +2415,12 @@ func TestStepConcurrentDirect(t *testing.T) {
 		bp := setFileBreakpoint(p, t, fixture.Source, 37)
 
 		assertNoError(p.Continue(), t, "Continue()")
-		_, err := p.ClearBreakpoint(bp.Addr)
+		err := p.ClearBreakpoint(bp.Addr)
 		assertNoError(err, t, "ClearBreakpoint()")
 
 		for _, b := range p.Breakpoints().M {
 			if b.Name == proc.UnrecoveredPanic {
-				_, err := p.ClearBreakpoint(b.Addr)
+				err := p.ClearBreakpoint(b.Addr)
 				assertNoError(err, t, "ClearBreakpoint(unrecovered-panic)")
 				break
 			}
@@ -2480,7 +2480,7 @@ func TestStepConcurrentPtr(t *testing.T) {
 
 		for _, b := range p.Breakpoints().M {
 			if b.Name == proc.UnrecoveredPanic {
-				_, err := p.ClearBreakpoint(b.Addr)
+				err := p.ClearBreakpoint(b.Addr)
 				assertNoError(err, t, "ClearBreakpoint(unrecovered-panic)")
 				break
 			}
@@ -2972,7 +2972,7 @@ func TestRecursiveNext(t *testing.T) {
 	withTestProcess("increment", t, func(p *proc.Target, fixture protest.Fixture) {
 		bp := setFunctionBreakpoint(p, t, "main.Increment")
 		assertNoError(p.Continue(), t, "Continue")
-		_, err := p.ClearBreakpoint(bp.Addr)
+		err := p.ClearBreakpoint(bp.Addr)
 		assertNoError(err, t, "ClearBreakpoint")
 		assertNoError(p.Next(), t, "Next 1")
 		assertNoError(p.Next(), t, "Next 2")
@@ -4509,12 +4509,12 @@ func TestCallConcurrent(t *testing.T) {
 		returned := testCallConcurrentCheckReturns(p, t, gid1, -1)
 
 		curthread := p.CurrentThread()
-		if curbp := curthread.Breakpoint(); curbp.Breakpoint == nil || curbp.LogicalID != bp.LogicalID || returned > 0 {
+		if curbp := curthread.Breakpoint(); curbp.Breakpoint == nil || curbp.LogicalID() != bp.LogicalID() || returned > 0 {
 			t.Logf("skipping test, the call injection terminated before we hit a breakpoint in a different thread")
 			return
 		}
 
-		_, err := p.ClearBreakpoint(bp.Addr)
+		err := p.ClearBreakpoint(bp.Addr)
 		assertNoError(err, t, "ClearBreakpoint() returned an error")
 
 		gid2 := p.SelectedGoroutine().ID
@@ -5551,7 +5551,7 @@ func TestWatchpointStack(t *testing.T) {
 			t.Errorf("wrong number of out-of-scope watchpoints after watchpoint goes out of scope: %d", len(p.Breakpoints().WatchOutOfScope))
 		}
 
-		_, err = p.ClearBreakpoint(retaddr)
+		err = p.ClearBreakpoint(retaddr)
 		assertNoError(err, t, "ClearBreakpoint")
 
 		if len(p.Breakpoints().M) != clearlen {

pkg/proc/scope_test.go

@@ -104,7 +104,7 @@ func TestScope(t *testing.T) {
 			}
 
 			scopeCheck.ok = true
-			_, err := p.ClearBreakpoint(bp.Addr)
+			err := p.ClearBreakpoint(bp.Addr)
 			assertNoError(err, t, "ClearBreakpoint")
 		}
 	})

pkg/proc/stackwatch.go

@@ -137,7 +137,7 @@ func (t *Target) clearStackWatchBreakpoints(watchpoint *Breakpoint) error {
 // user is notified of the watchpoint going out of scope.
 func watchpointOutOfScope(t *Target, watchpoint *Breakpoint) {
 	t.Breakpoints().WatchOutOfScope = append(t.Breakpoints().WatchOutOfScope, watchpoint)
-	_, err := t.ClearBreakpoint(watchpoint.Addr)
+	err := t.ClearBreakpoint(watchpoint.Addr)
 	if err != nil {
 		log := logflags.DebuggerLogger()
 		log.Errorf("could not clear out-of-scope watchpoint: %v", err)

pkg/proc/target.go

@@ -325,7 +325,7 @@ func (t *Target) Detach(kill bool) error {
 		}
 		for _, bp := range t.Breakpoints().M {
 			if bp != nil {
-				_, err := t.ClearBreakpoint(bp.Addr)
+				err := t.ClearBreakpoint(bp.Addr)
 				if err != nil {
 					return err
 				}

service/api/conversions.go

@@ -21,7 +21,7 @@ import (
 func ConvertBreakpoint(bp *proc.Breakpoint) *Breakpoint {
 	b := &Breakpoint{
 		Name:         bp.Name,
-		ID:           bp.LogicalID,
+		ID:           bp.LogicalID(),
 		FunctionName: bp.FunctionName,
 		File:         bp.File,
 		Line:         bp.Line,
@@ -68,10 +68,10 @@ func ConvertBreakpoints(bps []*proc.Breakpoint) []*Breakpoint {
 	r := make([]*Breakpoint, 0, len(bps))
 	for _, bp := range bps {
 		if len(r) > 0 {
-			if r[len(r)-1].ID == bp.LogicalID {
+			if r[len(r)-1].ID == bp.LogicalID() {
 				r[len(r)-1].Addrs = append(r[len(r)-1].Addrs, bp.Addr)
 				continue
-			} else if r[len(r)-1].ID > bp.LogicalID {
+			} else if r[len(r)-1].ID > bp.LogicalID() {
 				panic("input not sorted")
 			}
 		}

service/debugger/debugger.go

@@ -722,13 +722,13 @@ func createLogicalBreakpoint(d *Debugger, addrs []uint64, requestedBp *api.Break
 			bps[i], err = p.SetBreakpointWithID(id, addrs[i])
 		} else {
 			bps[i], err = p.SetBreakpoint(addrs[i], proc.UserBreakpoint, nil)
+			if err == nil {
+				id = bps[i].LogicalID()
+			}
 		}
 		if err != nil {
 			break
 		}
-		if i > 0 {
-			bps[i].LogicalID = bps[0].LogicalID
-		}
 		err = copyBreakpointInfo(bps[i], requestedBp)
 		if err != nil {
 			break
@@ -742,7 +742,7 @@ func createLogicalBreakpoint(d *Debugger, addrs []uint64, requestedBp *api.Break
 			if bp == nil {
 				continue
 			}
-			if _, err1 := p.ClearBreakpoint(bp.Addr); err1 != nil {
+			if err1 := p.ClearBreakpoint(bp.Addr); err1 != nil {
 				err = fmt.Errorf("error while creating breakpoint: %v, additionally the breakpoint could not be properly rolled back: %v", err, err1)
 				return nil, err
 			}
@@ -901,17 +901,20 @@ func (d *Debugger) clearBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint
 		return bp, nil
 	}
 
-	var bps []*proc.Breakpoint
+	var clearedBp *api.Breakpoint
 	var errs []error
 
 	clear := func(addr uint64) {
-		bp, err := d.target.ClearBreakpoint(addr)
+		if clearedBp == nil {
+			bp := d.target.Breakpoints().M[addr]
+			if bp != nil {
+				clearedBp = api.ConvertBreakpoint(bp)
+			}
+		}
+		err := d.target.ClearBreakpoint(addr)
 		if err != nil {
 			errs = append(errs, fmt.Errorf("address %#x: %v", addr, err))
 		}
-		if bp != nil {
-			bps = append(bps, bp)
-		}
 	}
 
 	clearAddr := true
@@ -934,18 +937,14 @@ func (d *Debugger) clearBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint
 			}
 		}
 
-		if len(bps) == 0 {
+		if clearedBp == nil {
 			return nil, fmt.Errorf("unable to clear breakpoint %d: %v", requestedBp.ID, buf.String())
 		}
 		return nil, fmt.Errorf("unable to clear breakpoint %d (partial): %s", requestedBp.ID, buf.String())
 	}
 
-	clearedBp := api.ConvertBreakpoints(bps)
-	if len(clearedBp) == 0 {
-		return nil, nil
-	}
 	d.log.Infof("cleared breakpoint: %#v", clearedBp)
-	return clearedBp[0], nil
+	return clearedBp, nil
 }
 
 // Breakpoints returns the list of current breakpoints.
@@ -998,7 +997,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.IsUser() && bp.LogicalID == id {
+		if bp.LogicalID() == id {
 			bps = append(bps, bp)
 		}
 	}
@@ -2181,11 +2180,11 @@ func (v breakpointsByLogicalID) Len() int      { return len(v) }
 func (v breakpointsByLogicalID) Swap(i, j int) { v[i], v[j] = v[j], v[i] }
 
 func (v breakpointsByLogicalID) Less(i, j int) bool {
-	if v[i].LogicalID == v[j].LogicalID {
+	if v[i].LogicalID() == v[j].LogicalID() {
 		if v[i].WatchType != v[j].WatchType {
 			return v[i].WatchType > v[j].WatchType // if a logical breakpoint contains a watchpoint let the watchpoint sort first
 		}
 		return v[i].Addr < v[j].Addr
 	}
-	return v[i].LogicalID < v[j].LogicalID
+	return v[i].LogicalID() < v[j].LogicalID()
 }