proc: better handling of hardcoded breakpoints (#2852)

This commit improves the handling of hardcoded breakpoints in Delve.
A hardcoded breakpoint is a breakpoint instruction hardcoded in the
text of the program, for example through runtime.Breakpoint.

1. hardcoded breakpoints are now indicated by setting the breakpoint
   field on any thread stopped by a hardcoded breakpoint
2. if multiple hardcoded breakpoints are hit during a single stop all
   will be notified to the user.
3. a debugger breakpoint with an unmet condition can't hide a hardcoded
   breakpoint anymore.

Documentation/backend_test_health.md

@@ -11,8 +11,8 @@ Tests skipped by each supported backend:
 	* 1 broken - cgo stacktraces
 * darwin/lldb skipped = 1
 	* 1 upstream issue
-* freebsd skipped = 15
-	* 11 broken
+* freebsd skipped = 16
+	* 12 broken
 	* 4 not implemented
 * linux/386/pie skipped = 1
 	* 1 broken

_fixtures/hcbpcountstest.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"fmt"
+	"math/rand"
+	"runtime"
+	"sync"
+	"time"
+)
+
+func demo(id int, wait *sync.WaitGroup) {
+	for i := 0; i < 100; i++ {
+		sleep := rand.Intn(10) + 1
+		runtime.Breakpoint()
+		fmt.Printf("id: %d step: %d sleeping %d\n", id, i, sleep)
+		time.Sleep(time.Duration(sleep) * time.Millisecond)
+	}
+
+	wait.Done()
+}
+
+func main() {
+	wait := new(sync.WaitGroup)
+	wait.Add(1)
+	wait.Add(1)
+	go demo(1, wait)
+	go demo(2, wait)
+
+	wait.Wait()
+}

pkg/proc/breakpoints.go

@@ -25,8 +25,13 @@ const (
 	// process dies because of a fatal runtime error.
 	FatalThrow = "runtime-fatal-throw"
 
-	unrecoveredPanicID = -1
-	fatalThrowID       = -2
+	// HardcodedBreakpoint is the name given to hardcoded breakpoints (for
+	// example: calls to runtime.Breakpoint)
+	HardcodedBreakpoint = "hardcoded-breakpoint"
+
+	unrecoveredPanicID    = -1
+	fatalThrowID          = -2
+	hardcodedBreakpointID = -3
 
 	NoLogicalID = -1000 // Logical breakpoint ID for breakpoints internal breakpoints.
 )

pkg/proc/core/core.go

@@ -355,18 +355,17 @@ func (t *thread) StepInstruction() error {
 	return ErrContinueCore
 }
 
-// Blocked will return false always for core files as there is
-// no execution.
-func (t *thread) Blocked() bool {
-	return false
-}
-
 // SetCurrentBreakpoint will always just return nil
 // for core files, as there are no breakpoints in core files.
 func (t *thread) SetCurrentBreakpoint(adjustPC bool) error {
 	return nil
 }
 
