From 16d8bd647f8621c769c5ecdc3ee0fd10a8c0619f Mon Sep 17 00:00:00 2001 From: aarzilli Date: Tue, 6 Jun 2017 17:26:16 +0200 Subject: [PATCH] proc/*: remove Process.Running Implementing proc.Process.Running in a thread safe way is complicated and nothing actually uses it besides tests, so we are better off rewriting the tests without Running and removing it. In particular: * The call to d.target.Running() in service/debugger/debugger.go (Restart) can never return true because that line executes while holding processMutex and all continue operations are also executed while holding processMutex. * The call to dbp.Running() pkg/proc/native/proc.go (Detach) can never return true, because it's only called from debugger.(*Debugger).detach() which is also always called while holding processMutex. Since some tests are hard to write correctly without Process.Running a simpler interface, Process.NotifyResumed, is introduced. Fixes #830 --- pkg/proc/core/core.go | 3 +-- pkg/proc/gdbserial/gdbserver.go | 4 ++-- pkg/proc/gdbserial/gdbserver_conn.go | 7 ++++++- pkg/proc/interface.go | 4 +++- pkg/proc/native/proc.go | 22 ++++++++-------------- pkg/proc/proc_test.go | 26 +++++++------------------- pkg/proc/proc_unix_test.go | 27 ++++++++++++--------------- service/debugger/debugger.go | 3 --- 8 files changed, 39 insertions(+), 57 deletions(-) diff --git a/pkg/proc/core/core.go b/pkg/proc/core/core.go index c0cc84a6..90818263 100644 --- a/pkg/proc/core/core.go +++ b/pkg/proc/core/core.go @@ -304,8 +304,7 @@ func (p *Process) Pid() int { return p.core.Pid } -func (p *Process) Running() bool { - return false +func (p *Process) ResumeNotify(chan<- struct{}) { } func (p *Process) SelectedGoroutine() *proc.G { diff --git a/pkg/proc/gdbserial/gdbserver.go b/pkg/proc/gdbserial/gdbserver.go index 73234a44..5903a6b9 100644 --- a/pkg/proc/gdbserial/gdbserver.go +++ b/pkg/proc/gdbserial/gdbserver.go @@ -490,8 +490,8 @@ func (p *Process) Exited() bool { return p.exited } -func (p *Process) Running() bool { - return p.conn.running +func (p *Process) ResumeNotify(ch chan<- struct{}) { + p.conn.resumeChan = ch } func (p *Process) FindThread(threadID int) (proc.Thread, bool) { diff --git a/pkg/proc/gdbserial/gdbserver_conn.go b/pkg/proc/gdbserial/gdbserver_conn.go index bf6b6924..955cfe0a 100644 --- a/pkg/proc/gdbserial/gdbserver_conn.go +++ b/pkg/proc/gdbserial/gdbserver_conn.go @@ -23,7 +23,8 @@ type gdbConn struct { inbuf []byte outbuf bytes.Buffer - running bool + running bool + resumeChan chan<- struct{} direction proc.Direction // direction of execution @@ -543,6 +544,10 @@ func (conn *gdbConn) resume(sig uint8, tu *threadUpdater) (string, uint8, error) defer func() { conn.running = false }() + if conn.resumeChan != nil { + close(conn.resumeChan) + conn.resumeChan = nil + } return conn.waitForvContStop("resume", "-1", tu) } diff --git a/pkg/proc/interface.go b/pkg/proc/interface.go index 3e013cb7..a926fe11 100644 --- a/pkg/proc/interface.go +++ b/pkg/proc/interface.go @@ -52,8 +52,10 @@ type Checkpoint struct { // Info is an interface that provides general information on the target. type Info interface { Pid() int + // ResumeNotify specifies a channel that will be closed the next time + // ContinueOnce finishes resuming the target. + ResumeNotify(chan<- struct{}) Exited() bool - Running() bool BinInfo() *BinaryInfo ThreadInfo diff --git a/pkg/proc/native/proc.go b/pkg/proc/native/proc.go index 2231eab0..897e86fa 100644 --- a/pkg/proc/native/proc.go +++ b/pkg/proc/native/proc.go @@ -39,6 +39,7 @@ type Process struct { firstStart bool haltMu sync.Mutex halt bool + resumeChan chan<- struct{} exited bool ptraceChan chan func() ptraceDoneChan chan interface{} @@ -89,11 +90,6 @@ func (dbp *Process) Detach(kill bool) (err error) { dbp.bi.Close() return nil } - if dbp.Running() { - if err = dbp.Halt(); err != nil { - return - } - } if !kill { // Clean up any breakpoints we've set. for _, bp := range dbp.breakpoints { @@ -124,15 +120,8 @@ func (dbp *Process) Exited() bool { return dbp.exited } -// Running returns whether the debugged -// process is currently executing. -func (dbp *Process) Running() bool { - for _, th := range dbp.threads { - if th.running { - return true - } - } - return false +func (dbp *Process) ResumeNotify(ch chan<- struct{}) { + dbp.resumeChan = ch } func (dbp *Process) Pid() int { @@ -275,6 +264,11 @@ func (dbp *Process) ContinueOnce() (proc.Thread, error) { th.clearBreakpointState() } + if dbp.resumeChan != nil { + close(dbp.resumeChan) + dbp.resumeChan = nil + } + trapthread, err := dbp.trapWait(-1) if err != nil { return nil, err diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index 292aca2b..fbc36571 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -213,27 +213,22 @@ func TestHalt(t *testing.T) { _, err := setFunctionBreakpoint(p, "main.loop") assertNoError(err, t, "SetBreakpoint") assertNoError(proc.Continue(p), t, "Continue") - if p.Running() { - t.Fatal("process still running") - } if p, ok := p.(*native.Process); ok { for _, th := range p.ThreadList() { _, err := th.Registers(false) assertNoError(err, t, "Registers") } } + resumeChan := make(chan struct{}) go func() { - for { - time.Sleep(100 * time.Millisecond) - if p.Running() { - if err := p.RequestManualStop(); err != nil { - t.Fatal(err) - } - stopChan <- nil - return - } + <-resumeChan + time.Sleep(100 * time.Millisecond) + if err := p.RequestManualStop(); err != nil { + t.Fatal(err) } + stopChan <- nil }() + p.ResumeNotify(resumeChan) assertNoError(proc.Continue(p), t, "Continue") <-stopChan // Loop through threads and make sure they are all @@ -601,9 +596,6 @@ func TestNextNetHTTP(t *testing.T) { } withTestProcess("testnextnethttp", t, func(p proc.Process, fixture protest.Fixture) { go func() { - for !p.Running() { - time.Sleep(50 * time.Millisecond) - } // Wait for program to start listening. for { conn, err := net.Dial("tcp", "localhost:9191") @@ -1934,10 +1926,6 @@ func TestIssue462(t *testing.T) { } withTestProcess("testnextnethttp", t, func(p proc.Process, fixture protest.Fixture) { go func() { - for !p.Running() { - time.Sleep(50 * time.Millisecond) - } - // Wait for program to start listening. for { conn, err := net.Dial("tcp", "localhost:9191") diff --git a/pkg/proc/proc_unix_test.go b/pkg/proc/proc_unix_test.go index 5c06a21f..14ef04ad 100644 --- a/pkg/proc/proc_unix_test.go +++ b/pkg/proc/proc_unix_test.go @@ -25,24 +25,21 @@ func TestIssue419(t *testing.T) { _, err := setFunctionBreakpoint(p, "main.main") assertNoError(err, t, "SetBreakpoint()") assertNoError(proc.Continue(p), t, "Continue()") + resumeChan := make(chan struct{}) go func() { - for { - time.Sleep(500 * time.Millisecond) - if p.Running() { - time.Sleep(2 * time.Second) - if p.Pid() <= 0 { - // if we don't stop the inferior the test will never finish - p.RequestManualStop() - p.Kill() - t.Fatalf("Pid is zero or negative: %d", p.Pid()) - return - } - err := syscall.Kill(p.Pid(), syscall.SIGINT) - assertNoError(err, t, "syscall.Kill") - return - } + time.Sleep(500 * time.Millisecond) + <-resumeChan + if p.Pid() <= 0 { + // if we don't stop the inferior the test will never finish + p.RequestManualStop() + p.Kill() + t.Fatalf("Pid is zero or negative: %d", p.Pid()) + return } + err := syscall.Kill(p.Pid(), syscall.SIGINT) + assertNoError(err, t, "syscall.Kill") }() + p.ResumeNotify(resumeChan) err = proc.Continue(p) if _, exited := err.(proc.ProcessExitedError); !exited { t.Fatalf("Unexpected error after Continue(): %v\n", err) diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go index 4a34c15b..d6940495 100644 --- a/service/debugger/debugger.go +++ b/service/debugger/debugger.go @@ -200,9 +200,6 @@ func (d *Debugger) Restart(pos string) ([]api.DiscardedBreakpoint, error) { } if !d.target.Exited() { - if d.target.Running() { - d.target.Halt() - } // Ensure the process is in a PTRACE_STOP. if err := stopProcess(d.ProcessPid()); err != nil { return nil, err