From a97da22762ac08478db61373a7efe372f84d2d79 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Wed, 29 Sep 2021 12:01:37 +0200 Subject: [PATCH] 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 | 59 +++++++++++++++++++++++------------ pkg/proc/gdbserial/rr_test.go | 4 +-- pkg/proc/proc_test.go | 28 ++++++++--------- pkg/proc/scope_test.go | 2 +- pkg/proc/stackwatch.go | 2 +- pkg/proc/target.go | 2 +- service/api/conversions.go | 6 ++-- service/debugger/debugger.go | 35 ++++++++++----------- 8 files changed, 78 insertions(+), 60 deletions(-) diff --git a/pkg/proc/breakpoints.go b/pkg/proc/breakpoints.go index 929ea086..c5b9bfe4 100644 --- a/pkg/proc/breakpoints.go +++ b/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 @@ -448,8 +460,7 @@ type BreakpointMap struct { // the last resume operation WatchOutOfScope []*Breakpoint - breakpointIDCounter int - internalBreakpointIDCounter int + breakpointIDCounter 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 - bpmap.breakpointIDCounter-- + 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, diff --git a/pkg/proc/gdbserial/rr_test.go b/pkg/proc/gdbserial/rr_test.go index 65eb340a..66dc44bf 100644 --- a/pkg/proc/gdbserial/rr_test.go +++ b/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)") diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index d55e45d5..f7ff3a25 100644 --- a/pkg/proc/proc_test.go +++ b/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 { diff --git a/pkg/proc/scope_test.go b/pkg/proc/scope_test.go index df371338..03a4cf0e 100644 --- a/pkg/proc/scope_test.go +++ b/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") } }) diff --git a/pkg/proc/stackwatch.go b/pkg/proc/stackwatch.go index 2f1d1ca5..9b671f77 100644 --- a/pkg/proc/stackwatch.go +++ b/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) diff --git a/pkg/proc/target.go b/pkg/proc/target.go index ba026ed6..89d68d19 100644 --- a/pkg/proc/target.go +++ b/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 } diff --git a/service/api/conversions.go b/service/api/conversions.go index c79ef8d6..5d258326 100644 --- a/service/api/conversions.go +++ b/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") } } diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go index 678fad13..2562d672 100644 --- a/service/debugger/debugger.go +++ b/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() }