proc: refactorings to implement follow-exec mode on Windows (#3441)

Miscellaneous non-functional changes to prepare for adding support for
follow-exec mode on Windows:

- removed (*nativeProcess).wait function from Windows backend (unused).
- move close of ptraceDoneChan from release to handlePtraceFuncs, this
  makes postExit callable by a function executed by execPtraceFunc.
- change addTarget to detach before creating the target object if we
  don't actually want to attach to the child process, also moved the
  detach call to (*processGroup).add instead of having one in addTarget
  and one in the code that calls (*processGroup).add.
- changed Detach to be a method of TargetGroup/ProcessGroup, the
  Windows backend will need access to the process group to call
  WaitForDebugEvent.
- moved resume method to processGroup. First all threads stopped at a
  breakpoint need to be stepped, then all other threads can be resumed.
  This is true also for linux even though it didn't cause the current
  tests to fail.

pkg/proc/core/core.go

@@ -454,7 +454,7 @@ func (p *process) Memory() proc.MemoryReadWriter {
 // Detach will always return nil and have no
 // effect as you cannot detach from a core file
 // and have it continue execution or exit.
-func (p *process) Detach(bool) error {
+func (p *process) Detach(int, bool) error {
 	return nil
 }
 

pkg/proc/gdbserial/gdbserver.go

@@ -749,7 +749,7 @@ func (p *gdbProcess) initialize(path, cmdline string, debugInfoDirs []string, st
 	})
 	_, err = addTarget(p, p.conn.pid, p.currentThread, path, stopReason, cmdline)
 	if err != nil {
-		p.Detach(true)
+		p.Detach(p.conn.pid, true)
 		return nil, err
 	}
 	return grp, nil
@@ -1059,7 +1059,9 @@ func (p *gdbProcess) getCtrlC(cctx *proc.ContinueOnceContext) bool {
 
 // Detach will detach from the target process,
 // if 'kill' is true it will also kill the process.
-func (p *gdbProcess) Detach(kill bool) error {
+// The _pid argument is unused as follow exec
+// mode is not implemented with this backend.
+func (p *gdbProcess) Detach(_pid int, kill bool) error {
 	if kill && !p.exited {
 		err := p.conn.kill()
 		if err != nil {

pkg/proc/interface.go

@@ -11,6 +11,7 @@ import (
 // ProcessGroup is a group of processes that are resumed at the same time.
 type ProcessGroup interface {
 	ContinueOnce(*ContinueOnceContext) (Thread, StopReason, error)
+	Detach(int, bool) error
 }
 
 // Process represents the target of the debugger. This
@@ -43,7 +44,6 @@ type ProcessInternal interface {
 	// also returns an error describing why the Process is invalid (either
 	// ErrProcessExited or ErrProcessDetached).
 	Valid() (bool, error)
-	Detach(bool) error
 
 	// RequestManualStop attempts to stop all the process' threads.
 	RequestManualStop(cctx *ContinueOnceContext) error

pkg/proc/native/nonative_darwin.go

@@ -57,7 +57,7 @@ func (dbp *nativeProcess) requestManualStop() (err error) {
 	panic(ErrNativeBackendDisabled)
 }
 
-func (dbp *nativeProcess) resume() error {
+func (*processGroup) resume() error {
 	panic(ErrNativeBackendDisabled)
 }
 
@@ -73,7 +73,7 @@ func (dbp *nativeProcess) updateThreadList() error {
 	panic(ErrNativeBackendDisabled)
 }
 
-func (dbp *nativeProcess) kill() (err error) {
+func (*processGroup) kill(dbp *nativeProcess) (err error) {
 	panic(ErrNativeBackendDisabled)
 }
 

pkg/proc/native/proc.go

@@ -96,17 +96,24 @@ func (dbp *nativeProcess) BinInfo() *proc.BinaryInfo {
 // StartCallInjection notifies the backend that we are about to inject a function call.
 func (dbp *nativeProcess) StartCallInjection() (func(), error) { return func() {}, nil }
 
+// detachWithoutGroup is a helper function to detach from a process which we
+// haven't added to a process group yet.
+func detachWithoutGroup(dbp *nativeProcess, kill bool) error {
+	grp := &processGroup{procs: []*nativeProcess{dbp}}
+	return grp.Detach(dbp.pid, kill)
+}
+
 // Detach from the process being debugged, optionally killing it.
-func (dbp *nativeProcess) Detach(kill bool) (err error) {
+func (procgrp *processGroup) Detach(pid int, kill bool) (err error) {
+	dbp := procgrp.procForPid(pid)
 	if dbp.exited {
 		return nil
 	}
 	if kill && dbp.childProcess {
-		err := dbp.kill()
+		err := procgrp.kill(dbp)
 		if err != nil {
 			return err
 		}
-		dbp.bi.Close()
 		return nil
 	}
 	dbp.execPtraceFunc(func() {
@@ -226,8 +233,25 @@ func (procgrp *processGroup) procForThread(tid int) *nativeProcess {
 	return nil
 }
 
+func (procgrp *processGroup) procForPid(pid int) *nativeProcess {
+	for _, p := range procgrp.procs {
+		if p.pid == pid {
+			return p
+		}
+	}
+	return nil
+}
+
 func (procgrp *processGroup) add(p *nativeProcess, pid int, currentThread proc.Thread, path string, stopReason proc.StopReason, cmdline string) (*proc.Target, error) {
 	tgt, err := procgrp.addTarget(p, pid, currentThread, path, stopReason, cmdline)
+	if tgt == nil {
+		i := len(procgrp.procs)
+		procgrp.procs = append(procgrp.procs, p)
+		procgrp.Detach(p.pid, false)
+		if i == len(procgrp.procs)-1 {
+			procgrp.procs = procgrp.procs[:i]
+		}
+	}
 	if err != nil {
 		return nil, err
 	}
@@ -246,15 +270,15 @@ func (procgrp *processGroup) ContinueOnce(cctx *proc.ContinueOnceContext) (proc.
 	}
 
 	for {
+		err := procgrp.resume()
+		if err != nil {
+			return nil, proc.StopUnknown, err
+		}
 		for _, dbp := range procgrp.procs {
-			if dbp.exited {
+			if valid, _ := dbp.Valid(); valid {
-				continue
+				for _, th := range dbp.threads {
-			}
+					th.CurrentBreakpoint.Clear()
-			if err := dbp.resume(); err != nil {
+				}
-				return nil, proc.StopUnknown, err
-			}
-			for _, th := range dbp.threads {
-				th.CurrentBreakpoint.Clear()
 			}
 		}
 
@@ -377,6 +401,7 @@ func (pt *ptraceThread) handlePtraceFuncs() {
 		fn()
 		pt.ptraceDoneChan <- nil
 	}
+	close(pt.ptraceDoneChan)
 }
 
 func (dbp *nativeProcess) execPtraceFunc(fn func()) {
@@ -473,6 +498,5 @@ func (pt *ptraceThread) release() {
 	pt.ptraceRefCnt--
 	if pt.ptraceRefCnt == 0 {
 		close(pt.ptraceChan)
-		close(pt.ptraceDoneChan)
 	}
 }

pkg/proc/native/proc_darwin.go

@@ -71,7 +71,7 @@ func Launch(cmd []string, wd string, flags proc.LaunchFlags, _ []string, _ strin
 	dbp := newProcess(0)
 	defer func() {
 		if err != nil && dbp.pid != 0 {
-			_ = dbp.Detach(true)
+			_ = detachWithoutGroup(dbp, true)
 		}
 	}()
 	var pid int
@@ -172,14 +172,14 @@ func Attach(pid int, waitFor *proc.WaitFor, _ []string) (*proc.TargetGroup, erro
 
 	tgt, err := dbp.initialize("", []string{})
 	if err != nil {
-		dbp.Detach(false)
+		detachWithoutGroup(dbp, false)
 		return nil, err
 	}
 	return tgt, nil
 }
 
 // Kill kills the process.
-func (dbp *nativeProcess) kill() (err error) {
+func (procgrp *processGroup) kill(dbp *nativeProcess) (err error) {
 	if dbp.exited {
 		return nil
 	}
@@ -420,6 +420,10 @@ func (dbp *nativeProcess) exitGuard(err error) error {
 	return err
 }
 
+func (procgrp *processGroup) resume() error {
+	return procgrp.procs[0].resume()
+}
+
 func (dbp *nativeProcess) resume() error {
 	// all threads stopped over a breakpoint are made to step over it
 	for _, thread := range dbp.threads {

pkg/proc/native/proc_freebsd.go

@@ -79,7 +79,7 @@ func Launch(cmd []string, wd string, flags proc.LaunchFlags, debugInfoDirs []str
 	dbp := newProcess(0)
 	defer func() {
 		if err != nil && dbp.pid != 0 {
-			_ = dbp.Detach(true)
+			_ = detachWithoutGroup(dbp, true)
 		}
 	}()
 	dbp.execPtraceFunc(func() {
@@ -146,7 +146,7 @@ func Attach(pid int, waitFor *proc.WaitFor, debugInfoDirs []string) (*proc.Targe
 
 	tgt, err := dbp.initialize(findExecutable("", dbp.pid), debugInfoDirs)
 	if err != nil {
-		dbp.Detach(false)
+		detachWithoutGroup(dbp, false)
 		return nil, err
 	}
 	return tgt, nil
@@ -186,7 +186,7 @@ func initialize(dbp *nativeProcess) (string, error) {
 }
 
 // kill kills the target process.
-func (dbp *nativeProcess) kill() (err error) {
+func (procgrp *processGroup) kill(dbp *nativeProcess) (err error) {
 	if dbp.exited {
 		return nil
 	}
@@ -425,7 +425,8 @@ func (dbp *nativeProcess) exitGuard(err error) error {
 }
 
 // Used by ContinueOnce
-func (dbp *nativeProcess) resume() error {
+func (procgrp *processGroup) resume() error {
+	dbp := procgrp.procs[0]
 	// all threads stopped over a breakpoint are made to step over it
 	for _, thread := range dbp.threads {
 		if thread.CurrentBreakpoint.Breakpoint != nil {

pkg/proc/native/proc_linux.go

@@ -85,7 +85,7 @@ func Launch(cmd []string, wd string, flags proc.LaunchFlags, debugInfoDirs []str
 	dbp := newProcess(0)
 	defer func() {
 		if err != nil && dbp.pid != 0 {
-			_ = dbp.Detach(true)
+			_ = detachWithoutGroup(dbp, true)
 		}
 	}()
 	dbp.execPtraceFunc(func() {
@@ -165,7 +165,7 @@ func Attach(pid int, waitFor *proc.WaitFor, debugInfoDirs []string) (*proc.Targe
 
 	tgt, err := dbp.initialize(findExecutable("", dbp.pid), debugInfoDirs)
 	if err != nil {
-		_ = dbp.Detach(false)
+		_ = detachWithoutGroup(dbp, false)
 		return nil, err
 	}
 
@@ -261,7 +261,7 @@ func (dbp *nativeProcess) GetBufferedTracepoints() []ebpf.RawUProbeParams {
 }
 
 // kill kills the target process.
-func (dbp *nativeProcess) kill() error {
+func (procgrp *processGroup) kill(dbp *nativeProcess) error {
 	if dbp.exited {
 		return nil
 	}
@@ -519,7 +519,6 @@ func trapWaitInternal(procgrp *processGroup, pid int, options trapWaitOptions) (
 			cmdline, _ := dbp.initializeBasic()
 			tgt, err := procgrp.add(dbp, dbp.pid, dbp.memthread, findExecutable("", dbp.pid), proc.StopLaunched, cmdline)
 			if err != nil {
-				_ = dbp.Detach(false)
 				return nil, err
 			}
 			if halt {
@@ -648,20 +647,28 @@ func exitGuard(dbp *nativeProcess, procgrp *processGroup, err error) error {
 	return err
 }
 
-func (dbp *nativeProcess) resume() error {
+func (procgrp *processGroup) resume() error {
 	// all threads stopped over a breakpoint are made to step over it
-	for _, thread := range dbp.threads {
+	for _, dbp := range procgrp.procs {
-		if thread.CurrentBreakpoint.Breakpoint != nil {
+		if valid, _ := dbp.Valid(); valid {
-			if err := thread.StepInstruction(); err != nil {
+			for _, thread := range dbp.threads {
-				return err
+				if thread.CurrentBreakpoint.Breakpoint != nil {
+					if err := thread.StepInstruction(); err != nil {
+						return err
+					}
+					thread.CurrentBreakpoint.Clear()
+				}
 			}
-			thread.CurrentBreakpoint.Clear()
 		}
 	}
 	// everything is resumed
-	for _, thread := range dbp.threads {
+	for _, dbp := range procgrp.procs {
-		if err := thread.resume(); err != nil && err != sys.ESRCH {
+		if valid, _ := dbp.Valid(); valid {
-			return err
+			for _, thread := range dbp.threads {
+				if err := thread.resume(); err != nil && err != sys.ESRCH {
+					return err
+				}
+			}
 		}
 	}
 	return nil

pkg/proc/native/proc_windows.go

@@ -65,7 +65,7 @@ func Launch(cmd []string, wd string, flags proc.LaunchFlags, _ []string, _ strin
 
 	tgt, err := dbp.initialize(argv0Go, []string{})
 	if err != nil {
-		dbp.Detach(true)
+		detachWithoutGroup(dbp, true)
 		return nil, err
 	}
 	return tgt, nil
@@ -183,7 +183,7 @@ func Attach(pid int, waitFor *proc.WaitFor, _ []string) (*proc.TargetGroup, erro
 	}
 	tgt, err := dbp.initialize(exepath, []string{})
 	if err != nil {
-		dbp.Detach(true)
+		detachWithoutGroup(dbp, true)
 		return nil, err
 	}
 	return tgt, nil
@@ -261,7 +261,7 @@ func waitForSearchProcess(pfx string, seen map[int]struct{}) (int, error) {
 }
 
 // kill kills the process.
-func (dbp *nativeProcess) kill() error {
+func (procgrp *processGroup) kill(dbp *nativeProcess) error {
 	if dbp.exited {
 		return nil
 	}
@@ -496,15 +496,12 @@ func trapWait(procgrp *processGroup, pid int) (*nativeThread, error) {
 	return th, nil
 }
 
-func (dbp *nativeProcess) wait(pid, options int) (int, *sys.WaitStatus, error) {
-	return 0, nil, fmt.Errorf("not implemented: wait")
-}
-
 func (dbp *nativeProcess) exitGuard(err error) error {
 	return err
 }
 
-func (dbp *nativeProcess) resume() error {
+func (procgrp *processGroup) resume() error {
+	dbp := procgrp.procs[0]
 	for _, thread := range dbp.threads {
 		if thread.CurrentBreakpoint.Breakpoint != nil {
 			if err := thread.StepInstruction(); err != nil {

pkg/proc/target.go

@@ -323,28 +323,6 @@ func (t *Target) SwitchThread(tid int) error {
 	return fmt.Errorf("thread %d does not exist", tid)
 }
 
-// detach will detach the target from the underlying process.
-// This means the debugger will no longer receive events from the process
-// we were previously debugging.
-// If kill is true then the process will be killed when we detach.
-func (t *Target) detach(kill bool) error {
-	if !kill {
-		if t.asyncPreemptChanged {
-			setAsyncPreemptOff(t, t.asyncPreemptOff)
-		}
-		for _, bp := range t.Breakpoints().M {
-			if bp != nil {
-				err := t.ClearBreakpoint(bp.Addr)
-				if err != nil {
-					return err
-				}
-			}
-		}
-	}
-	t.StopReason = StopUnknown
-	return t.proc.Detach(kill)
-}
-
 // setAsyncPreemptOff enables or disables async goroutine preemption by
 // writing the value 'v' to runtime.debug.asyncpreemptoff.
 // A value of '1' means off, a value of '0' means on.

pkg/proc/target_group.go

@@ -97,18 +97,21 @@ func Restart(grp, oldgrp *TargetGroup, discard func(*LogicalBreakpoint, error))
 
 func (grp *TargetGroup) addTarget(p ProcessInternal, pid int, currentThread Thread, path string, stopReason StopReason, cmdline string) (*Target, error) {
 	logger := logflags.DebuggerLogger()
+	if len(grp.targets) > 0 {
+		if !grp.followExecEnabled {
+			logger.Debugf("Detaching from child target (follow-exec disabled) %d %q", pid, cmdline)
+			return nil, nil
+		}
+		if grp.followExecRegex != nil && !grp.followExecRegex.MatchString(cmdline) {
+			logger.Debugf("Detaching from child target (follow-exec regex not matched) %d %q", pid, cmdline)
+			return nil, nil
+		}
+	}
 	t, err := grp.newTarget(p, pid, currentThread, path, cmdline)
 	if err != nil {
 		return nil, err
 	}
 	t.StopReason = stopReason
-	if grp.followExecRegex != nil && len(grp.targets) > 0 {
-		if !grp.followExecRegex.MatchString(cmdline) {
-			logger.Debugf("Detaching from child target %d %q", t.Pid(), t.CmdLine)
-			t.detach(false)
-			return nil, nil
-		}
-	}
 	logger.Debugf("Adding target %d %q", t.Pid(), t.CmdLine)
 	if t.partOfGroup {
 		panic("internal error: target is already part of group")
@@ -178,7 +181,7 @@ func (grp *TargetGroup) Detach(kill bool) error {
 		if !isvalid {
 			continue
 		}
-		err := t.detach(kill)
+		err := grp.detachTarget(t, kill)
 		if err != nil {
 			errs = append(errs, fmt.Sprintf("could not detach process %d: %v", t.Pid(), err))
 		}
@@ -189,6 +192,28 @@ func (grp *TargetGroup) Detach(kill bool) error {
 	return nil
 }
 
+// detachTarget will detach the target from the underlying process.
+// This means the debugger will no longer receive events from the process
+// we were previously debugging.
+// If kill is true then the process will be killed when we detach.
+func (grp *TargetGroup) detachTarget(t *Target, kill bool) error {
+	if !kill {
+		if t.asyncPreemptChanged {
+			setAsyncPreemptOff(t, t.asyncPreemptOff)
+		}
+		for _, bp := range t.Breakpoints().M {
+			if bp != nil {
+				err := t.ClearBreakpoint(bp.Addr)
+				if err != nil {
+					return err
+				}
+			}
+		}
+	}
+	t.StopReason = StopUnknown
+	return grp.procgrp.Detach(t.Pid(), kill)
+}
+
 // HasSteppingBreakpoints returns true if any of the targets has stepping breakpoints set.
 func (grp *TargetGroup) HasSteppingBreakpoints() bool {
 	for _, t := range grp.targets {