From bba7547156f271842da912f2c213285e8fab0169 Mon Sep 17 00:00:00 2001 From: Derek Parker Date: Mon, 29 Apr 2024 05:06:32 -0700 Subject: [PATCH] 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 | 2 -- pkg/proc/bininfo.go | 1 - pkg/proc/gdbserial/gdbserver.go | 39 +++++++++++++++++++++-- pkg/proc/gdbserial/gdbserver_conn.go | 27 +++++++++++----- pkg/proc/proc_test.go | 47 ++++++++++++++++++---------- service/debugger/debugger.go | 1 - service/test/integration2_test.go | 2 -- 7 files changed, 86 insertions(+), 33 deletions(-) diff --git a/_fixtures/databpcountstest.go b/_fixtures/databpcountstest.go index 6a79a28b..a317db5c 100644 --- a/_fixtures/databpcountstest.go +++ b/_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) } diff --git a/pkg/proc/bininfo.go b/pkg/proc/bininfo.go index 6be55f82..0171fea4 100644 --- a/pkg/proc/bininfo.go +++ b/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 } diff --git a/pkg/proc/gdbserial/gdbserver.go b/pkg/proc/gdbserial/gdbserver.go index 23fdbafd..436af81f 100644 --- a/pkg/proc/gdbserial/gdbserver.go +++ b/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 diff --git a/pkg/proc/gdbserial/gdbserver_conn.go b/pkg/proc/gdbserial/gdbserver_conn.go index 61d61b9e..47295d12 100644 --- a/pkg/proc/gdbserial/gdbserver_conn.go +++ b/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,8 +799,18 @@ 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] + } } } @@ -805,9 +818,7 @@ func (conn *gdbConn) parseStopPacket(resp []byte, threadID string, tu *threadUpd case 'W', 'X': // process exited, next two character are exit code - semicolon := bytes.Index(resp, []byte{';'}) - if semicolon < 0 { semicolon = len(resp) } diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index d127d9e6..219f7170 100644 --- a/pkg/proc/proc_test.go +++ b/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) diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go index 4432426b..0be8c70a 100644 --- a/service/debugger/debugger.go +++ b/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 { diff --git a/service/test/integration2_test.go b/service/test/integration2_test.go index b6aaac43..48382c21 100644 --- a/service/test/integration2_test.go +++ b/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 }