Skip to content

Commit

Permalink
proc: fix bug with range-over-func stepping
Browse files Browse the repository at this point in the history 
Set a breakpoint on the return address of the current function, if it's
a range-over-func body, and clear the stepping breakpoints for the
current function (except the entry one) when its hit.

Without this what can happen is the following:

1. the range-over-func body finishes and returns to the iterator
2. the iterator calls back into the range-over-func body
3. a stepping breakpoint that's inside the prologue gets hit

Updates go-delve#3733
  • Loading branch information
@aarzilli
aarzilli committed Jul 12, 2024
1 parent abad6bb commit f0e2558
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 70 deletions.
33 changes: 32 additions & 1 deletion pkg/proc/breakpoints.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,10 @@ const (
// goroutine.
StepIntoNewProcBreakpoint

steppingMask = NextBreakpoint | NextDeferBreakpoint | StepBreakpoint | StepIntoNewProcBreakpoint
// NextInactivatedBreakpoint a NextBreakpoint that has been inactivated, see rangeFrameInactivateNextBreakpoints
NextInactivatedBreakpoint

steppingMask = NextBreakpoint | NextDeferBreakpoint | StepBreakpoint | StepIntoNewProcBreakpoint | NextInactivatedBreakpoint
)

// WatchType is the watchpoint type
Expand Down Expand Up @@ -221,6 +224,8 @@ func (bp *Breakpoint) VerboseDescr() []string {
r = append(r, "PluginOpenBreakpoint")
case StepIntoNewProcBreakpoint:
r = append(r, "StepIntoNewProcBreakpoint")
case NextInactivatedBreakpoint:
r = append(r, "NextInactivatedBreakpoint")
default:
r = append(r, fmt.Sprintf("Unknown %d", breaklet.Kind))
}
Expand Down Expand Up @@ -318,6 +323,9 @@ func (bpstate *BreakpointState) checkCond(tgt *Target, breaklet *Breaklet, threa
case StackResizeBreakpoint, PluginOpenBreakpoint, StepIntoNewProcBreakpoint:
// no further checks

case NextInactivatedBreakpoint:
active = false

default:
bpstate.CondError = fmt.Errorf("internal error unknown breakpoint kind %v", breaklet.Kind)
}
Expand Down Expand Up @@ -834,6 +842,29 @@ func (t *Target) ClearSteppingBreakpoints() error {
return nil
}

func (t *Target) clearInactivatedSteppingBreakpoint() error {
threads := t.ThreadList()
for _, bp := range t.Breakpoints().M {
for i := range bp.Breaklets {
if bp.Breaklets[i].Kind == NextInactivatedBreakpoint {
bp.Breaklets[i] = nil
}
}
cleared, err := t.finishClearBreakpoint(bp)
if err != nil {
return err
}
if cleared {
for _, thread := range threads {
if thread.Breakpoint().Breakpoint == bp {
thread.Breakpoint().Clear()
}
}
}
}
return nil
}

