proc: step breakpoints shouldn't hide normal breakpoints (#3287)

When using Step on a function that has a dynamic CALL instruction we
set a Step breakpoint on the call.
When it is hit we determine the destination of the CALL by looking at
registers, set a breakpoint there and continue.
If the Step breakpoint is hit simultaneously with a normal breakpoint
our Step logic will take precedence and the normal breakpoint hit will
be hidden from the user.

Move the Step logic to a breaklet callback so that it does not
interfere with the decision to stop.

_fixtures/stepshadow.go

@@ -0,0 +1,31 @@
+package main
+
+import (
+	"fmt"
+	"sync"
+)
+
+func main() {
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go goroutineA(&wg)
+	f := stacktraceme1
+	for i := 0; i < 100; i++ {
+		fmt.Printf("main %d\n", i)
+		f()
+	}
+	wg.Wait()
+}
+
+func goroutineA(wg *sync.WaitGroup) {
+	defer wg.Done()
+	for i := 0; i < 100; i++ {
+		stacktraceme2()
+	}
+}
+
+func stacktraceme1() {
+}
+
+func stacktraceme2() {
+}

pkg/proc/breakpoints.go

@@ -96,7 +96,7 @@ type Breaklet struct {
 	// the return value will determine if the breaklet should be considered
 	// active.
 	// The callback can have side-effects.
-	callback func(th Thread) bool
+	callback func(th Thread, p *Target) (bool, error)
 
 	// For WatchOutOfScopeBreakpoints and StackResizeBreakpoints the watchpoint
 	// field contains the watchpoint related to this out of scope sentinel.
@@ -294,12 +294,6 @@ func (bpstate *BreakpointState) checkCond(tgt *Target, breaklet *Breaklet, threa
 			}
 		}
 		active = active && nextDeferOk
-		if active {
-			bpstate.Stepping = true
-			if breaklet.Kind == StepBreakpoint {
-				bpstate.SteppingInto = true
-			}
-		}
 
 	case WatchOutOfScopeBreakpoint:
 		if breaklet.checkPanicCall {
@@ -319,10 +313,24 @@ func (bpstate *BreakpointState) checkCond(tgt *Target, breaklet *Breaklet, threa
 
 	if active {
 		if breaklet.callback != nil {
-			active = breaklet.callback(thread)
+			var err error
+			active, err = breaklet.callback(thread, tgt)
+			if err != nil && bpstate.CondError == nil {
+				bpstate.CondError = err
+			}
 		}
 		bpstate.Active = active
 	}
+
+	if bpstate.Active {
+		switch breaklet.Kind {
+		case NextBreakpoint, NextDeferBreakpoint:
+			bpstate.Stepping = true
+		case StepBreakpoint:
+			bpstate.Stepping = true
+			bpstate.SteppingInto = true
+		}
+	}
 }
 
 // checkHitCond evaluates bp's hit condition on thread.

pkg/proc/proc_test.go

@@ -6057,3 +6057,63 @@ func TestEscapeCheckUnreadable(t *testing.T) {
 		proc.EvalExpressionWithCalls(grp, p.SelectedGoroutine(), "value.Type()", normalLoadConfig, true)
 	})
 }
+
+func TestStepShadowConcurrentBreakpoint(t *testing.T) {
+	// Checks that a StepBreakpoint can not shadow a concurrently hit user breakpoint
+	withTestProcess("stepshadow", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
+		break2 := setFunctionBreakpoint(p, t, "main.stacktraceme2")
+		breakMain := setFileBreakpoint(p, t, fixture.Source, 15)
+		assertNoError(grp.Continue(), t, "Continue()")
+
+		stacktraceme1calls, stacktraceme2calls := 0, 0
+
+		for {
+			t.Logf("stop (%d %d):", stacktraceme1calls, stacktraceme2calls)
+			for _, th := range p.ThreadList() {
+				loc, _ := th.Location()
+				t.Logf("\t%s:%d\n", loc.File, loc.Line)
+				bp := th.Breakpoint().Breakpoint
+				if bp != nil && bp.Addr == break2.Addr {
+					stacktraceme2calls++
+				}
+				// make stop on the breakpoint in main.main the selected goroutine so we can use step later
+				if bp != nil && bp.Addr == breakMain.Addr {
+					g, _ := proc.GetG(th)
+					p.SwitchGoroutine(g)
+				}
+			}
+
+			file, lineno := currentLineNumber(p, t)
+
+			var err error
+			var reason string
+			switch lineno {
+			default:
+				t.Fatalf("unexpected stop location %s:%d", file, lineno)
+			case 15: // loop in main.main
+				reason = "Step()"
+				err = grp.Step()
+			case 28: // main.stacktraceme1
+				stacktraceme1calls++
+				reason = "Continue()"
+				err = grp.Continue()
+			case 30, 31: // main.stacktraceme2
+				reason = "Continue()"
+				err = grp.Continue()
+			}
+			if _, isexited := err.(proc.ErrProcessExited); isexited {
+				break
+			}
+			assertNoError(err, t, reason)
+		}
+
+		t.Logf("%d %d\n", stacktraceme1calls, stacktraceme2calls)
+
+		if stacktraceme1calls != 100 {
+			t.Errorf("wrong number of calls to stacktraceme1 found: %d", stacktraceme1calls)
+		}
+		if stacktraceme2calls != 100 {
+			t.Errorf("wrong number of calls to stacktraceme2 found: %d", stacktraceme2calls)
+		}
+	})
+}

