proc: next/step/stepout restarts thread from wrong instruction (#1657)

proc.Next and proc.Step will call, after setting their temp
breakpoints, curthread.SetCurrentBreakpoint. This is intended to find
if one of the newly created breakpoints happens to be at the same
instruction that curthread is stopped at.
However SetCurrentBreakpoint is intended to be called after a Continue
and StepInstruction operation so it will also detect if curthread is
stopped one byte after a breakpoint.
If the instruction immediately preceeding the current instruction of
curthread happens to:
 1. have one of the newly created temp breakpoints
 2. be one byte long
SetCurrentBreakpoint will believe that we just hit that breakpoint and
therefore the instruction should be repeated, and thus rewind the PC of
curthread by 1.

We should distinguish between the two uses of SetCurrentBreakpoint and
disable the check for "just hit" breakpoints when inappropriate.

Fixes #1656

_fixtures/issue1656/main.go

@@ -0,0 +1,19 @@
+package main
+
+import (
+	"fmt"
+)
+
+var g int = 0
+
+func compromised(n int64)
+
+//go:nosplit
+func skipped() {
+	g++
+}
+
+func main() {
+	compromised(1)
+	fmt.Printf("%d\n", g)
+}

_fixtures/issue1656/main.s

@@ -0,0 +1,12 @@
+#include "textflag.h"
+
+TEXT ·compromised(SB),NOSPLIT,$0-8
+	CMPQ n+0(FP), $0
+	JNZ notzero
+	RET
+notzero:
+	MOVQ $0, AX
+	MOVQ $1, AX
+	CALL main·skipped(SB)
+	RET
+	

pkg/proc/core/core.go

@@ -340,7 +340,7 @@ func (t *Thread) Blocked() bool {
 
 // SetCurrentBreakpoint will always just return nil
 // for core files, as there are no breakpoints in core files.
-func (t *Thread) SetCurrentBreakpoint() error {
+func (t *Thread) SetCurrentBreakpoint(adjustPC bool) error {
 	return nil
 }
 

pkg/proc/gdbserial/gdbserver.go

@@ -767,7 +767,7 @@ func (p *Process) StepInstruction() error {
 	if err != nil {
 		return err
 	}
-	err = thread.SetCurrentBreakpoint()
+	err = thread.SetCurrentBreakpoint(true)
 	if err != nil {
 		return err
 	}
@@ -1024,10 +1024,6 @@ func (p *Process) Breakpoints() *proc.BreakpointMap {
 
 // FindBreakpoint returns the breakpoint at the given address.
 func (p *Process) FindBreakpoint(pc uint64) (*proc.Breakpoint, bool) {
-	// Check to see if address is past the breakpoint, (i.e. breakpoint was hit).
-	if bp, ok := p.breakpoints.M[pc-uint64(p.bi.Arch.BreakpointSize())]; ok {
-		return bp, true
-	}
 	// Directly use addr to lookup breakpoint.
 	if bp, ok := p.breakpoints.M[pc]; ok {
 		return bp, true
@@ -1193,7 +1189,7 @@ func (p *Process) setCurrentBreakpoints() error {
 	if p.threadStopInfo {
 		for _, th := range p.threads {
 			if th.setbp {
-				err := th.SetCurrentBreakpoint()
+				err := th.SetCurrentBreakpoint(true)
 				if err != nil {
 					return err
 				}
@@ -1203,7 +1199,7 @@ func (p *Process) setCurrentBreakpoints() error {
 	if !p.threadStopInfo {
 		for _, th := range p.threads {
 			if th.CurrentBreakpoint.Breakpoint == nil {
-				err := th.SetCurrentBreakpoint()
+				err := th.SetCurrentBreakpoint(true)
 				if err != nil {
 					return err
 				}
@@ -1573,7 +1569,9 @@ func (t *Thread) clearBreakpointState() {
 }
 
 // SetCurrentBreakpoint will find and set the threads current breakpoint.
-func (t *Thread) SetCurrentBreakpoint() error {
+func (t *Thread) 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()
 	regs, err := t.Registers(false)
 	if err != nil {

pkg/proc/native/proc.go

@@ -294,7 +294,7 @@ func (dbp *Process) StepInstruction() (err error) {
 	if err != nil {
 		return err
 	}
-	err = thread.SetCurrentBreakpoint()
+	err = thread.SetCurrentBreakpoint(true)
 	if err != nil {
 		return err
 	}
@@ -339,10 +339,12 @@ func (dbp *Process) SwitchGoroutine(gid int) error {
 }
 
 // FindBreakpoint finds the breakpoint for the given pc.
-func (dbp *Process) FindBreakpoint(pc uint64) (*proc.Breakpoint, bool) {
+func (dbp *Process) FindBreakpoint(pc uint64, adjustPC bool) (*proc.Breakpoint, bool) {
-	// Check to see if address is past the breakpoint, (i.e. breakpoint was hit).
+	if adjustPC {
-	if bp, ok := dbp.breakpoints.M[pc-uint64(dbp.bi.Arch.BreakpointSize())]; ok {
+		// Check to see if address is past the breakpoint, (i.e. breakpoint was hit).
-		return bp, true
+		if bp, ok := dbp.breakpoints.M[pc-uint64(dbp.bi.Arch.BreakpointSize())]; ok {
+			return bp, true
+		}
 	}
 	// Directly use addr to lookup breakpoint.
 	if bp, ok := dbp.breakpoints.M[pc]; ok {

pkg/proc/native/proc_darwin.go

@@ -443,10 +443,10 @@ func (dbp *Process) stop(trapthread *Thread) (err error) {
 	if !dbp.os.initialized {
 		return nil
 	}
-	trapthread.SetCurrentBreakpoint()
+	trapthread.SetCurrentBreakpoint(true)
 	for _, port := range ports {
 		if th, ok := dbp.threads[port]; ok {
-			err := th.SetCurrentBreakpoint()
+			err := th.SetCurrentBreakpoint(true)
 			if err != nil {
 				return err
 			}

pkg/proc/native/proc_freebsd.go

@@ -340,7 +340,7 @@ func (dbp *Process) stop(trapthread *Thread) (err error) {
 	// set breakpoints on all threads
 	for _, th := range dbp.threads {
 		if th.CurrentBreakpoint.Breakpoint == nil {
-			if err := th.SetCurrentBreakpoint(); err != nil {
+			if err := th.SetCurrentBreakpoint(true); err != nil {
 				return err
 			}
 		}

pkg/proc/native/proc_linux.go

@@ -457,7 +457,7 @@ func (dbp *Process) stop(trapthread *Thread) (err error) {
 	// set breakpoints on all threads
 	for _, th := range dbp.threads {
 		if th.CurrentBreakpoint.Breakpoint == nil {
-			if err := th.SetCurrentBreakpoint(); err != nil {
+			if err := th.SetCurrentBreakpoint(true); err != nil {
 				return err
 			}
 		}

pkg/proc/native/proc_windows.go

@@ -432,7 +432,7 @@ func (dbp *Process) stop(trapthread *Thread) (err error) {
 	// call to _ContinueDebugEvent will resume execution of some of the
 	// target threads.
 
-	err = trapthread.SetCurrentBreakpoint()
+	err = trapthread.SetCurrentBreakpoint(true)
 	if err != nil {
 		return err
 	}
@@ -459,7 +459,7 @@ func (dbp *Process) stop(trapthread *Thread) (err error) {
 		if tid == 0 {
 			break
 		}
-		err = dbp.threads[tid].SetCurrentBreakpoint()
+		err = dbp.threads[tid].SetCurrentBreakpoint(true)
 		if err != nil {
 			return err
 		}

pkg/proc/native/threads.go

@@ -34,7 +34,7 @@ func (t *Thread) Continue() error {
 	}
 	// Check whether we are stopped at a breakpoint, and
 	// if so, single step over it before continuing.
-	if _, ok := t.dbp.FindBreakpoint(pc); ok {
+	if _, ok := t.dbp.FindBreakpoint(pc, false); ok {
 		if err := t.StepInstruction(); err != nil {
 			return err
 		}
@@ -58,7 +58,7 @@ func (t *Thread) StepInstruction() (err error) {
 		return err
 	}
 
-	bp, ok := t.dbp.FindBreakpoint(pc)
+	bp, ok := t.dbp.FindBreakpoint(pc, true)
 	if ok {
 		// Clear the breakpoint so that we can continue execution.
 		err = t.ClearBreakpoint(bp)
@@ -113,13 +113,13 @@ func (t *Thread) Common() *proc.CommonThread {
 
 // SetCurrentBreakpoint sets the current breakpoint that this
 // thread is stopped at as CurrentBreakpoint on the thread struct.
-func (t *Thread) SetCurrentBreakpoint() error {
+func (t *Thread) SetCurrentBreakpoint(adjustPC bool) error {
 	t.CurrentBreakpoint.Clear()
 	pc, err := t.PC()
 	if err != nil {
 		return err
 	}
-	if bp, ok := t.dbp.FindBreakpoint(pc); ok {
+	if bp, ok := t.dbp.FindBreakpoint(pc, adjustPC); ok {
 		if err = t.SetPC(bp.Addr); err != nil {
 			return err
 		}

pkg/proc/proc.go

@@ -308,7 +308,7 @@ func stepInstructionOut(dbp Process, curthread Thread, fnname1, fnname2 string)
 			if g != nil && selg != nil && g.ID == selg.ID {
 				selg.CurrentLoc = *loc
 			}
-			return curthread.SetCurrentBreakpoint()
+			return curthread.SetCurrentBreakpoint(true)
 		}
 	}
 }
@@ -456,7 +456,7 @@ func StepOut(dbp Process) error {
 	}
 
 	if bp := curthread.Breakpoint(); bp.Breakpoint == nil {
-		curthread.SetCurrentBreakpoint()
+		curthread.SetCurrentBreakpoint(false)
 	}
 
 	success = true

pkg/proc/proc_test.go

@@ -4480,3 +4480,16 @@ func TestCgoStacktrace2(t *testing.T) {
 		stacktraceCheck(t, []string{"C.sigsegv", "C.testfn", "main.main"}, frames)
 	})
 }
+
+func TestIssue1656(t *testing.T) {
+	withTestProcess("issue1656/", t, func(p proc.Process, fixture protest.Fixture) {
+		setFileLineBreakpoint(p, t, filepath.ToSlash(filepath.Join(fixture.BuildDir, "main.s")), 5)
+		assertNoError(proc.Continue(p), t, "Continue()")
+		t.Logf("step1\n")
+		assertNoError(proc.Step(p), t, "Step()")
+		assertLineNumber(p, t, 8, "wrong line number after first step")
+		t.Logf("step2\n")
+		assertNoError(proc.Step(p), t, "Step()")
+		assertLineNumber(p, t, 9, "wrong line number after second step")
+	})
+}

pkg/proc/threads.go

@@ -38,8 +38,8 @@ type Thread interface {
 	StepInstruction() error
 	// Blocked returns true if the thread is blocked
 	Blocked() bool
-	// SetCurrentBreakpoint updates the current breakpoint of this thread
+	// 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() error
+	SetCurrentBreakpoint(adjustPC bool) error
 	// Common returns the CommonThread structure for this thread
 	Common() *CommonThread
 
@@ -319,7 +319,7 @@ func next(dbp Process, stepInto, inlinedStepOut bool) error {
 	}
 
 	if bp := curthread.Breakpoint(); bp.Breakpoint == nil {
-		curthread.SetCurrentBreakpoint()
+		curthread.SetCurrentBreakpoint(false)
 	}
 	success = true
 	return nil