proc: fix interaction of RequestManualStop and conditional breakpoints (#876)

* proc: fix interaction of RequestManualStop and conditional breakpoints

A conditional breakpoint that is hit but has the condition evaluate to
false can block a RequestManualStop from working. If the conditional
breakpoint is set on an instruction that is executed very frequently by
multiple goroutines (or many conditional breakpoints are set) it could
prevent all calls to RequestManualStop from working.

This commit fixes the problem by changing proc.Continue to exit
unconditionally after a RequestManualStop is called.

* proc/gdbserial: fix ContinueOnce getting stuck on macOS

Fixes #902

pkg/proc/core/core.go

@@ -269,6 +269,10 @@ func (p *Process) RequestManualStop() error {
 	return nil
 }
 
+func (p *Process) ManualStopRequested() bool {
+	return false
+}
+
 func (p *Process) CurrentThread() proc.Thread {
 	return &Thread{p.currentThread, p}
 }

pkg/proc/gdbserial/gdbserver.go

@@ -108,6 +108,8 @@ type Process struct {
 	exited bool
 	ctrlC  bool // ctrl-c was sent to stop inferior
 
+	manualStopRequested bool
+
 	breakpoints                 map[uint64]*proc.Breakpoint
 	breakpointIDCounter         int
 	internalBreakpointIDCounter int
@@ -540,7 +542,7 @@ func (p *Process) ContinueOnce() (proc.Thread, error) {
 		th.clearBreakpointState()
 	}
 
-	p.ctrlC = false
+	p.setCtrlC(false)
 
 	// resume all threads
 	var threadID string
@@ -565,7 +567,7 @@ continueLoop:
 		// the ctrlC flag to know that we are the originators.
 		switch sig {
 		case interruptSignal: // interrupt
-			if p.ctrlC {
+			if p.getCtrlC() {
 				break continueLoop
 			}
 		case breakpointSignal: // breakpoint
@@ -655,18 +657,42 @@ func (p *Process) SwitchGoroutine(gid int) error {
 }
 
 func (p *Process) RequestManualStop() error {
+	p.conn.manualStopMutex.Lock()
+	p.manualStopRequested = true
 	if !p.conn.running {
+		p.conn.manualStopMutex.Unlock()
 		return nil
 	}
 	p.ctrlC = true
+	p.conn.manualStopMutex.Unlock()
 	return p.conn.sendCtrlC()
 }
 
+func (p *Process) ManualStopRequested() bool {
+	p.conn.manualStopMutex.Lock()
+	msr := p.manualStopRequested
+	p.manualStopRequested = false
+	p.conn.manualStopMutex.Unlock()
+	return msr
+}
+
+func (p *Process) setCtrlC(v bool) {
+	p.conn.manualStopMutex.Lock()
+	p.ctrlC = v
+	p.conn.manualStopMutex.Unlock()
+}
+
+func (p *Process) getCtrlC() bool {
+	p.conn.manualStopMutex.Lock()
+	defer p.conn.manualStopMutex.Unlock()
+	return p.ctrlC
+}
+
 func (p *Process) Halt() error {
 	if p.exited {
 		return nil
 	}
-	p.ctrlC = true
+	p.setCtrlC(true)
 	return p.conn.sendCtrlC()
 }
 
@@ -715,7 +741,7 @@ func (p *Process) Restart(pos string) error {
 		th.clearBreakpointState()
 	}
 
-	p.ctrlC = false
+	p.setCtrlC(false)
 
 	err := p.conn.restart(pos)
 	if err != nil {
@@ -1129,8 +1155,11 @@ func (t *Thread) Blocked() bool {
 		return false
 	}
 	pc := regs.PC()
-	fn := t.BinInfo().PCToFunc(pc)
+	f, ln, fn := t.BinInfo().PCToLine(pc)
 	if fn == nil {
+		if f == "" && ln == 0 {
+			return true
+		}
 		return false
 	}
 	switch fn.Name {
@@ -1306,6 +1335,12 @@ func (t *Thread) reloadGAtPC() error {
 
 	_, _, err = t.p.conn.step(t.strID, nil)
 	if err != nil {
+		if err == threadBlockedError {
+			t.regs.tls = 0
+			t.regs.gaddr = 0
+			t.regs.hasgaddr = true
+			return nil
+		}
 		return err
 	}
 
@@ -1353,6 +1388,12 @@ func (t *Thread) reloadGAlloc() error {
 
 	_, _, err = t.p.conn.step(t.strID, nil)
 	if err != nil {
+		if err == threadBlockedError {
+			t.regs.tls = 0
+			t.regs.gaddr = 0
+			t.regs.hasgaddr = true
+			return nil
+		}
 		return err
 	}
 

pkg/proc/gdbserial/gdbserver_conn.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/derekparker/delve/pkg/proc"
@@ -23,8 +24,9 @@ type gdbConn struct {
 	inbuf  []byte
 	outbuf bytes.Buffer
 
-	running    bool
+	manualStopMutex sync.Mutex
-	resumeChan chan<- struct{}
+	running         bool
+	resumeChan      chan<- struct{}
 
 	direction proc.Direction // direction of execution
 
@@ -536,12 +538,17 @@ func (conn *gdbConn) resume(sig uint8, tu *threadUpdater) (string, uint8, error)
 		conn.outbuf.Reset()
 		fmt.Fprint(&conn.outbuf, "$bc")
 	}
+	conn.manualStopMutex.Lock()
 	if err := conn.send(conn.outbuf.Bytes()); err != nil {
+		conn.manualStopMutex.Unlock()
 		return "", 0, err
 	}
 	conn.running = true
+	conn.manualStopMutex.Unlock()
 	defer func() {
+		conn.manualStopMutex.Lock()
 		conn.running = false
+		conn.manualStopMutex.Unlock()
 	}()
 	if conn.resumeChan != nil {
 		close(conn.resumeChan)
@@ -568,7 +575,11 @@ func (conn *gdbConn) step(threadID string, tu *threadUpdater) (string, uint8, er
 	return conn.waitForvContStop("singlestep", threadID, tu)
 }
 
+var threadBlockedError = errors.New("thread blocked")
+
 func (conn *gdbConn) waitForvContStop(context string, threadID string, tu *threadUpdater) (string, uint8, error) {
+	count := 0
+	failed := false
 	for {
 		conn.conn.SetReadDeadline(time.Now().Add(heartbeatInterval))
 		resp, err := conn.recv(nil, context)
@@ -581,6 +592,13 @@ func (conn *gdbConn) waitForvContStop(context string, threadID string, tu *threa
 			if conn.isDebugserver {
 				conn.send([]byte("$?"))
 			}
+			if count > 1 && context == "singlestep" {
+				failed = true
+				conn.sendCtrlC()
+			}
+			count++
+		} else if failed {
+			return "", 0, threadBlockedError
 		} else if err != nil {
 			return "", 0, err
 		} else {

pkg/proc/interface.go

@@ -87,6 +87,9 @@ type ProcessManipulation interface {
 	SwitchThread(int) error
 	SwitchGoroutine(int) error
 	RequestManualStop() error
+	// ManualStopRequested returns true the first time it's called after a call
+	// to RequestManualStop.
+	ManualStopRequested() bool
 	Halt() error
 	Kill() error
 	Detach(bool) error

pkg/proc/native/proc.go

@@ -44,6 +44,7 @@ type Process struct {
 	ptraceChan                  chan func()
 	ptraceDoneChan              chan interface{}
 	childProcess                bool // this process was launched, not attached to
+	manualStopRequested         bool
 }
 
 // New returns an initialized Process struct. Before returning,
@@ -179,10 +180,19 @@ func (dbp *Process) RequestManualStop() error {
 	}
 	dbp.haltMu.Lock()
 	defer dbp.haltMu.Unlock()
+	dbp.manualStopRequested = true
 	dbp.halt = true
 	return dbp.requestManualStop()
 }
 
+func (dbp *Process) ManualStopRequested() bool {
+	dbp.haltMu.Lock()
+	msr := dbp.manualStopRequested
+	dbp.manualStopRequested = false
+	dbp.haltMu.Unlock()
+	return msr
+}
+
 // SetBreakpoint sets a breakpoint at addr, and stores it in the process wide
 // break point table. Setting a break point must be thread specific due to
 // ptrace actions needing the thread to be in a signal-delivery-stop.

pkg/proc/proc.go

@@ -95,7 +95,11 @@ func Next(dbp Process) (err error) {
 // process. It will continue until it hits a breakpoint
 // or is otherwise stopped.
 func Continue(dbp Process) error {
+	dbp.ManualStopRequested()
 	for {
+		if dbp.ManualStopRequested() {
+			return nil
+		}
 		trapthread, err := dbp.ContinueOnce()
 		if err != nil {
 			return err