From a68f8a26cf4aee77e1922eb9ee5aa1341108d3ee Mon Sep 17 00:00:00 2001 From: aarzilli Date: Tue, 18 Jun 2024 16:06:08 +0200 Subject: [PATCH] proc: fix bug with range-over-func stepping 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 #3733 --- pkg/proc/breakpoints.go | 8 +++++- pkg/proc/eval.go | 10 ++----- pkg/proc/proc_test.go | 64 ++++++++++++++--------------------------- pkg/proc/stack.go | 59 +++++++++++++++++++++++++------------ pkg/proc/target_exec.go | 49 +++++++++++++++++++++++++++++-- 5 files changed, 119 insertions(+), 71 deletions(-) diff --git a/pkg/proc/breakpoints.go b/pkg/proc/breakpoints.go index ff9ff9fbaa..a9204c1152 100644 --- a/pkg/proc/breakpoints.go +++ b/pkg/proc/breakpoints.go @@ -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 @@ -318,6 +321,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) } diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index 743b249d4d..ce0cee19d5 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -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) @@ -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 } diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index ddcb309ed5..897c138d07 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -6300,26 +6300,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, @@ -6662,20 +6642,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() @@ -6776,14 +6756,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), @@ -6812,14 +6792,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) diff --git a/pkg/proc/stack.go b/pkg/proc/stack.go index 17988dce60..9021c3322a 100644 --- a/pkg/proc/stack.go +++ b/pkg/proc/stack.go @@ -691,7 +691,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 @@ -901,8 +901,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: // @@ -917,9 +917,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. @@ -932,7 +934,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 @@ -942,6 +954,7 @@ func rangeFuncStackTrace(tgt *Target, g *G) ([]Stackframe, error) { if fr.closurePtr != 0 { closurePtr = fr.closurePtr } + addretframe = true } closureptrok := func(fr *Stackframe) bool { @@ -977,33 +990,40 @@ func rangeFuncStackTrace(tgt *Target, g *G) ([]Stackframe, error) { } } + if addretframe { + addretframe = false + frames = append(frames, fr) + } + switch stage { - case 0: + case startStage: ap(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) { - ap(fr) - stage++ + frames = append(frames, fr) + stage = lastFrameStage } else if fr.Call.Fn.extra(tgt.BinInfo()).rangeParent == rangeParent && closureptrok(&fr) { ap(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 @@ -1014,9 +1034,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 } diff --git a/pkg/proc/target_exec.go b/pkg/proc/target_exec.go index 2fde475a67..5813002147 100644 --- a/pkg/proc/target_exec.go +++ b/pkg/proc/target_exec.go @@ -847,8 +847,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 { @@ -857,7 +859,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 } @@ -866,6 +868,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] } @@ -1657,3 +1677,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 + } + } +}