+// SoftExc returns true if this thread received a software exception during the last resume.
+func (t *thread) SoftExc() bool {
+	return false
+}
+
 // Common returns a struct containing common information
 // across thread implementations.
 func (t *thread) Common() *proc.CommonThread {

pkg/proc/gdbserial/gdbserver.go

@@ -795,8 +795,8 @@ const (
 	childSignal      = 0x11
 	stopSignal       = 0x13
 
-	_SIGILL = 0x4
-	_SIGFPE = 0x8
+	_SIGILL  = 0x4
+	_SIGFPE  = 0x8
 	_SIGKILL = 0x9
 
 	debugServerTargetExcBadAccess      = 0x91
@@ -1541,6 +1541,11 @@ func (t *gdbThread) StepInstruction() error {
 	return t.p.conn.step(t, &threadUpdater{p: t.p}, false)
 }
 
+// SoftExc returns true if this thread received a software exception during the last resume.
+func (t *gdbThread) SoftExc() bool {
+	return t.setbp
+}
+
 // Blocked returns true if the thread is blocked in runtime or kernel code.
 func (t *gdbThread) Blocked() bool {
 	regs, err := t.Registers()
@@ -1883,7 +1888,7 @@ func (t *gdbThread) clearBreakpointState() {
 func (t *gdbThread) SetCurrentBreakpoint(adjustPC bool) error {
 	// adjustPC is ignored, it is the stub's responsibiility to set the PC
 	// address correctly after hitting a breakpoint.
-	t.clearBreakpointState()
+	t.CurrentBreakpoint.Clear()
 	if t.watchAddr > 0 {
 		t.CurrentBreakpoint.Breakpoint = t.p.Breakpoints().M[t.watchAddr]
 		if t.CurrentBreakpoint.Breakpoint == nil {

pkg/proc/native/nonative_darwin.go

@@ -137,4 +137,9 @@ func (t *nativeThread) Stopped() bool {
 	panic(ErrNativeBackendDisabled)
 }
 
+// SoftExc returns true if this thread received a software exception during the last resume.
+func (t *nativeThread) SoftExc() bool {
+	panic(ErrNativeBackendDisabled)
+}
+
 func initialize(dbp *nativeProcess) error { return nil }

pkg/proc/native/proc_windows.go

@@ -341,6 +341,9 @@ func (dbp *nativeProcess) waitForDebugEvent(flags waitForDebugEventFlags) (threa
 
 				if atbp {
 					dbp.os.breakThread = tid
+					if th := dbp.threads[tid]; th != nil {
+						th.os.setbp = true
+					}
 					return tid, 0, nil
 				} else {
 					continueStatus = _DBG_CONTINUE
@@ -428,6 +431,10 @@ func (dbp *nativeProcess) stop(trapthread *nativeThread) (*nativeThread, error)
 	}
 
 	dbp.os.running = false
+	for _, th := range dbp.threads {
+		th.os.setbp = false
+	}
+	trapthread.os.setbp = true
 
 	// While the debug event that stopped the target was being propagated
 	// other target threads could generate other debug events.

pkg/proc/native/threads_darwin.go

@@ -138,3 +138,8 @@ func (t *nativeThread) restoreRegisters(sr proc.Registers) error {
 func (t *nativeThread) withDebugRegisters(f func(*amd64util.DebugRegisters) error) error {
 	return proc.ErrHWBreakUnsupported
 }
+
+// SoftExc returns true if this thread received a software exception during the last resume.
+func (t *nativeThread) SoftExc() bool {
+	return false
+}

pkg/proc/native/threads_freebsd.go

@@ -125,3 +125,8 @@ func (t *nativeThread) ReadMemory(data []byte, addr uint64) (n int, err error) {
 func (t *nativeThread) withDebugRegisters(f func(*amd64util.DebugRegisters) error) error {
 	return proc.ErrHWBreakUnsupported
 }
+
+// SoftExc returns true if this thread received a software exception during the last resume.
+func (t *nativeThread) SoftExc() bool {
+	return false
+}

pkg/proc/native/threads_linux.go

@@ -115,3 +115,8 @@ func (t *nativeThread) ReadMemory(data []byte, addr uint64) (n int, err error) {
 	}
 	return
 }
+
+// SoftExc returns true if this thread received a software exception during the last resume.
+func (t *nativeThread) SoftExc() bool {
+	return t.os.setbp
+}

pkg/proc/native/threads_windows.go

@@ -22,6 +22,7 @@ type osSpecificDetails struct {
 	hThread            syscall.Handle
 	dbgUiRemoteBreakIn bool // whether thread is an auxiliary DbgUiRemoteBreakIn thread created by Windows
 	delayErr           error
+	setbp              bool
 }
 
 func (t *nativeThread) singleStep() error {
@@ -186,3 +187,8 @@ func (t *nativeThread) withDebugRegisters(f func(*amd64util.DebugRegisters) erro
 
 	return nil
 }
+
+// SoftExc returns true if this thread received a software exception during the last resume.
+func (t *nativeThread) SoftExc() bool {
+	return t.os.setbp
+}

pkg/proc/proc_test.go

@@ -1488,6 +1488,44 @@ func TestBreakpointCounts(t *testing.T) {
 	})
 }
 
+func TestHardcodedBreakpointCounts(t *testing.T) {
+	skipOn(t, "broken", "freebsd")
+	withTestProcess("hcbpcountstest", t, func(p *proc.Target, fixture protest.Fixture) {
+		counts := map[int]int{}
+		for {
+			if err := p.Continue(); err != nil {
+				if _, exited := err.(proc.ErrProcessExited); exited {
+					break
+				}
+				assertNoError(err, t, "Continue()")
+			}
+
+			for _, th := range p.ThreadList() {
+				bp := th.Breakpoint().Breakpoint
+				if bp == nil {
+					continue
+				}
+				if bp.Name != proc.HardcodedBreakpoint {
+					t.Fatalf("wrong breakpoint name %s", bp.Name)
+				}
+				g, err := proc.GetG(th)
+				assertNoError(err, t, "GetG")
+				counts[g.ID]++
+			}
+		}
+
+		if len(counts) != 2 {
+			t.Fatalf("Wrong number of goroutines for hardcoded breakpoint (%d)", len(counts))
+		}
+
+		for goid, count := range counts {
+			if count != 100 {
+				t.Fatalf("Wrong hit count for hardcoded breakpoint (%d) on goroutine %d", count, goid)
+			}
+		}
+	})
+}
+
 func BenchmarkArray(b *testing.B) {
 	// each bencharr struct is 128 bytes, bencharr is 64 elements long
 	b.SetBytes(int64(64 * 128))

pkg/proc/target_exec.go

@@ -55,12 +55,14 @@ func (dbp *Target) Continue() error {
 		thread.Common().returnValues = nil
 	}
 	dbp.Breakpoints().WatchOutOfScope = nil
+	dbp.clearHardcodedBreakpoints()
 	dbp.CheckAndClearManualStopRequest()
 	defer func() {
 		// Make sure we clear internal breakpoints if we simultaneously receive a
 		// manual stop request and hit a breakpoint.
 		if dbp.CheckAndClearManualStopRequest() {
 			dbp.StopReason = StopManual
+			dbp.clearHardcodedBreakpoints()
 			if dbp.KeepSteppingBreakpoints&HaltKeepsSteppingBreakpoints == 0 {
 				dbp.ClearSteppingBreakpoints()
 			}
@@ -69,6 +71,7 @@ func (dbp *Target) Continue() error {
 	for {
 		if dbp.CheckAndClearManualStopRequest() {
 			dbp.StopReason = StopManual
+			dbp.clearHardcodedBreakpoints()
 			if dbp.KeepSteppingBreakpoints&HaltKeepsSteppingBreakpoints == 0 {
 				dbp.ClearSteppingBreakpoints()
 			}
@@ -107,9 +110,10 @@ func (dbp *Target) Continue() error {
 		}
 
 		callInjectionDone, callErr := callInjectionProtocol(dbp, threads)
-		// callErr check delayed until after pickCurrentThread, which must always
-		// happen, otherwise the debugger could be left in an inconsistent
-		// state.
+		hcbpErr := dbp.handleHardcodedBreakpoints(trapthread, threads)
+		// callErr and hcbpErr check delayed until after pickCurrentThread, which
+		// must always happen, otherwise the debugger could be left in an
+		// inconsistent state.
 
 		if err := pickCurrentThread(dbp, trapthread, threads); err != nil {
 			return err
@@ -118,52 +122,14 @@ func (dbp *Target) Continue() error {
 		if callErr != nil {
 			return callErr
 		}
+		if hcbpErr != nil {
+			return hcbpErr
+		}
 
 		curthread := dbp.CurrentThread()
 		curbp := curthread.Breakpoint()
 
 		switch {
-		case curbp.Breakpoint == nil:
-			// runtime.Breakpoint, manual stop or debugCallV1-related stop
-
-			loc, err := curthread.Location()
-			if err != nil || loc.Fn == nil {
-				return conditionErrors(threads)
-			}
-			g, _ := GetG(curthread)
-			arch := dbp.BinInfo().Arch
-
-			switch {
-			case loc.Fn.Name == "runtime.breakpoint":
-				if recorded, _ := dbp.Recorded(); recorded {
-					return conditionErrors(threads)
-				}
-				// In linux-arm64, PtraceSingleStep seems cannot step over BRK instruction
-				// (linux-arm64 feature or kernel bug maybe).
-				if !arch.BreakInstrMovesPC() {
-					setPC(curthread, loc.PC+uint64(arch.BreakpointSize()))
-				}
-				// Single-step current thread until we exit runtime.breakpoint and
-				// runtime.Breakpoint.
-				// On go < 1.8 it was sufficient to single-step twice on go1.8 a change
-				// to the compiler requires 4 steps.
-				if err := stepInstructionOut(dbp, curthread, "runtime.breakpoint", "runtime.Breakpoint"); err != nil {
-					return err
-				}
-				dbp.StopReason = StopHardcodedBreakpoint
-				return conditionErrors(threads)
-			case g == nil || dbp.fncallForG[g.ID] == nil:
-				// a hardcoded breakpoint somewhere else in the code (probably cgo), or manual stop in cgo
-				if !arch.BreakInstrMovesPC() {
-					bpsize := arch.BreakpointSize()
-					bp := make([]byte, bpsize)
-					dbp.Memory().ReadMemory(bp, loc.PC)
-					if bytes.Equal(bp, arch.BreakpointInstruction()) {
-						setPC(curthread, loc.PC+uint64(bpsize))
-					}
-				}
-				return conditionErrors(threads)
-			}
 		case curbp.Active && curbp.Stepping:
 			if curbp.SteppingInto {
 				// See description of proc.(*Process).next for the meaning of StepBreakpoints
@@ -214,7 +180,9 @@ func (dbp *Target) Continue() error {
 			if curbp.Name == UnrecoveredPanic {
 				dbp.ClearSteppingBreakpoints()
 			}
-			dbp.StopReason = StopBreakpoint
+			if curbp.LogicalID() != hardcodedBreakpointID {
+				dbp.StopReason = StopBreakpoint
+			}
 			if curbp.Breakpoint.WatchType != 0 {
 				dbp.StopReason = StopWatchpoint
 			}
@@ -246,8 +214,8 @@ func conditionErrors(threads []Thread) error {
 }
 
 // pick a new dbp.currentThread, with the following priority:
-// 	- a thread with onTriggeredInternalBreakpoint() == true
-// 	- a thread with onTriggeredBreakpoint() == true (prioritizing trapthread)
+// 	- a thread with an active stepping breakpoint
+// 	- a thread with an active breakpoint, prioritizing trapthread
 // 	- trapthread
 func pickCurrentThread(dbp *Target, trapthread Thread, threads []Thread) error {
 	for _, th := range threads {
@@ -1084,3 +1052,112 @@ func (w *onNextGoroutineWalker) Visit(n ast.Node) ast.Visitor {
 	}
 	return w
 }
+
+func (tgt *Target) clearHardcodedBreakpoints() {
+	threads := tgt.ThreadList()
+	for _, thread := range threads {
+		if thread.Breakpoint().Breakpoint != nil && thread.Breakpoint().LogicalID() == hardcodedBreakpointID {
+			thread.Breakpoint().Active = false
+			thread.Breakpoint().Breakpoint = nil
+		}
+	}
+}
+
+// handleHardcodedBreakpoints looks for threads stopped at a hardcoded
+// breakpoint (i.e. a breakpoint instruction, like INT 3, hardcoded in the
+// program's text) and sets a fake breakpoint on them with logical id
+// hardcodedBreakpointID.
+// It checks trapthread and all threads that have SoftExc returning true.
+func (tgt *Target) handleHardcodedBreakpoints(trapthread Thread, threads []Thread) error {
+	mem := tgt.Memory()
+	arch := tgt.BinInfo().Arch
+	recorded, _ := tgt.Recorded()
+
+	isHardcodedBreakpoint := func(thread Thread, pc uint64) uint64 {
+		for _, bpinstr := range [][]byte{arch.BreakpointInstruction(), arch.AltBreakpointInstruction()} {
+			if bpinstr == nil {
+				continue
+			}
+			buf := make([]byte, len(bpinstr))
+			pc2 := pc
+			if arch.BreakInstrMovesPC() {
+				pc2 -= uint64(len(bpinstr))
+			}
+			_, _ = mem.ReadMemory(buf, pc2)
+			if bytes.Equal(buf, bpinstr) {
+				return uint64(len(bpinstr))
+			}
+		}
+		return 0
+	}
+
+	stepOverBreak := func(thread Thread, pc uint64) {
+		if arch.BreakInstrMovesPC() {
+			return
+		}
+		if recorded {
+			return
+		}
+		if bpsize := isHardcodedBreakpoint(thread, pc); bpsize > 0 {
+			setPC(thread, pc+uint64(bpsize))
+		}
+	}
+
+	setHardcodedBreakpoint := func(thread Thread, loc *Location) {
+		bpstate := thread.Breakpoint()
+		hcbp := &Breakpoint{}
+		bpstate.Active = true
+		bpstate.Breakpoint = hcbp
+		hcbp.FunctionName = loc.Fn.Name
+		hcbp.File = loc.File
+		hcbp.Line = loc.Line
+		hcbp.Addr = loc.PC
+		hcbp.Name = HardcodedBreakpoint
+		hcbp.Breaklets = []*Breaklet{&Breaklet{Kind: UserBreakpoint, LogicalID: hardcodedBreakpointID}}
+		tgt.StopReason = StopHardcodedBreakpoint
+	}
+
+	for _, thread := range threads {
+		if thread.Breakpoint().Breakpoint != nil {
+			continue
+		}
+		if (thread.ThreadID() != trapthread.ThreadID()) && !thread.SoftExc() {
+			continue
+		}
+
+		loc, err := thread.Location()
+		if err != nil || loc.Fn == nil {
+			continue
+		}
+
+		g, _ := GetG(thread)
+
+		switch {
+		case loc.Fn.Name == "runtime.breakpoint":
+			if recorded, _ := tgt.Recorded(); recorded {
+				setHardcodedBreakpoint(thread, loc)
+				continue
+			}
+			stepOverBreak(thread, loc.PC)
+			// In linux-arm64, PtraceSingleStep seems cannot step over BRK instruction
+			// (linux-arm64 feature or kernel bug maybe).
+			if !arch.BreakInstrMovesPC() {
+				setPC(thread, loc.PC+uint64(arch.BreakpointSize()))
+			}
+			// Single-step current thread until we exit runtime.breakpoint and
+			// runtime.Breakpoint.
+			// On go < 1.8 it was sufficient to single-step twice on go1.8 a change
+			// to the compiler requires 4 steps.
+			if err := stepInstructionOut(tgt, thread, "runtime.breakpoint", "runtime.Breakpoint"); err != nil {
+				return err
+			}
+			setHardcodedBreakpoint(thread, loc)
+		case g == nil || tgt.fncallForG[g.ID] == nil:
+			if isHardcodedBreakpoint(thread, loc.PC) > 0 {
+				stepOverBreak(thread, loc.PC)
+				setHardcodedBreakpoint(thread, loc)
+			}
+		}
+	}
+	return nil
+}

pkg/proc/threads.go

@@ -30,6 +30,8 @@ type Thread interface {
 	StepInstruction() error
 	// SetCurrentBreakpoint updates the current breakpoint of this thread, if adjustPC is true also checks for breakpoints that were just hit (this should only be passed true after a thread resume)
 	SetCurrentBreakpoint(adjustPC bool) error
+	// SoftExc returns true if this thread received a software exception during the last resume.
+	SoftExc() bool
 	// Common returns the CommonThread structure for this thread
 	Common() *CommonThread