pkg/proc/stackwatch.go

@@ -28,9 +28,9 @@ import (
 func (t *Target) setStackWatchBreakpoints(scope *EvalScope, watchpoint *Breakpoint) error {
 	// Watchpoint Out-of-scope Sentinel
 
-	woos := func(_ Thread) bool {
+	woos := func(_ Thread, _ *Target) (bool, error) {
 		watchpointOutOfScope(t, watchpoint)
-		return true
+		return true, nil
 	}
 
 	topframe, retframe, err := topframe(scope.g, nil)
@@ -111,9 +111,9 @@ func (t *Target) setStackWatchBreakpoints(scope *EvalScope, watchpoint *Breakpoi
 
 	rszbreaklet := rszbp.Breaklets[len(rszbp.Breaklets)-1]
 	rszbreaklet.watchpoint = watchpoint
-	rszbreaklet.callback = func(th Thread) bool {
+	rszbreaklet.callback = func(th Thread, _ *Target) (bool, error) {
 		adjustStackWatchpoint(t, th, watchpoint)
-		return false // we never want this breakpoint to be shown to the user
+		return false, nil // we never want this breakpoint to be shown to the user
 	}
 
 	return nil

pkg/proc/target.go

@@ -587,7 +587,7 @@ func (t *Target) dwrapUnwrap(fn *Function) *Function {
 	return fn
 }
 
-func (t *Target) pluginOpenCallback(Thread) bool {
+func (t *Target) pluginOpenCallback(Thread, *Target) (bool, error) {
 	logger := logflags.DebuggerLogger()
 	for _, lbp := range t.Breakpoints().Logical {
 		if isSuspended(t, lbp) {
@@ -599,7 +599,7 @@ func (t *Target) pluginOpenCallback(Thread) bool {
 			}
 		}
 	}
-	return false
+	return false, nil
 }
 
 func isSuspended(t *Target, lbp *LogicalBreakpoint) bool {

pkg/proc/target_exec.go

@@ -173,22 +173,7 @@ func (grp *TargetGroup) Continue() error {
 				if err := conditionErrors(grp); err != nil {
 					return err
 				}
-				if grp.GetDirection() == Forward {
-					text, err := disassembleCurrentInstruction(dbp, curthread, 0)
-					if err != nil {
-						return err
-					}
-					var fn *Function
-					if loc, _ := curthread.Location(); loc != nil {
-						fn = loc.Fn
-					}
-					// here we either set a breakpoint into the destination of the CALL
-					// instruction or we determined that the called function is hidden,
-					// either way we need to resume execution
-					if err = setStepIntoBreakpoint(dbp, fn, text, sameGoroutineCondition(dbp.SelectedGoroutine())); err != nil {
-						return err
-					}
-				} else {
+				if grp.GetDirection() == Backward {
 					if err := dbp.ClearSteppingBreakpoints(); err != nil {
 						return err
 					}
@@ -797,14 +782,47 @@ func setStepIntoBreakpoints(dbp *Target, curfn *Function, text []AsmInstruction,
 			}
 		} else {
 			// Non-absolute call instruction, set a StepBreakpoint here
-			if _, err := allowDuplicateBreakpoint(dbp.SetBreakpoint(0, instr.Loc.PC, StepBreakpoint, sameGCond)); err != nil {
+			bp, err := allowDuplicateBreakpoint(dbp.SetBreakpoint(0, instr.Loc.PC, StepBreakpoint, sameGCond))
+			if err != nil {
 				return err
 			}
+			breaklet := bp.Breaklets[len(bp.Breaklets)-1]
+			breaklet.callback = stepIntoCallback
 		}
 	}
 	return nil
 }
 
+// stepIntoCallback is a callback called when a StepBreakpoint is hit, it
+// disassembles the current instruction to figure out its destination and
+// sets a breakpoint on it.
+func stepIntoCallback(curthread Thread, p *Target) (bool, error) {
+	if p.recman.GetDirection() != Forward {
+		// This should never happen, step into breakpoints with callbacks are only
+		// set when moving forward and direction changes are forbidden while
+		// breakpoints are set.
+		return true, nil
+	}
+
+	text, err := disassembleCurrentInstruction(p, curthread, 0)
+	if err != nil {
+		return false, err
+	}
+	var fn *Function
+	if loc, _ := curthread.Location(); loc != nil {
+		fn = loc.Fn
+	}
+	g, _ := GetG(curthread)
+	// here we either set a breakpoint into the destination of the CALL
+	// instruction or we determined that the called function is hidden,
+	// either way we need to resume execution
+	if err = setStepIntoBreakpoint(p, fn, text, sameGoroutineCondition(g)); err != nil {
+		return false, err
+	}
+
+	return false, nil
+}
+
 func setStepIntoBreakpointsReverse(dbp *Target, text []AsmInstruction, topframe Stackframe, sameGCond ast.Expr) error {
 	bpmap := dbp.Breakpoints()
 	// Set a breakpoint after every CALL instruction