From d97b4712929b0c477717a4eed28faf7900e73641 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Thu, 5 Dec 2024 04:07:56 +0100 Subject: [PATCH] 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 | 72 ++++++++++++---- pkg/proc/proc_test.go | 25 ++---- pkg/proc/target_exec.go | 5 ++ pkg/proc/target_group.go | 139 ++++++++++++++++++++++++++++-- pkg/terminal/command_test.go | 1 - service/api/conversions.go | 15 +--- service/debugger/debugger.go | 134 +++------------------------- service/test/integration2_test.go | 114 +++++++++++------------- 8 files changed, 265 insertions(+), 240 deletions(-) diff --git a/pkg/proc/breakpoints.go b/pkg/proc/breakpoints.go index 7f37669a..596eefc4 100644 --- a/pkg/proc/breakpoints.go +++ b/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,7 +362,7 @@ 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) @@ -368,21 +370,21 @@ func checkHitCond(lbp *LogicalBreakpoint, goroutineID int64) bool { 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 +} diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index c9f67020..be59bde6 100644 --- a/pkg/proc/proc_test.go +++ b/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") diff --git a/pkg/proc/target_exec.go b/pkg/proc/target_exec.go index 55dc933d..71c113ea 100644 --- a/pkg/proc/target_exec.go +++ b/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 diff --git a/pkg/proc/target_group.go b/pkg/proc/target_group.go index 1e64dbdd..b31a578f 100644 --- a/pkg/proc/target_group.go +++ b/pkg/proc/target_group.go @@ -3,7 +3,10 @@ package proc import ( "bytes" "fmt" + "go/parser" + "go/token" "regexp" + "strconv" "strings" "github.com/go-delve/delve/pkg/logflags" @@ -76,8 +79,9 @@ func Restart(grp, oldgrp *TargetGroup, discard func(*LogicalBreakpoint, error)) bp.TotalHitCount = 0 bp.HitCount = make(map[int64]uint64) bp.Set.PidAddrs = nil // breakpoints set through a list of addresses can not be restored after a restart - if bp.Enabled { - err := grp.EnableBreakpoint(bp) + if bp.enabled { + bp.condSatisfiable = breakpointConditionSatisfiable(bp) + err := grp.enableBreakpoint(bp) if err != nil { if discard != nil { discard(bp, err) @@ -253,8 +257,22 @@ func (grp *TargetGroup) TargetForThread(tid int) *Target { return nil } -// EnableBreakpoint re-enables a disabled logical breakpoint. -func (grp *TargetGroup) EnableBreakpoint(lbp *LogicalBreakpoint) error { +// SetBreakpointEnabled either enables or disabled the specified breakpoint based on the value of enabled. +func (grp *TargetGroup) SetBreakpointEnabled(lbp *LogicalBreakpoint, enabled bool) (err error) { + switch { + case lbp.enabled && !enabled: + lbp.enabled = false + err = grp.disableBreakpoint(lbp) + case !lbp.enabled && enabled: + lbp.enabled = true + lbp.condSatisfiable = breakpointConditionSatisfiable(lbp) + err = grp.enableBreakpoint(lbp) + } + return +} + +// enableBreakpoint re-enables a disabled logical breakpoint. +func (grp *TargetGroup) enableBreakpoint(lbp *LogicalBreakpoint) error { var err0, errNotFound, errExists error didSet := false targetLoop: @@ -297,11 +315,13 @@ targetLoop: } return err0 } - lbp.Enabled = true return nil } func enableBreakpointOnTarget(p *Target, lbp *LogicalBreakpoint) error { + if !lbp.enabled || !lbp.condSatisfiable { + return nil + } var err error var addrs []uint64 switch { @@ -338,8 +358,8 @@ func enableBreakpointOnTarget(p *Target, lbp *LogicalBreakpoint) error { return err } -// DisableBreakpoint disables a logical breakpoint. -func (grp *TargetGroup) DisableBreakpoint(lbp *LogicalBreakpoint) error { +// disableBreakpoint disables a logical breakpoint. +func (grp *TargetGroup) disableBreakpoint(lbp *LogicalBreakpoint) error { var errs []error n := 0 it := ValidTargets{Group: grp} @@ -368,7 +388,110 @@ func (grp *TargetGroup) DisableBreakpoint(lbp *LogicalBreakpoint) error { } return fmt.Errorf("unable to clear breakpoint %d (partial): %s", lbp.LogicalID, buf.String()) } - lbp.Enabled = false + return nil +} + +// ChangeBreakpointCondition changes the breakpoint condition of lbp. +func (grp *TargetGroup) ChangeBreakpointCondition(lbp *LogicalBreakpoint, cond, hitCond string, hitCondPerG bool) error { + lbp.cond = nil + if cond != "" { + var err error + lbp.cond, err = parser.ParseExpr(cond) + if err != nil { + return err + } + } + + t := ValidTargets{Group: grp} + for t.Next() { + for _, bp := range t.Breakpoints().M { + if bp.LogicalID() == lbp.LogicalID { + bp.UserBreaklet().Cond = lbp.cond + } + } + } + + lbp.hitCond = nil + if hitCond != "" { + opTok, val, err := parseHitCondition(hitCond) + if err != nil { + return err + } + lbp.hitCond = &struct { + Op token.Token + Val int + }{opTok, val} + lbp.HitCondPerG = hitCondPerG + } + + if lbp.enabled { + switch { + case lbp.condSatisfiable && !breakpointConditionSatisfiable(lbp): + lbp.condSatisfiable = false + grp.disableBreakpoint(lbp) + case !lbp.condSatisfiable && breakpointConditionSatisfiable(lbp): + lbp.condSatisfiable = true + grp.enableBreakpoint(lbp) + } + } + + return nil +} + +func parseHitCondition(hitCond string) (token.Token, int, error) { + // A hit condition can be in the following formats: + // - "number" + // - "OP number" + hitConditionRegex := regexp.MustCompile(`(([=><%!])+|)( |)((\d|_)+)`) + + match := hitConditionRegex.FindStringSubmatch(strings.TrimSpace(hitCond)) + if match == nil || len(match) != 6 { + return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\nhit conditions should be of the form \"number\" or \"OP number\"", hitCond) + } + + opStr := match[1] + var opTok token.Token + switch opStr { + case "==", "": + opTok = token.EQL + case ">=": + opTok = token.GEQ + case "<=": + opTok = token.LEQ + case ">": + opTok = token.GTR + case "<": + opTok = token.LSS + case "%": + opTok = token.REM + case "!=": + opTok = token.NEQ + default: + return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\ninvalid operator: %q", hitCond, opStr) + } + + numStr := match[4] + val, parseErr := strconv.Atoi(numStr) + if parseErr != nil { + return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\ninvalid number: %q", hitCond, numStr) + } + + return opTok, val, nil +} + +// manageUnsatisfiableBreakpoints automatically disables breakpoints with unsatisifiable hit conditions. +func (grp *TargetGroup) manageUnsatisfiableBreakpoints() error { + for _, lbp := range grp.LogicalBreakpoints { + if lbp.enabled { + if lbp.condSatisfiable && !breakpointConditionSatisfiable(lbp) { + lbp.condSatisfiable = false + err := grp.disableBreakpoint(lbp) + if err != nil { + return err + } + } + } + } return nil } diff --git a/pkg/terminal/command_test.go b/pkg/terminal/command_test.go index b4269926..3dde9987 100644 --- a/pkg/terminal/command_test.go +++ b/pkg/terminal/command_test.go @@ -1231,7 +1231,6 @@ func TestHitCondBreakpoint(t *testing.T) { if !strings.Contains(out, "2\n") { t.Fatalf("wrong value of j") } - term.MustExec("toggle bp1") listIsAt(t, term, "continue", 16, -1, -1) // second g hit out = term.MustExec("print j") diff --git a/service/api/conversions.go b/service/api/conversions.go index 55933a46..fb90e414 100644 --- a/service/api/conversions.go +++ b/service/api/conversions.go @@ -1,11 +1,8 @@ package api import ( - "bytes" "fmt" "go/constant" - "go/printer" - "go/token" "reflect" "sort" "strconv" @@ -32,7 +29,7 @@ func ConvertLogicalBreakpoint(lbp *proc.LogicalBreakpoint) *Breakpoint { LoadArgs: LoadConfigFromProc(lbp.LoadArgs), LoadLocals: LoadConfigFromProc(lbp.LoadLocals), TotalHitCount: lbp.TotalHitCount, - Disabled: !lbp.Enabled, + Disabled: !lbp.Enabled(), UserData: lbp.UserData, RootFuncName: lbp.RootFuncName, TraceFollowCalls: lbp.TraceFollowCalls, @@ -43,14 +40,10 @@ func ConvertLogicalBreakpoint(lbp *proc.LogicalBreakpoint) *Breakpoint { b.HitCount[strconv.FormatInt(idx, 10)] = lbp.HitCount[idx] } - if lbp.HitCond != nil { - b.HitCond = fmt.Sprintf("%s %d", lbp.HitCond.Op.String(), lbp.HitCond.Val) - b.HitCondPerG = lbp.HitCondPerG - } + b.HitCond = lbp.HitCond() + b.HitCondPerG = lbp.HitCondPerG - var buf bytes.Buffer - printer.Fprint(&buf, token.NewFileSet(), lbp.Cond) - b.Cond = buf.String() + b.Cond = lbp.Cond() return b } diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go index 5ea04f1a..50d4c416 100644 --- a/service/debugger/debugger.go +++ b/service/debugger/debugger.go @@ -7,8 +7,6 @@ import ( "debug/pe" "errors" "fmt" - "go/parser" - "go/token" "io" "os" "os/exec" @@ -18,7 +16,6 @@ import ( "runtime" "slices" "sort" - "strconv" "strings" "sync" "time" @@ -768,10 +765,10 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint, locExpr string, d.breakpointIDCounter = id } - lbp := &proc.LogicalBreakpoint{LogicalID: id, HitCount: make(map[int64]uint64), Enabled: true} + lbp := &proc.LogicalBreakpoint{LogicalID: id, HitCount: make(map[int64]uint64)} d.target.LogicalBreakpoints[id] = lbp - err = copyLogicalBreakpointInfo(lbp, requestedBp) + err = d.copyLogicalBreakpointInfo(lbp, requestedBp) if err != nil { return nil, err } @@ -790,7 +787,7 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint, locExpr string, } } - err = d.target.EnableBreakpoint(lbp) + err = d.target.SetBreakpointEnabled(lbp, true) if err != nil { if suspended { logflags.DebuggerLogger().Debugf("could not enable new breakpoint: %v (breakpoint will be suspended)", err) @@ -847,34 +844,18 @@ func (d *Debugger) amendBreakpoint(amend *api.Breakpoint) error { if original == nil { return fmt.Errorf("no breakpoint with ID %d", amend.ID) } - enabledBefore := original.Enabled - err := copyLogicalBreakpointInfo(original, amend) + if d.isWatchpoint(original) && amend.Disabled { + return errors.New("can not disable watchpoints") + } + err := d.copyLogicalBreakpointInfo(original, amend) if err != nil { return err } - original.Enabled = !amend.Disabled - - switch { - case enabledBefore && !original.Enabled: - if d.isWatchpoint(original) { - return errors.New("can not disable watchpoints") - } - err = d.target.DisableBreakpoint(original) - case !enabledBefore && original.Enabled: - err = d.target.EnableBreakpoint(original) - } + err = d.target.SetBreakpointEnabled(original, !amend.Disabled) if err != nil { return err } - t := proc.ValidTargets{Group: d.target} - for t.Next() { - for _, bp := range t.Breakpoints().M { - if bp.LogicalID() == amend.ID { - bp.UserBreaklet().Cond = original.Cond - } - } - } return nil } @@ -907,7 +888,7 @@ func (d *Debugger) CancelNext() error { return d.target.ClearSteppingBreakpoints() } -func copyLogicalBreakpointInfo(lbp *proc.LogicalBreakpoint, requested *api.Breakpoint) error { +func (d *Debugger) copyLogicalBreakpointInfo(lbp *proc.LogicalBreakpoint, requested *api.Breakpoint) error { lbp.Name = requested.Name lbp.Tracepoint = requested.Tracepoint lbp.TraceReturn = requested.TraceReturn @@ -919,70 +900,8 @@ func copyLogicalBreakpointInfo(lbp *proc.LogicalBreakpoint, requested *api.Break lbp.UserData = requested.UserData lbp.RootFuncName = requested.RootFuncName lbp.TraceFollowCalls = requested.TraceFollowCalls - lbp.Cond = nil - if requested.Cond != "" { - var err error - lbp.Cond, err = parser.ParseExpr(requested.Cond) - if err != nil { - return err - } - } - lbp.HitCond = nil - if requested.HitCond != "" { - opTok, val, err := parseHitCondition(requested.HitCond) - if err != nil { - return err - } - lbp.HitCond = &struct { - Op token.Token - Val int - }{opTok, val} - lbp.HitCondPerG = requested.HitCondPerG - } - - return nil -} - -func parseHitCondition(hitCond string) (token.Token, int, error) { - // A hit condition can be in the following formats: - // - "number" - // - "OP number" - hitConditionRegex := regexp.MustCompile(`(([=><%!])+|)( |)((\d|_)+)`) - - match := hitConditionRegex.FindStringSubmatch(strings.TrimSpace(hitCond)) - if match == nil || len(match) != 6 { - return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\nhit conditions should be of the form \"number\" or \"OP number\"", hitCond) - } - - opStr := match[1] - var opTok token.Token - switch opStr { - case "==", "": - opTok = token.EQL - case ">=": - opTok = token.GEQ - case "<=": - opTok = token.LEQ - case ">": - opTok = token.GTR - case "<": - opTok = token.LSS - case "%": - opTok = token.REM - case "!=": - opTok = token.NEQ - default: - return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\ninvalid operator: %q", hitCond, opStr) - } - - numStr := match[4] - val, parseErr := strconv.Atoi(numStr) - if parseErr != nil { - return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\ninvalid number: %q", hitCond, numStr) - } - - return opTok, val, nil + return d.target.ChangeBreakpointCondition(lbp, requested.Cond, requested.HitCond, requested.HitCondPerG) } // ClearBreakpoint clears a breakpoint. @@ -1000,7 +919,7 @@ func (d *Debugger) ClearBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint lbp := d.target.LogicalBreakpoints[requestedBp.ID] clearedBp := d.convertBreakpoint(lbp) - err := d.target.DisableBreakpoint(lbp) + err := d.target.SetBreakpointEnabled(lbp, false) if err != nil { return nil, err } @@ -1011,33 +930,6 @@ 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() @@ -1337,10 +1229,6 @@ 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) - } d.maybePrintUnattendedStopWarning(d.target.Selected.StopReason, state.CurrentThread, clientStatusCh) return state, err diff --git a/service/test/integration2_test.go b/service/test/integration2_test.go index 8982442a..8a6c87b0 100644 --- a/service/test/integration2_test.go +++ b/service/test/integration2_test.go @@ -625,66 +625,46 @@ func TestClientServer_disableHitCondLSSBreakpoint(t *testing.T) { Line: 7, HitCond: "< 3", }) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + assertNoError(err, t, "CreateBreakpoint") + bp, err := c.CreateBreakpoint(&api.Breakpoint{File: fp, Line: 8}) + assertNoError(err, t, "CreateBreakpoint") + + if len(bp.Addrs) == 0 { + t.Fatalf("no addresses for breakpoint") } - state := <-c.Continue() - if state.Err != nil { - t.Fatalf("Unexpected error: %v, state: %#v", state.Err, state) + continueTo := func(ln int, ival string) { + state := <-c.Continue() + assertNoError(state.Err, t, fmt.Sprintf("Unexpected error: %v, state: %#v", state.Err, state)) + + f, l := state.CurrentThread.File, state.CurrentThread.Line + if f != fp || l != ln { + t.Fatalf("Program did not hit breakpoint %s:%d", f, l) + } + + if ival == "" { + return + } + + ivar, err := c.EvalVariable(api.EvalScope{GoroutineID: -1}, "i", normalLoadConfig) + assertNoError(err, t, "EvalVariable") + + t.Logf("ivar: %s", ivar.SinglelineString()) + + if ivar.Value != ival { + t.Fatalf("Wrong variable value: %s", ivar.Value) + } } - 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) - } + continueTo(7, "1") + continueTo(7, "2") + continueTo(8, "") 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, - ) + if len(bp.Addrs) != 0 { + t.Fatalf("Hit condition %s is no longer satisfiable but breakpoint has not been disabled", bp.HitCond) } }) } @@ -697,17 +677,19 @@ func TestClientServer_disableHitEQLCondBreakpoint(t *testing.T) { Line: 7, HitCond: "== 3", }) - if err != nil { - t.Fatalf("Unexpected error: %v", err) + assertNoError(err, t, "CreateBreakpoint") + bp, err := c.CreateBreakpoint(&api.Breakpoint{File: fp, Line: 8}) + assertNoError(err, t, "CreateBreakpoint") + + if len(bp.Addrs) == 0 { + t.Fatalf("no addresses for breakpoint") } state := <-c.Continue() - if state.Err != nil { - t.Fatalf("Unexpected error: %v, state: %#v", state.Err, state) - } + assertNoError(state.Err, t, "Continue") f, l := state.CurrentThread.File, state.CurrentThread.Line - if f != "break.go" && l != 7 { + if f != fp || l != 7 { t.Fatal("Program did not hit breakpoint") } @@ -720,14 +702,18 @@ func TestClientServer_disableHitEQLCondBreakpoint(t *testing.T) { t.Fatalf("Wrong variable value: %s", ivar.Value) } - bp, err := c.GetBreakpoint(hitCondBp.ID) + state = <-c.Continue() + assertNoError(state.Err, t, "Continue") + + if state.CurrentThread.File != fp || state.CurrentThread.Line != 8 { + t.Fatal("Program did not hit breakpoint") + } + + 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, - ) + if len(bp.Addrs) != 0 { + t.Fatalf("Hit condition %s is no more satisfiable but breakpoint has not been disabled", bp.HitCond) } }) }