pkg/proc,service/debugger: do not disable unsatisfiable breakpoints (#…

…3868)

Previously breakpoints with hitcount conditions that became
unsatisfiable
would become disabled, this was done as an optimization so that the
continue loop would no longer need to stop on them and evaluate their
conditions.
As a side effect this meant that on restart these breakpoints would
remain disabled, even though their hit condition returned satisfiable.

This commit changes Delve behavior so that breakpoints with
unsatisifiable hitcount conditions are no longer disabled but the
associated physical breakpoints are removed anyway, preserving the
optimization.

Some refactoring is done to the way conditions are represented and the
enable status is managed so that in the future it will be possible to
use hitcount conditions to implement "chained" breakpoints (also known
as dependet breakpoints), i.e. breakpoints that become active only
after a second breakpoint has been hit.

pkg/proc/breakpoints.go

@@ -1,12 +1,14 @@
 package proc
 
 import (
+	"bytes"
 	"debug/dwarf"
 	"errors"
 	"fmt"
 	"go/ast"
 	"go/constant"
 	"go/parser"
+	"go/printer"
 	"go/token"
 	"reflect"
 
@@ -211,7 +213,7 @@ func (bp *Breakpoint) VerboseDescr() []string {
 	for _, breaklet := range bp.Breaklets {
 		switch breaklet.Kind {
 		case UserBreakpoint:
-			r = append(r, fmt.Sprintf("User Cond=%q HitCond=%v", exprToString(breaklet.Cond), lbp.HitCond))
+			r = append(r, fmt.Sprintf("User Cond=%q HitCond=%v", exprToString(breaklet.Cond), lbp.hitCond))
 		case NextBreakpoint:
 			r = append(r, fmt.Sprintf("Next Cond=%q", exprToString(breaklet.Cond)))
 		case NextDeferBreakpoint:
@@ -360,29 +362,29 @@ func (bpstate *BreakpointState) checkCond(tgt *Target, breaklet *Breaklet, threa
 
 // checkHitCond evaluates bp's hit condition on thread.
 func checkHitCond(lbp *LogicalBreakpoint, goroutineID int64) bool {
-	if lbp == nil || lbp.HitCond == nil {
+	if lbp == nil || lbp.hitCond == nil {
 		return true
 	}
 	hitCount := int(lbp.TotalHitCount)
 	if lbp.HitCondPerG && goroutineID > 0 {
 		hitCount = int(lbp.HitCount[goroutineID])
 	}
 	// Evaluate the breakpoint condition.
-	switch lbp.HitCond.Op {
+	switch lbp.hitCond.Op {
 	case token.EQL:
-		return hitCount == lbp.HitCond.Val
+		return hitCount == lbp.hitCond.Val
 	case token.NEQ:
-		return hitCount != lbp.HitCond.Val
+		return hitCount != lbp.hitCond.Val
 	case token.GTR:
-		return hitCount > lbp.HitCond.Val
+		return hitCount > lbp.hitCond.Val
 	case token.LSS:
-		return hitCount < lbp.HitCond.Val
+		return hitCount < lbp.hitCond.Val
 	case token.GEQ:
-		return hitCount >= lbp.HitCond.Val
+		return hitCount >= lbp.hitCond.Val
 	case token.LEQ:
-		return hitCount <= lbp.HitCond.Val
+		return hitCount <= lbp.hitCond.Val
 	case token.REM:
-		return hitCount%lbp.HitCond.Val == 0
+		return hitCount%lbp.hitCond.Val == 0
 	}
 	return false
 }
@@ -699,13 +701,14 @@ func (t *Target) setBreakpointInternal(logicalID int, addr uint64, kind Breakpoi
 		if lbp == nil {
 			lbp = &LogicalBreakpoint{LogicalID: logicalID}
 			lbp.HitCount = make(map[int64]uint64)
-			lbp.Enabled = true
+			lbp.enabled = true
+			lbp.condSatisfiable = true
 			bpmap.Logical[logicalID] = lbp
 		}
 		bp.Logical = lbp
 		breaklet := bp.UserBreaklet()
 		if breaklet != nil && breaklet.Cond == nil {
-			breaklet.Cond = lbp.Cond
+			breaklet.Cond = lbp.cond
 		}
 		if lbp.File == "" && lbp.Line == 0 {
 			lbp.File = bp.File
@@ -1032,7 +1035,7 @@ type LogicalBreakpoint struct {
 	FunctionName string
 	File         string
 	Line         int
-	Enabled      bool
+	enabled      bool
 
 	Set SetBreakpoint
 
@@ -1048,15 +1051,18 @@ type LogicalBreakpoint struct {
 	TotalHitCount uint64           // Number of times a breakpoint has been reached
 	HitCondPerG   bool             // Use per goroutine hitcount as HitCond operand, instead of total hitcount
 
-	// HitCond: if not nil the breakpoint will be triggered only if the evaluated HitCond returns
+	// hitCond: if not nil the breakpoint will be triggered only if the evaluated HitCond returns
 	// true with the TotalHitCount.
-	HitCond *struct {
+	hitCond *struct {
 		Op  token.Token
 		Val int
 	}
 
-	// Cond: if not nil the breakpoint will be triggered only if evaluating Cond returns true
-	Cond ast.Expr
+	// cond: if not nil the breakpoint will be triggered only if evaluating Cond returns true
+	cond ast.Expr
+
+	// condSatisfiable is true when 'cond && hitCond' can potentially be true.
+	condSatisfiable bool
 
 	UserData interface{} // Any additional information about the breakpoint
 	// Name of root function from where tracing needs to be done
@@ -1079,3 +1085,35 @@ type PidAddr struct {
 	Pid  int
 	Addr uint64
 }
+
+// Enabled returns true if the breakpoint is enabled.
+func (lbp *LogicalBreakpoint) Enabled() bool {
+	return lbp.enabled
+}
+
+// HitCond returns the hit condition.
+func (lbp *LogicalBreakpoint) HitCond() string {
+	if lbp.hitCond == nil {
+		return ""
+	}
+	return fmt.Sprintf("%s %d", lbp.hitCond.Op.String(), lbp.hitCond.Val)
+}
+
+func (lbp *LogicalBreakpoint) Cond() string {
+	var buf bytes.Buffer
+	printer.Fprint(&buf, token.NewFileSet(), lbp.cond)
+	return buf.String()
+}
+
+func breakpointConditionSatisfiable(lbp *LogicalBreakpoint) bool {
+	if lbp.hitCond == nil || lbp.HitCondPerG {
+		return true
+	}
+	switch lbp.hitCond.Op {
+	case token.EQL, token.LEQ:
+		return int(lbp.TotalHitCount) < lbp.hitCond.Val
+	case token.LSS:
+		return int(lbp.TotalHitCount) < lbp.hitCond.Val-1
+	}
+	return true
+}

pkg/proc/proc_test.go

@@ -1540,10 +1540,7 @@ func TestCondBreakpointError(t *testing.T) {
 func TestHitCondBreakpointEQ(t *testing.T) {
 	withTestProcess("break", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
 		bp := setFileBreakpoint(p, t, fixture.Source, 7)
-		bp.Logical.HitCond = &struct {
-			Op  token.Token
-			Val int
-		}{token.EQL, 3}
+		grp.ChangeBreakpointCondition(bp.Logical, "", "== 3", false)
 
 		assertNoError(grp.Continue(), t, "Continue()")
 		ivar := evalVariable(p, t, "i")
@@ -1564,10 +1561,7 @@ func TestHitCondBreakpointGEQ(t *testing.T) {
 	protest.AllowRecording(t)
 	withTestProcess("break", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
 		bp := setFileBreakpoint(p, t, fixture.Source, 7)
-		bp.Logical.HitCond = &struct {
-			Op  token.Token
-			Val int
-		}{token.GEQ, 3}
+		grp.ChangeBreakpointCondition(bp.Logical, "", ">= 3", false)
 
 		for it := 3; it <= 10; it++ {
 			assertNoError(grp.Continue(), t, "Continue()")
@@ -1587,10 +1581,7 @@ func TestHitCondBreakpointREM(t *testing.T) {
 	protest.AllowRecording(t)
 	withTestProcess("break", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
 		bp := setFileBreakpoint(p, t, fixture.Source, 7)
-		bp.Logical.HitCond = &struct {
-			Op  token.Token
-			Val int
-		}{token.REM, 2}
+		grp.ChangeBreakpointCondition(bp.Logical, "", "% 2", false)
 
 		for it := 2; it <= 10; it += 2 {
 			assertNoError(grp.Continue(), t, "Continue()")
@@ -5123,8 +5114,9 @@ func TestFollowExec(t *testing.T) {
 		grp.LogicalBreakpoints[2] = &proc.LogicalBreakpoint{LogicalID: 2, Set: proc.SetBreakpoint{FunctionName: "main.traceme2"}, HitCount: make(map[int64]uint64)}
 		grp.LogicalBreakpoints[3] = &proc.LogicalBreakpoint{LogicalID: 3, Set: proc.SetBreakpoint{FunctionName: "main.traceme3"}, HitCount: make(map[int64]uint64)}
 
-		assertNoError(grp.EnableBreakpoint(grp.LogicalBreakpoints[1]), t, "EnableBreakpoint(main.traceme1)")
-		assertNoError(grp.EnableBreakpoint(grp.LogicalBreakpoints[3]), t, "EnableBreakpoint(main.traceme3)")
+		assertNoError(grp.SetBreakpointEnabled(grp.LogicalBreakpoints[1], true), t, "EnableBreakpoint(main.traceme1)")
+		assertNoError(grp.SetBreakpointEnabled(grp.LogicalBreakpoints[2], true), t, "EnableBreakpoint(main.traceme2)")
+		assertNoError(grp.SetBreakpointEnabled(grp.LogicalBreakpoints[3], true), t, "EnableBreakpoint(main.traceme3)")
 
 		assertNoError(grp.FollowExec(true, ""), t, "FollowExec")
 
@@ -5297,8 +5289,9 @@ func TestFollowExecRegexFilter(t *testing.T) {
 		grp.LogicalBreakpoints[2] = &proc.LogicalBreakpoint{LogicalID: 2, Set: proc.SetBreakpoint{FunctionName: "main.traceme2"}, HitCount: make(map[int64]uint64)}
 		grp.LogicalBreakpoints[3] = &proc.LogicalBreakpoint{LogicalID: 3, Set: proc.SetBreakpoint{FunctionName: "main.traceme3"}, HitCount: make(map[int64]uint64)}
 
-		assertNoError(grp.EnableBreakpoint(grp.LogicalBreakpoints[1]), t, "EnableBreakpoint(main.traceme1)")
-		assertNoError(grp.EnableBreakpoint(grp.LogicalBreakpoints[3]), t, "EnableBreakpoint(main.traceme3)")
+		assertNoError(grp.SetBreakpointEnabled(grp.LogicalBreakpoints[1], true), t, "EnableBreakpoint(main.traceme1)")
+		assertNoError(grp.SetBreakpointEnabled(grp.LogicalBreakpoints[2], true), t, "EnableBreakpoint(main.traceme2)")
+		assertNoError(grp.SetBreakpointEnabled(grp.LogicalBreakpoints[3], true), t, "EnableBreakpoint(main.traceme3)")
 
 		assertNoError(grp.FollowExec(true, "spawn.* child C1"), t, "FollowExec")
 

pkg/proc/target_exec.go

@@ -78,6 +78,11 @@ func (grp *TargetGroup) Continue() error {
 		}
 	}()
 	for {
+		err := grp.manageUnsatisfiableBreakpoints()
+		if err != nil {
+			return err
+		}
+
 		if grp.cctx.CheckAndClearManualStopRequest() {
 			grp.finishManualStop()
 			return nil