pkg/proc: fix watchpoints on macos

_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
 		}