// finishClearBreakpoint clears nil breaklets from the breaklet list of bp
// and if it is empty erases the breakpoint.
// Returns true if the breakpoint was deleted
Expand Down
10 changes: 3 additions & 7 deletions pkg/proc/eval.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,12 +340,8 @@ func (scope *EvalScope) Locals(flags localsFlags, wantedName string) ([]*Variabl
}
}
for i, scope2 := range scope.enclosingRangeScopes {
if i == len(scope.enclosingRangeScopes)-1 {
// Last one is the caller frame, we shouldn't check it
break
}
if scope2 == nil {
scope2 = FrameToScope(scope.target, scope.target.Memory(), scope.g, scope.threadID, scope.rangeFrames[i:]...)
scope2 = FrameToScope(scope.target, scope.target.Memory(), scope.g, scope.threadID, scope.rangeFrames[2*i:]...)
scope.enclosingRangeScopes[i] = scope2
}
vars, err := scope2.simpleLocals(flags, wantedName)
Expand Down Expand Up @@ -381,8 +377,8 @@ func (scope *EvalScope) setupRangeFrames() error {
if err != nil {
return err
}
scope.rangeFrames = scope.rangeFrames[1:]
scope.enclosingRangeScopes = make([]*EvalScope, len(scope.rangeFrames))
scope.rangeFrames = scope.rangeFrames[2:] // skip the first frame and its return frame
scope.enclosingRangeScopes = make([]*EvalScope, len(scope.rangeFrames)/2)
return nil
}

Expand Down
63 changes: 22 additions & 41 deletions pkg/proc/proc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6299,25 +6299,6 @@ func TestRangeOverFuncNext(t *testing.T) {
return seqTest{contNext, n}
}

nx2 := func(t *testing.T, n int) seqTest {
return seqTest{contNothing, func(grp *proc.TargetGroup, p *proc.Target) {
_, ln1 := currentLineNumber(p, t)
assertNoError(grp.Next(), t, "Next() returned an error")
f, ln2 := currentLineNumber(p, t)
if ln2 == n {
return
}
if ln2 != ln1 {
t.Fatalf("Program did not continue to correct next location (expected %d or %d) was %s:%d", ln1, n, f, ln2)
}
assertNoError(grp.Next(), t, "Next() returned an error")
f, ln2 = currentLineNumber(p, t)
if ln2 != n {
t.Fatalf("Program did not continue to correct next location (expected %d) was %s:%d", n, f, ln2)
}
}}
}

assertLocals := func(t *testing.T, varnames ...string) seqTest {
return seqTest{
contNothing,
Expand Down Expand Up @@ -6659,20 +6640,20 @@ func TestRangeOverFuncNext(t *testing.T) {
nx(118), // if z == 4
nx(121),

nx(116), // for _, z := range (z == 2)
nx2(t, 117), // result = append(result, z)
nx(118), // if z == 4
nx(116), // for _, z := range (z == 2)
nx(117), // result = append(result, z)
nx(118), // if z == 4
nx(121),

nx(116), // for _, z := range (z == 3)
nx2(t, 117), // result = append(result, z)
nx(118), // if z == 4
nx(116), // for _, z := range (z == 3)
nx(117), // result = append(result, z)
nx(118), // if z == 4
nx(121),

nx(116), // for _, z := range (z == 4)
nx2(t, 117), // result = append(result, z)
nx(118), // if z == 4
nx(119), // break
nx(116), // for _, z := range (z == 4)
nx(117), // result = append(result, z)
nx(118), // if z == 4
nx(119), // break

nx(112), // defer func()
nx(113), // r := recover()
Expand Down Expand Up @@ -6773,14 +6754,14 @@ func TestRangeOverFuncNext(t *testing.T) {
nx(203), // result = append(result, y)
nx(204),

nx(199), // for _, y := range (y == 2)
nx2(t, 200), // if y == 3
nx(203), // result = append(result, y)
nx(199), // for _, y := range (y == 2)
nx(200), // if y == 3
nx(203), // result = append(result, y)
nx(204),

nx(199), // for _, y := range (y == 3)
nx2(t, 200), // if y == 3
nx(201), // goto A
nx(199), // for _, y := range (y == 3)
nx(200), // if y == 3
nx(201), // goto A
nx(204),
nx(206), // result = append(result, x)
nx(207),
Expand Down Expand Up @@ -6809,14 +6790,14 @@ func TestRangeOverFuncNext(t *testing.T) {
nx(222), // result = append(result, y)
nx(223),

nx(218), // for _, y := range (y == 2)
nx2(t, 219), // if y == 3
nx(222), // result = append(result, y)
nx(218), // for _, y := range (y == 2)
nx(219), // if y == 3
nx(222), // result = append(result, y)
nx(223),

nx(218), // for _, y := range (y == 3)
nx2(t, 219), // if y == 3
nx(220), // goto B
nx(218), // for _, y := range (y == 3)
nx(219), // if y == 3
nx(220), // goto B
nx(223),
nx(225),
nx(227), // result = append(result, 999)
Expand Down
59 changes: 41 additions & 18 deletions pkg/proc/stack.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ func (g *G) readDefers(frames []Stackframe) {
frames[i].TopmostDefer = curdefer.topdefer()
}

if frames[i].SystemStack || curdefer.SP >= uint64(frames[i].Regs.CFA) {
if frames[i].SystemStack || frames[i].Inlined || curdefer.SP >= uint64(frames[i].Regs.CFA) {
// frames[i].Regs.CFA is the value that SP had before the function of
// frames[i] was called.
// This means that when curdefer.SP == frames[i].Regs.CFA then curdefer
Expand Down Expand Up @@ -900,8 +900,8 @@ func ruleString(rule *frame.DWRule, regnumToString func(uint64) string) string {

// rangeFuncStackTrace, if the topmost frame of the stack is a the body of a
// range-over-func statement, returns a slice containing the stack of range
// bodies on the stack, the frame of the function containing them and
// finally the function that called it.
// bodies on the stack, interleaved with their return frames, the frame of
// the function containing them and finally the function that called it.
//
// For example, given:
//
Expand All @@ -916,9 +916,11 @@ func ruleString(rule *frame.DWRule, regnumToString func(uint64) string) string {
// It will return the following frames:
//
// 0. f-range2()
// 1. f-range1()
// 2. f()
// 3. function that called f()
// 1. function that called f-range2
// 2. f-range1()
// 3. function that called f-range1
// 4. f()
// 5. function that called f()
//
// If the topmost frame of the stack is *not* the body closure of a
// range-over-func statement then nothing is returned.
Expand All @@ -931,7 +933,17 @@ func rangeFuncStackTrace(tgt *Target, g *G) ([]Stackframe, error) {
return nil, err
}
frames := []Stackframe{}
stage := 0

const (
startStage = iota
normalStage
lastFrameStage
doneStage
)

stage := startStage
addRetFrame := false

var rangeParent *Function
nonMonotonicSP := false
var closurePtr int64
Expand All @@ -941,6 +953,7 @@ func rangeFuncStackTrace(tgt *Target, g *G) ([]Stackframe, error) {
if fr.closurePtr != 0 {
closurePtr = fr.closurePtr
}
addRetFrame = true
}

closurePtrOk := func(fr *Stackframe) bool {
Expand Down Expand Up @@ -976,33 +989,40 @@ func rangeFuncStackTrace(tgt *Target, g *G) ([]Stackframe, error) {
}
}

if addRetFrame {
addRetFrame = false
frames = append(frames, fr)
}

switch stage {
case 0:
case startStage:
appendFrame(fr)
rangeParent = fr.Call.Fn.extra(tgt.BinInfo()).rangeParent
stage++
stage = normalStage
if rangeParent == nil || closurePtr == 0 {
frames = nil
stage = 3
addRetFrame = false
stage = doneStage
return false
}
case 1:
case normalStage:
if fr.Call.Fn.offset == rangeParent.offset && closurePtrOk(&fr) {
appendFrame(fr)
stage++
frames = append(frames, fr)
stage = lastFrameStage
} else if fr.Call.Fn.extra(tgt.BinInfo()).rangeParent == rangeParent && closurePtrOk(&fr) {
appendFrame(fr)
if closurePtr == 0 {
frames = nil
stage = 3
addRetFrame = false
stage = doneStage
return false
}
}
case 2:
case lastFrameStage:
frames = append(frames, fr)
stage++
stage = doneStage
return false
case 3:
case doneStage:
return false
}
return true
Expand All @@ -1013,9 +1033,12 @@ func rangeFuncStackTrace(tgt *Target, g *G) ([]Stackframe, error) {
if nonMonotonicSP {
return nil, errors.New("corrupted stack (SP not monotonically decreasing)")
}
if stage != 3 {
if stage != doneStage {
return nil, errors.New("could not find range-over-func closure parent on the stack")
}
if len(frames)%2 != 0 {
return nil, errors.New("incomplete range-over-func stacktrace")
}
g.readDefers(frames)
return frames, nil
}
54 changes: 51 additions & 3 deletions pkg/proc/target_exec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ func (grp *TargetGroup) Continue() error {
}
delete(it.Breakpoints().Logical, watchpoint.LogicalID())
}
// Clear inactivated breakpoints
err := it.clearInactivatedSteppingBreakpoint()
if err != nil {
logflags.DebuggerLogger().Errorf("could not clear inactivated stepping breakpoints: %v", err)
}
}

if contOnceErr != nil {
Expand Down Expand Up @@ -847,8 +852,10 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error {

// Set step-out breakpoints for range-over-func body closures
if !stepInto && selg != nil && topframe.Current.Fn.extra(bi).rangeParent != nil && len(rangeFrames) > 0 {
for _, fr := range rangeFrames[:len(rangeFrames)-1] {
retframecond := astutil.And(sameGCond, frameoffCondition(&fr))
// Set step-out breakpoint for every range-over-func body currently on the stack so that we stop on them.
for i := 2; i < len(rangeFrames); i += 2 {
fr := &rangeFrames[i]
retframecond := astutil.And(sameGCond, frameoffCondition(fr))
if !fr.hasInlines {
dbp.SetBreakpoint(0, fr.Current.PC, NextBreakpoint, retframecond)
} else {
Expand All @@ -857,7 +864,7 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error {
if err != nil {
return err
}
pcs, err = removeInlinedCalls(pcs, &fr, bi)
pcs, err = removeInlinedCalls(pcs, fr, bi)
if err != nil {
return err
}
Expand All @@ -866,6 +873,24 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error {
}
}
}

// Set a step-out breakpoint for the first range-over-func body on the
// stack, this breakpoint will never cause a stop because the associated
// callback always returns false.
// Its purpose is to inactivate all the breakpoints for the current
// range-over-func body function so that if the iterator re-calls it we
// don't end up inside the prologue.
if !rangeFrames[0].Inlined {
bp, err := dbp.SetBreakpoint(0, rangeFrames[1].Call.PC, NextBreakpoint, astutil.And(sameGCond, frameoffCondition(&rangeFrames[1])))
if err == nil {
bplet := bp.Breaklets[len(bp.Breaklets)-1]
bplet.callback = func(th Thread, p *Target) (bool, error) {
rangeFrameInactivateNextBreakpoints(p, rangeFrames[0].Call.Fn)
return false, nil
}
}
}

topframe, retframe = rangeFrames[len(rangeFrames)-2], rangeFrames[len(rangeFrames)-1]
}

Expand Down Expand Up @@ -1657,3 +1682,26 @@ func (t *Target) handleHardcodedBreakpoints(grp *TargetGroup, trapthread Thread,
}
return nil
}

func rangeFrameInactivateNextBreakpoints(p *Target, fn *Function) {
pc, err := FirstPCAfterPrologue(p, fn, false)
if err != nil {
logflags.DebuggerLogger().Errorf("Error inactivating next breakpoints after exiting a range-over-func body: %v", err)
return
}

for _, bp := range p.Breakpoints().M {
if bp.Addr < fn.Entry || bp.Addr >= fn.End || bp.Addr == pc {
continue
}
for _, bplet := range bp.Breaklets {
if bplet.Kind != NextBreakpoint {
continue
}
// We set to NextInactivatedBreakpoint instead of deleting them because
// we can't delete breakpoints (or breakpointlets) while breakpoint
// conditions are being evaluated.
bplet.Kind = NextInactivatedBreakpoint
}
}
}

0 comments on commit f0e2558

Please sign in to comment.