proc: fix bug with range-over-func stepping (#3778)

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

Author: aarzilli

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
@@ -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))
 		}
@@ -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)
 	}
@@ -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

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
 }
 

pkg/proc/proc_test.go

@@ -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,
@@ -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()
@@ -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),
@@ -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)

pkg/proc/stack.go

@@ -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
@@ -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:
 //
@@ -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.
@@ -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
@@ -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 {
@@ -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
@@ -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
 }

pkg/proc/target_exec.go

@@ -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 {
@@ -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 {
@@ -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
 				}
@@ -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]
 	}
 
@@ -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
+		}
+	}
+}