Automatically disable breakpoints with hitcount conditions that will never be satisfied again (#2833)

* service/debugger: disable breakpoints with hitcond not satisfiable

To avoid slowing down the debugged process unnecessarily, we disable
breakpoints with a hit condition that can no longer be hit again.

* test: add integration tests for hit conditions no more satisfiable

* proc/test: fix typo in breakpoints related tests

* test: use the new API for hitcond integration tests

pkg/proc/proc_test.go

@@ -1679,7 +1679,7 @@ func TestCondBreakpoint(t *testing.T) {
 
 		n, _ := constant.Int64Val(nvar.Value)
 		if n != 7 {
-			t.Fatalf("Stoppend on wrong goroutine %d\n", n)
+			t.Fatalf("Stopped on wrong goroutine %d\n", n)
 		}
 	})
 }
@@ -1720,7 +1720,7 @@ func TestCondBreakpointError(t *testing.T) {
 
 			n, _ := constant.Int64Val(nvar.Value)
 			if n != 7 {
-				t.Fatalf("Stoppend on wrong goroutine %d\n", n)
+				t.Fatalf("Stopped on wrong goroutine %d\n", n)
 			}
 		}
 	})
@@ -1739,7 +1739,7 @@ func TestHitCondBreakpointEQ(t *testing.T) {
 
 		i, _ := constant.Int64Val(ivar.Value)
 		if i != 3 {
-			t.Fatalf("Stoppend on wrong hitcount %d\n", i)
+			t.Fatalf("Stopped on wrong hitcount %d\n", i)
 		}
 
 		err := p.Continue()
@@ -1764,7 +1764,7 @@ func TestHitCondBreakpointGEQ(t *testing.T) {
 
 			i, _ := constant.Int64Val(ivar.Value)
 			if int(i) != it {
-				t.Fatalf("Stoppend on wrong hitcount %d\n", i)
+				t.Fatalf("Stopped on wrong hitcount %d\n", i)
 			}
 		}
 
@@ -1787,7 +1787,7 @@ func TestHitCondBreakpointREM(t *testing.T) {
 
 			i, _ := constant.Int64Val(ivar.Value)
 			if int(i) != it {
-				t.Fatalf("Stoppend on wrong hitcount %d\n", i)
+				t.Fatalf("Stopped on wrong hitcount %d\n", i)
 			}
 		}
 

service/dap/server.go

@@ -1346,6 +1346,7 @@ func (s *Session) setBreakpoints(prefix string, totalBps int, metadataFunc func(
 		if _, ok := createdBps[want.name]; ok {
 			err = fmt.Errorf("breakpoint already exists")
 		} else {
+			got.Disabled = false
 			got.Cond = want.condition
 			got.HitCond = want.hitCondition
 			err = setLogMessage(got, want.logMessage)

service/debugger/debugger.go

@@ -770,12 +770,10 @@ func (d *Debugger) CreateEBPFTracepoint(fnName string) error {
 	return d.target.SetEBPFTracepoint(fnName)
 }
 
-// AmendBreakpoint will update the breakpoint with the matching ID.
+// amendBreakpoint will update the breakpoint with the matching ID.
 // It also enables or disables the breakpoint.
-func (d *Debugger) AmendBreakpoint(amend *api.Breakpoint) error {
+// We can consume this function to avoid locking a goroutine.
-	d.targetMutex.Lock()
+func (d *Debugger) amendBreakpoint(amend *api.Breakpoint) error {
-	defer d.targetMutex.Unlock()
-
 	originals := d.findBreakpoint(amend.ID)
 
 	if len(originals) > 0 && originals[0].WatchExpr != "" && amend.Disabled {
@@ -792,6 +790,17 @@ func (d *Debugger) AmendBreakpoint(amend *api.Breakpoint) error {
 			return err
 		}
 		copyBreakpointInfo(bp, amend)
+		if breaklet := bp.UserBreaklet(); breaklet != nil {
+			breaklet.TotalHitCount = amend.TotalHitCount
+			breaklet.HitCount = map[int]uint64{}
+			for idx := range amend.HitCount {
+				i, err := strconv.Atoi(idx)
+				if err != nil {
+					return fmt.Errorf("can't convert goroutine ID: %w", err)
+				}
+				breaklet.HitCount[i] = amend.HitCount[idx]
+			}
+		}
 		delete(d.disabledBreakpoints, amend.ID)
 	}
 	if amend.Disabled && !disabled { // disable the breakpoint
@@ -809,6 +818,15 @@ func (d *Debugger) AmendBreakpoint(amend *api.Breakpoint) error {
 	return nil
 }
 
+// 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()
+
+	return d.amendBreakpoint(amend)
+}
+
 // CancelNext will clear internal breakpoints, thus cancelling the 'next',
 // 'step' or 'stepout' operation.
 func (d *Debugger) CancelNext() error {
@@ -957,6 +975,33 @@ func (d *Debugger) clearBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint
 	return clearedBp, nil
 }
 
+// isBpHitCondNotSatisfiable returns true if the breakpoint bp has a hit
+// condition that is no more satisfiable.
+// The hit condition is considered no more satisfiable if it can no longer be
+// hit again, for example with {Op: "==", Val: 1} and TotalHitCount == 1.
+func isBpHitCondNotSatisfiable(bp *api.Breakpoint) bool {
+	if bp.HitCond == "" {
+		return false
+	}
+
+	tok, val, err := parseHitCondition(bp.HitCond)
+	if err != nil {
+		return false
+	}
+	switch tok {
+	case token.EQL, token.LEQ:
+		if int(bp.TotalHitCount) >= val {
+			return true
+		}
+	case token.LSS:
+		if int(bp.TotalHitCount) >= val-1 {
+			return true
+		}
+	}
+
+	return false
+}
+
 // Breakpoints returns the list of current breakpoints.
 func (d *Debugger) Breakpoints(all bool) []*api.Breakpoint {
 	d.targetMutex.Lock()
@@ -1284,6 +1329,10 @@ func (d *Debugger) Command(command *api.DebuggerCommand, resumeNotify chan struc
 			}
 		}
 	}
+	if bp := state.CurrentThread.Breakpoint; bp != nil && isBpHitCondNotSatisfiable(bp) {
+		bp.Disabled = true
+		d.amendBreakpoint(bp)
+	}
 	return state, err
 }
 

service/test/integration2_test.go

@@ -615,6 +615,121 @@ func TestClientServer_toggleAmendedBreakpoint(t *testing.T) {
 	})
 }
 
+func TestClientServer_disableHitCondLSSBreakpoint(t *testing.T) {
+	withTestClient2("break", t, func(c service.Client) {
+		fp := testProgPath(t, "break")
+		hitCondBp, err := c.CreateBreakpoint(&api.Breakpoint{
+			File:    fp,
+			Line:    7,
+			HitCond: "< 3",
+		})
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
+
+		state := <-c.Continue()
+		if state.Err != nil {
+			t.Fatalf("Unexpected error: %v, state: %#v", state.Err, state)
+		}
+
+		f, l := state.CurrentThread.File, state.CurrentThread.Line
+		if f != "break.go" && l != 7 {
+			t.Fatal("Program did not hit breakpoint")
+		}
+
+		ivar, err := c.EvalVariable(api.EvalScope{GoroutineID: -1}, "i", normalLoadConfig)
+		assertNoError(err, t, "EvalVariable")
+
+		t.Logf("ivar: %s", ivar.SinglelineString())
+
+		if ivar.Value != "1" {
+			t.Fatalf("Wrong variable value: %s", ivar.Value)
+		}
+
+		bp, err := c.GetBreakpoint(hitCondBp.ID)
+		assertNoError(err, t, "GetBreakpoint()")
+
+		if bp.Disabled {
+			t.Fatalf(
+				"Hit condition %s is still satisfiable but breakpoint has been disabled",
+				bp.HitCond,
+			)
+		}
+
+		state = <-c.Continue()
+		if state.Err != nil {
+			t.Fatalf("Unexpected error: %v, state: %#v", state.Err, state)
+		}
+
+		f, l = state.CurrentThread.File, state.CurrentThread.Line
+		if f != "break.go" && l != 7 {
+			t.Fatal("Program did not hit breakpoint")
+		}
+
+		ivar, err = c.EvalVariable(api.EvalScope{GoroutineID: -1}, "i", normalLoadConfig)
+		assertNoError(err, t, "EvalVariable")
+
+		t.Logf("ivar: %s", ivar.SinglelineString())
+
+		if ivar.Value != "2" {
+			t.Fatalf("Wrong variable value: %s", ivar.Value)
+		}
+
+		bp, err = c.GetBreakpoint(hitCondBp.ID)
+		assertNoError(err, t, "GetBreakpoint()")
+
+		if !bp.Disabled {
+			t.Fatalf(
+				"Hit condition %s is no more satisfiable but breakpoint has not been disabled",
+				bp.HitCond,
+			)
+		}
+	})
+}
+
+func TestClientServer_disableHitEQLCondBreakpoint(t *testing.T) {
+	withTestClient2("break", t, func(c service.Client) {
+		fp := testProgPath(t, "break")
+		hitCondBp, err := c.CreateBreakpoint(&api.Breakpoint{
+			File:    fp,
+			Line:    7,
+			HitCond: "== 3",
+		})
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
+
+		state := <-c.Continue()
+		if state.Err != nil {
+			t.Fatalf("Unexpected error: %v, state: %#v", state.Err, state)
+		}
+
+		f, l := state.CurrentThread.File, state.CurrentThread.Line
+		if f != "break.go" && l != 7 {
+			t.Fatal("Program did not hit breakpoint")
+		}
+
+		ivar, err := c.EvalVariable(api.EvalScope{GoroutineID: -1}, "i", normalLoadConfig)
+		assertNoError(err, t, "EvalVariable")
+
+		t.Logf("ivar: %s", ivar.SinglelineString())
+
+		if ivar.Value != "3" {
+			t.Fatalf("Wrong variable value: %s", ivar.Value)
+		}
+
+		bp, err := c.GetBreakpoint(hitCondBp.ID)
+		assertNoError(err, t, "GetBreakpoint()")
+
+		if !bp.Disabled {
+			t.Fatalf(
+				"Hit condition %s is no more satisfiable but breakpoint has not been disabled",
+				bp.HitCond,
+			)
+		}
+	})
+}
+
 func TestClientServer_switchThread(t *testing.T) {
 	protest.AllowRecording(t)
 	withTestClient2("testnextprog", t, func(c service.Client) {