pkg/proc: fix watchpoints on macos (#3703)

It seems newer MacOS kernels are sending mach exceptions for watchpoints
which contain the hardware register number as opposed to the address
which triggered the exception. Also, ARM64 seems to have switched to
sending _EXC_I386_SGL as medata[0] for this exception type.

_fixtures/databpcountstest.go

@@ -1,7 +1,6 @@
 package main
 
 import (
-	"fmt"
 	"math/rand"
 	"sync"
 	"time"
@@ -12,7 +11,6 @@ var globalvar1 int
 func demo(id int, wait *sync.WaitGroup) {
 	for i := 0; i < 100; i++ {
 		sleep := rand.Intn(10) + 1
-		fmt.Printf("id: %d step: %d sleeping %d\n", id, i, sleep)
 		globalvar1 = globalvar1 + 1
 		time.Sleep(time.Duration(sleep) * time.Millisecond)
 	}

pkg/proc/bininfo.go

@@ -1876,7 +1876,6 @@ func (bi *BinaryInfo) parseDebugFramePE(image *Image, exe *pe.File, debugInfoByt
 // loadBinaryInfoMacho specifically loads information from a Mach-O binary.
 func loadBinaryInfoMacho(bi *BinaryInfo, image *Image, path string, entryPoint uint64, wg *sync.WaitGroup) error {
 	exe, err := macho.Open(path)
-
 	if err != nil {
 		return err
 	}

pkg/proc/gdbserial/gdbserver.go

@@ -182,6 +182,7 @@ type gdbThread struct {
 	sig               uint8  // signal received by thread after last stop
 	setbp             bool   // thread was stopped because of a breakpoint
 	watchAddr         uint64 // if > 0 this is the watchpoint address
+	watchReg          int    // if < 0 there are no active watchpoints returned
 	common            proc.CommonThread
 }
 
@@ -851,7 +852,13 @@ func (p *gdbProcess) ContinueOnce(cctx *proc.ContinueOnceContext) (proc.Thread,
 	if p.conn.direction == proc.Forward {
 		// step threads stopped at any breakpoint over their breakpoint
 		for _, thread := range p.threads {
-			if thread.CurrentBreakpoint.Breakpoint != nil {
+			// Do not single step over a breakpoint if it is a watchpoint. The PC will already have advanced and we
+			// won't experience the same stutter effect as with a breakpoint. Also, there is a bug in certain versions
+			// of the MacOS mach kernel where single stepping when we have hardware watchpoints set will cause the
+			// kernel to send a spurious mach exception.
+			// See: https://github.com/llvm/llvm-project/blob/b9f2c16b50f68c978e90190f46a7c0db3f39e98c/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp#L814
+			// TODO(deparker): We can skip single stepping in the case of thread.BinInfo().Arch.BreakInstrMovesPC().
+			if thread.CurrentBreakpoint.Breakpoint != nil && thread.CurrentBreakpoint.WatchType == 0 {
 				if err := thread.stepInstruction(); err != nil {
 					return nil, proc.StopUnknown, err
 				}
@@ -896,6 +903,7 @@ continueLoop:
 			// reason the thread returned by resume() stopped.
 			trapthread.sig = sp.sig
 			trapthread.watchAddr = sp.watchAddr
+			trapthread.watchReg = sp.watchReg
 		}
 
 		var shouldStop, shouldExitErr bool
@@ -1427,12 +1435,14 @@ func (p *gdbProcess) updateThreadList(tu *threadUpdater) error {
 				}
 				return err
 			}
-			th.setbp = (sp.reason == "breakpoint" || (sp.reason == "" && sp.sig == breakpointSignal) || (sp.watchAddr > 0))
+			th.setbp = (sp.reason == "breakpoint" || (sp.reason == "" && sp.sig == breakpointSignal) || (sp.watchAddr > 0) || (sp.watchReg >= 0))
 			th.sig = sp.sig
 			th.watchAddr = sp.watchAddr
+			th.watchReg = sp.watchReg
 		} else {
 			th.sig = 0
 			th.watchAddr = 0
+			th.watchReg = -1
 		}
 	}
 
@@ -1913,6 +1923,8 @@ func (t *gdbThread) reloadGAlloc() error {
 
 func (t *gdbThread) clearBreakpointState() {
 	t.setbp = false
+	t.watchAddr = 0
+	t.watchReg = -1
 	t.CurrentBreakpoint.Clear()
 }
 
@@ -1921,13 +1933,36 @@ func (t *gdbThread) SetCurrentBreakpoint(adjustPC bool) error {
 	// adjustPC is ignored, it is the stub's responsibility to set the PC
 	// address correctly after hitting a breakpoint.
 	t.CurrentBreakpoint.Clear()
+	// t.watchAddr on certain mach kernel versions could contain the address of a
+	// software breakpoint, hardcoded breakpoint (e.g. runtime.Breakpoint) or a
+	// hardware watchpoint. The mach exception produced by the kernel *should* disambiguate
+	// but it doesn't.
 	if t.watchAddr > 0 {
 		t.CurrentBreakpoint.Breakpoint = t.p.Breakpoints().M[t.watchAddr]
 		if t.CurrentBreakpoint.Breakpoint == nil {
+			buf := make([]byte, t.BinInfo().Arch.BreakpointSize())
+			_, err := t.p.ReadMemory(buf, t.watchAddr)
+			isHardcodedBreakpoint := err == nil && (bytes.Equal(t.BinInfo().Arch.BreakpointInstruction(), buf) || bytes.Equal(t.BinInfo().Arch.AltBreakpointInstruction(), buf))
+			if isHardcodedBreakpoint {
+				// This is a hardcoded breakpoint, ignore.
+				// TODO(deparker): There's an optimization here since we will do this
+				// again at a higher level to determine if we've stopped at a hardcoded breakpoint.
+				// We could set some state here so that we don't do extra work later.
+				t.watchAddr = 0
+				return nil
+			}
 			return fmt.Errorf("could not find watchpoint at address %#x", t.watchAddr)
 		}
 		return nil
 	}
+	if t.watchReg >= 0 {
+		for _, bp := range t.p.Breakpoints().M {
+			if bp.WatchType != 0 && bp.HWBreakIndex == uint8(t.watchReg) {
+				t.CurrentBreakpoint.Breakpoint = bp
+				return nil
+			}
+		}
+	}
 	regs, err := t.Registers()
 	if err != nil {
 		return err

pkg/proc/gdbserial/gdbserver_conn.go

@@ -723,6 +723,7 @@ type stopPacket struct {
 	sig       uint8
 	reason    string
 	watchAddr uint64
+	watchReg  int
 }
 
 // Mach exception codes used to decode metype/medata keys in stop packets (necessary to support watchpoints with debugserver).
@@ -732,9 +733,10 @@ type stopPacket struct {
 //	https://opensource.apple.com/source/xnu/xnu-4570.1.46/osfmk/mach/i386/exception.h.auto.html
 //	https://opensource.apple.com/source/xnu/xnu-4570.1.46/osfmk/mach/arm/exception.h.auto.html
 const (
-	_EXC_BREAKPOINT   = 6     // mach exception type for hardware breakpoints
-	_EXC_I386_SGL     = 1     // mach exception code for single step on x86, for some reason this is also used for watchpoints
-	_EXC_ARM_DA_DEBUG = 0x102 // mach exception code for debug fault on arm/arm64
+	_EXC_BREAKPOINT     = 6     // mach exception type for hardware breakpoints
+	_EXC_I386_SGL       = 1     // mach exception code for single step on x86, for some reason this is also used for watchpoints
+	_EXC_ARM_DA_DEBUG   = 0x102 // mach exception code for debug fault on arm/arm64
+	_EXC_ARM_BREAKPOINT = 1     // mach exception code for breakpoint on arm/arm64
 )
 
 // executes 'vCont' (continue/step) command
@@ -750,13 +752,14 @@ func (conn *gdbConn) parseStopPacket(resp []byte, threadID string, tu *threadUpd
 			return false, stopPacket{}, fmt.Errorf("malformed stop packet: %s", string(resp))
 		}
 		sp.sig = uint8(sig)
+		sp.watchReg = -1
 
 		if logflags.GdbWire() && gdbWireFullStopPacket {
 			conn.log.Debugf("full stop packet: %s", string(resp))
 		}
 
 		var metype int
-		var medata = make([]uint64, 0, 10)
+		medata := make([]uint64, 0, 10)
 
 		csp := colonSemicolonParser{buf: resp[3:]}
 		for csp.next() {
@@ -796,18 +799,26 @@ func (conn *gdbConn) parseStopPacket(resp []byte, threadID string, tu *threadUpd
 				sp.watchAddr = medata[1] // this should be zero if this is really a single step stop and non-zero for watchpoints
 			}
 		case "arm64":
-			if metype == _EXC_BREAKPOINT && len(medata) >= 2 && medata[0] == _EXC_ARM_DA_DEBUG {
-				sp.watchAddr = medata[1]
+			if metype == _EXC_BREAKPOINT && len(medata) >= 2 && (medata[0] == _EXC_ARM_DA_DEBUG || medata[0] == _EXC_ARM_BREAKPOINT) {
+				// The arm64 specification allows for up to 16 debug registers.
+				// The registers are zero indexed, thus a value less than 16 will
+				// be a hardware breakpoint register index.
+				// See: https://developer.arm.com/documentation/102120/0101/Debug-exceptions
+				// TODO(deparker): we can ask debugserver for the number of hardware breakpoints
+				// directly.
+				if medata[1] < 16 {
+					sp.watchReg = int(medata[1])
+				} else {
+					sp.watchAddr = medata[1]
+				}
 			}
 		}
 
 		return false, sp, nil
 
 	case 'W', 'X':
 		// process exited, next two character are exit code
-
 		semicolon := bytes.Index(resp, []byte{';'})
-
 		if semicolon < 0 {
 			semicolon = len(resp)
 		}

pkg/proc/proc_test.go

@@ -3,6 +3,7 @@ package proc_test
 import (
 	"bytes"
 	"encoding/binary"
+	"errors"
 	"flag"
 	"fmt"
 	"go/ast"
@@ -173,6 +174,22 @@ func assertLineNumber(p *proc.Target, t *testing.T, lineno int, descr string) (s
 	return f, l
 }
 
+func assertLineNumberIn(p *proc.Target, t *testing.T, linenos []int, descr string) (string, int) {
+	f, l := currentLineNumber(p, t)
+	found := false
+	for _, lineno := range linenos {
+		if l == lineno {
+			found = true
+			break
+		}
+	}
+	if !found {
+		_, callerFile, callerLine, _ := runtime.Caller(1)
+		t.Fatalf("%s expected lines :%#v got %s:%d\n\tat %s:%d", descr, linenos, f, l, callerFile, callerLine)
+	}
+	return f, l
+}
+
 func assertFunctionName(p *proc.Target, t *testing.T, fnname string, descr string) {
 	pc := currentPC(p, t)
 	f, l, fn := p.BinInfo().PCToLine(pc)
@@ -985,7 +1002,6 @@ func TestStacktrace2(t *testing.T) {
 			t.Fatalf("Stack error at main.g()\n%v\n", locations)
 		}
 	})
-
 }
 
 func stackMatch(stack []loc, locations []proc.Stackframe, skipRuntime bool) bool {
@@ -5421,13 +5437,8 @@ func TestWatchpointsBasic(t *testing.T) {
 	skipOn(t, "see https://github.com/go-delve/delve/issues/2768", "windows")
 	protest.AllowRecording(t)
 
-	position1 := 19
-	position5 := 41
-
-	if runtime.GOARCH == "arm64" {
-		position1 = 18
-		position5 = 40
-	}
+	position1 := []int{18, 19}
+	position5 := []int{40, 41}
 
 	withTestProcess("databpeasy", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
 		setFunctionBreakpoint(p, t, "main.main")
@@ -5443,7 +5454,7 @@ func TestWatchpointsBasic(t *testing.T) {
 		assertNoError(err, t, "SetDataBreakpoint(write-only)")
 
 		assertNoError(grp.Continue(), t, "Continue 1")
-		assertLineNumber(p, t, position1, "Continue 1") // Position 1
+		assertLineNumberIn(p, t, position1, "Continue 1") // Position 1
 
 		if curbp := p.CurrentThread().Breakpoint().Breakpoint; curbp == nil || (curbp.LogicalID() != bp.LogicalID()) {
 			t.Fatal("breakpoint not set")
@@ -5470,7 +5481,7 @@ func TestWatchpointsBasic(t *testing.T) {
 		assertNoError(err, t, "SetDataBreakpoint(write-only, again)")
 
 		assertNoError(grp.Continue(), t, "Continue 5")
-		assertLineNumber(p, t, position5, "Continue 5") // Position 5
+		assertLineNumberIn(p, t, position5, "Continue 5") // Position 5
 	})
 }
 
@@ -5479,6 +5490,9 @@ func TestWatchpointCounts(t *testing.T) {
 	skipOn(t, "not implemented", "386")
 	skipOn(t, "see https://github.com/go-delve/delve/issues/2768", "windows")
 	skipOn(t, "not implemented", "ppc64le")
+	if _, isTeamCityTest := os.LookupEnv("TEAMCITY_VERSION"); isTeamCityTest {
+		skipOn(t, "CI is running a version of macOS that is too old (11.2)", "darwin", "arm64")
+	}
 	protest.AllowRecording(t)
 
 	withTestProcess("databpcountstest", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
@@ -5493,7 +5507,7 @@ func TestWatchpointCounts(t *testing.T) {
 
 		for {
 			if err := grp.Continue(); err != nil {
-				if _, exited := err.(proc.ErrProcessExited); exited {
+				if errors.As(err, &proc.ErrProcessExited{}) {
 					break
 				}
 				assertNoError(err, t, "Continue()")
@@ -5595,13 +5609,12 @@ func TestWatchpointStack(t *testing.T) {
 	skipOn(t, "not implemented", "386")
 	skipOn(t, "not implemented", "ppc64le")
 	skipOn(t, "see https://github.com/go-delve/delve/issues/2768", "windows")
+	if _, isTeamCityTest := os.LookupEnv("TEAMCITY_VERSION"); isTeamCityTest {
+		skipOn(t, "CI is running a version of macOS that is too old (11.2)", "darwin", "arm64")
+	}
 	protest.AllowRecording(t)
 
-	position1 := 17
-
-	if runtime.GOARCH == "arm64" {
-		position1 = 16
-	}
+	position1 := []int{16, 17}
 
 	withTestProcess("databpstack", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
 		setFileBreakpoint(p, t, fixture.Source, 11) // Position 0 breakpoint
@@ -5650,7 +5663,7 @@ func TestWatchpointStack(t *testing.T) {
 		}
 
 		assertNoError(grp.Continue(), t, "Continue 1")
-		assertLineNumber(p, t, position1, "Continue 1") // Position 1
+		assertLineNumberIn(p, t, position1, "Continue 1") // Position 1
 
 		assertNoError(grp.Continue(), t, "Continue 2")
 		t.Logf("%#v", p.CurrentThread().Breakpoint().Breakpoint)

service/debugger/debugger.go

@@ -494,7 +494,6 @@ func (d *Debugger) Restart(rerecord bool, pos string, resetArgs bool, newArgs []
 
 	if !resetArgs && (d.config.Stdout.File != nil || d.config.Stderr.File != nil) {
 		return nil, ErrCanNotRestart
-
 	}
 
 	if err := d.detach(true); err != nil {

service/test/integration2_test.go

@@ -1011,7 +1011,6 @@ func TestClientServer_FindLocations(t *testing.T) {
 		if locsNoSubst[0].PC != locsSubst[0].PC {
 			t.Fatalf("FindLocation with substitute rules mismatch %#v %#v", locsNoSubst[0], locsSubst[0])
 		}
-
 	})
 
 	withTestClient2("testnextdefer", t, func(c service.Client) {
@@ -2623,7 +2622,6 @@ func TestLongStringArg(t *testing.T) {
 			t.Logf("%#v\n", var2)
 			if var2.Value != val2 {
 				t.Fatalf("wrong value for variable: %q", var2.Value)
-
 			}
 			return var1.Addr
 		}