From 07e53f7cbb0b5329f698d61ea309eb997c14e4b9 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Sat, 8 Jul 2017 01:29:37 +0200 Subject: [PATCH] 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 | 4 +++ pkg/proc/gdbserial/gdbserver.go | 51 +++++++++++++++++++++++++--- pkg/proc/gdbserial/gdbserver_conn.go | 22 ++++++++++-- pkg/proc/interface.go | 3 ++ pkg/proc/native/proc.go | 10 ++++++ pkg/proc/proc.go | 4 +++ 6 files changed, 87 insertions(+), 7 deletions(-) diff --git a/pkg/proc/core/core.go b/pkg/proc/core/core.go index 6530f928..fbca5624 100644 --- a/pkg/proc/core/core.go +++ b/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} } diff --git a/pkg/proc/gdbserial/gdbserver.go b/pkg/proc/gdbserial/gdbserver.go index c5294105..11869cb7 100644 --- a/pkg/proc/gdbserial/gdbserver.go +++ b/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 } diff --git a/pkg/proc/gdbserial/gdbserver_conn.go b/pkg/proc/gdbserial/gdbserver_conn.go index 601de1f3..1c4e84a5 100644 --- a/pkg/proc/gdbserial/gdbserver_conn.go +++ b/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 - resumeChan chan<- struct{} + manualStopMutex sync.Mutex + 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 { diff --git a/pkg/proc/interface.go b/pkg/proc/interface.go index 3deef162..f3b55624 100644 --- a/pkg/proc/interface.go +++ b/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 diff --git a/pkg/proc/native/proc.go b/pkg/proc/native/proc.go index 8d6901f8..4ce45a08 100644 --- a/pkg/proc/native/proc.go +++ b/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. diff --git a/pkg/proc/proc.go b/pkg/proc/proc.go index 99480574..f3eb5b4e 100644 --- a/pkg/proc/proc.go +++ b/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