diff --git a/_fixtures/issue1101.go b/_fixtures/issue1101.go new file mode 100644 index 00000000..634b3a7a --- /dev/null +++ b/_fixtures/issue1101.go @@ -0,0 +1,20 @@ +package main + +import ( + "os" + "sync" +) + +var wg sync.WaitGroup + +func f(from string) { + defer wg.Done() + return +} + +func main() { + wg.Add(1) + go f("goroutine") + wg.Wait() + os.Exit(2) +} diff --git a/pkg/proc/native/proc_linux.go b/pkg/proc/native/proc_linux.go index 3d76820c..e666fecf 100644 --- a/pkg/proc/native/proc_linux.go +++ b/pkg/proc/native/proc_linux.go @@ -230,10 +230,20 @@ func (dbp *Process) trapWait(pid int) (*Thread, error) { if err != nil { if err == sys.ESRCH { // thread died while we were adding it + delete(dbp.threads, int(cloned)) continue } return nil, err } + dbp.haltMu.Lock() + halt := dbp.halt + dbp.haltMu.Unlock() + if halt { + dbp.halt = false + th.running = false + dbp.threads[int(wpid)].running = false + return nil, nil + } if err = th.Continue(); err != nil { if err == sys.ESRCH { // thread died while we were adding it @@ -256,7 +266,7 @@ func (dbp *Process) trapWait(pid int) (*Thread, error) { dbp.haltMu.Lock() halt := dbp.halt dbp.haltMu.Unlock() - if status.StopSignal() == sys.SIGTRAP && halt { + if halt && (status.StopSignal() == sys.SIGTRAP || status.StopSignal() == sys.SIGSTOP) { th.running = false dbp.halt = false return th, nil @@ -368,16 +378,28 @@ func (dbp *Process) wait(pid, options int) (int, *sys.WaitStatus, error) { } func (dbp *Process) setCurrentBreakpoints(trapthread *Thread) error { + // wait for all threads to stop + for { + allstopped := true + for _, th := range dbp.threads { + if th.running { + allstopped = false + break + } + } + if allstopped { + break + } + dbp.halt = true + _, err := dbp.trapWait(-1) + if err != nil { + return err + } + } for _, th := range dbp.threads { if th.CurrentBreakpoint.Breakpoint == nil { if err := th.SetCurrentBreakpoint(); err != nil { - if err == sys.ESRCH { - // This thread quit between the point where we received the breakpoint and - // the stop signal. - delete(dbp.threads, th.ID) - } else { - return err - } + return err } } } diff --git a/pkg/proc/native/threads.go b/pkg/proc/native/threads.go index ef51bca9..26eb02a5 100644 --- a/pkg/proc/native/threads.go +++ b/pkg/proc/native/threads.go @@ -120,22 +120,6 @@ func (thread *Thread) Stopped() bool { return thread.stopped() } -// Halt stops this thread from executing. Actual -// implementation is OS dependant. Look in OS -// thread file. -func (thread *Thread) Halt() (err error) { - defer func() { - if err == nil { - thread.running = false - } - }() - if thread.Stopped() { - return - } - err = thread.halt() - return -} - // SetCurrentBreakpoint sets the current breakpoint that this // thread is stopped at as CurrentBreakpoint on the thread struct. func (thread *Thread) SetCurrentBreakpoint() error { diff --git a/pkg/proc/native/threads_darwin.go b/pkg/proc/native/threads_darwin.go index 0104865f..77d032d8 100644 --- a/pkg/proc/native/threads_darwin.go +++ b/pkg/proc/native/threads_darwin.go @@ -27,6 +27,20 @@ type OSSpecificDetails struct { // be continued. var ErrContinueThread = fmt.Errorf("could not continue thread") +// Halt stops this thread from executing. +func (thread *Thread) Halt() (err error) { + defer func() { + if err == nil { + thread.running = false + } + }() + if thread.Stopped() { + return + } + err = thread.halt() + return +} + func (t *Thread) halt() (err error) { kret := C.thread_suspend(t.os.threadAct) if kret != C.KERN_SUCCESS { diff --git a/pkg/proc/native/threads_linux.go b/pkg/proc/native/threads_linux.go index d388f529..0e40d230 100644 --- a/pkg/proc/native/threads_linux.go +++ b/pkg/proc/native/threads_linux.go @@ -16,17 +16,21 @@ type OSSpecificDetails struct { registers sys.PtraceRegs } +// Halt stops this thread from executing. +func (thread *Thread) Halt() (err error) { + if thread.Stopped() { + return + } + err = thread.halt() + return +} + func (t *Thread) halt() (err error) { err = sys.Tgkill(t.dbp.pid, t.ID, sys.SIGSTOP) if err != nil { err = fmt.Errorf("halt err %s on thread %d", err, t.ID) return } - _, _, err = t.dbp.waitFast(t.ID) - if err != nil { - err = fmt.Errorf("wait err %s on thread %d", err, t.ID) - return - } return } diff --git a/pkg/proc/native/threads_windows.go b/pkg/proc/native/threads_windows.go index 766be120..a0d7f92c 100644 --- a/pkg/proc/native/threads_windows.go +++ b/pkg/proc/native/threads_windows.go @@ -18,6 +18,20 @@ type OSSpecificDetails struct { hThread syscall.Handle } +// Halt stops this thread from executing. +func (thread *Thread) Halt() (err error) { + defer func() { + if err == nil { + thread.running = false + } + }() + if thread.Stopped() { + return + } + err = thread.halt() + return +} + func (t *Thread) halt() (err error) { // Ignore the request to halt. On Windows, all threads are halted // on return from WaitForDebugEvent. diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index ab6c9438..65fd8694 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -3410,3 +3410,45 @@ func TestIssue1137(t *testing.T) { assertNoError(v2.Unreadable, t, "iface2 unreadable") }) } + +func TestIssue1101(t *testing.T) { + // If a breakpoint is hit close to process death on a thread that isn't the + // group leader the process could die while we are trying to stop it. + // + // This can be easily reproduced by having the goroutine that's executing + // main.main (which will almost always run on the thread group leader) wait + // for a second goroutine before exiting, then setting a breakpoint on the + // second goroutine and stepping through it (see TestIssue1101 in + // proc_test.go). + // + // When stepping over the return instruction of main.f the deferred + // wg.Done() call will be executed which will cause the main goroutine to + // resume and proceed to exit. Both the temporary breakpoint on wg.Done and + // the temporary breakpoint on the return address of main.f will be in + // close proximity to main.main calling os.Exit() and causing the death of + // the thread group leader. + + withTestProcess("issue1101", t, func(p proc.Process, fixture protest.Fixture) { + _, err := setFunctionBreakpoint(p, "main.f") + assertNoError(err, t, "setFunctionBreakpoint()") + assertNoError(proc.Continue(p), t, "Continue()") + assertNoError(proc.Next(p), t, "Next() 1") + assertNoError(proc.Next(p), t, "Next() 2") + lastCmd := "Next() 3" + exitErr := proc.Next(p) + if exitErr == nil { + lastCmd = "final Continue()" + exitErr = proc.Continue(p) + } + if pexit, exited := exitErr.(proc.ProcessExitedError); exited { + if pexit.Status != 2 && testBackend != "lldb" { + // looks like there's a bug with debugserver on macOS that sometimes + // will report exit status 0 instead of the proper exit status. + t.Fatalf("process exited status %d (expected 2)", pexit.Status) + } + } else { + assertNoError(exitErr, t, lastCmd) + t.Fatalf("process did not exit after %s", lastCmd) + } + }) +}