Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/proc/core/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
6 changes: 4 additions & 2 deletions pkg/proc/gdbserial/gdbserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,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
Expand Down Expand Up @@ -1058,7 +1058,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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/proc/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/proc/native/nonative_darwin.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (dbp *nativeProcess) requestManualStop() (err error) {
panic(ErrNativeBackendDisabled)
}

func (dbp *nativeProcess) resume() error {
func (*processGroup) resume() error {
panic(ErrNativeBackendDisabled)
}

Expand All @@ -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)
}

Expand Down
48 changes: 36 additions & 12 deletions pkg/proc/native/proc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. The process group doesn't have access to the target object and (*processGroup).Detach is in a different package and can't call private methods of proc.Target.

Quoting the reply from the other review comment. I should have put the comment against this change instead. Why not leave the (nativeProcess) Detach) method and add the (processGroup) one which simply calls nativeProcess.Detach, so the code paths which now use detachWithoutGroup can remain unchanged without having to jump through this hoop to detach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to move the kill method to processGroup because on Windows it needs it to call waitForDebugEvent:

func (procgrp *processGroup) kill(dbp *nativeProcess) error {
	if dbp.exited {
		return nil
	}

	p, err := os.FindProcess(dbp.pid)
	if err != nil {
		return err
	}
	defer p.Release()

	// TODO: Should not have to ignore failures here,
	// but some tests appear to Kill twice causing
	// this to fail on second attempt.
	_ = syscall.TerminateProcess(dbp.os.hProcess, 1)

	dbp.execPtraceFunc(func() {
		procgrp.waitForDebugEvent(waitBlocking | waitDontHandleExceptions)
	})

	p.Wait()

	dbp.postExit()
	return nil
}

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() {
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
continue
}
if err := dbp.resume(); err != nil {
return nil, proc.StopUnknown, err
}
for _, th := range dbp.threads {
th.CurrentBreakpoint.Clear()
if valid, _ := dbp.Valid(); valid {
for _, th := range dbp.threads {
th.CurrentBreakpoint.Clear()
}
}
}

Expand Down Expand Up @@ -377,6 +401,7 @@ func (pt *ptraceThread) handlePtraceFuncs() {
fn()
pt.ptraceDoneChan <- nil
}
close(pt.ptraceDoneChan)
}

func (dbp *nativeProcess) execPtraceFunc(fn func()) {
Expand Down Expand Up @@ -473,6 +498,5 @@ func (pt *ptraceThread) release() {
pt.ptraceRefCnt--
if pt.ptraceRefCnt == 0 {
close(pt.ptraceChan)
close(pt.ptraceDoneChan)
}
}
10 changes: 7 additions & 3 deletions pkg/proc/native/proc_darwin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
9 changes: 5 additions & 4 deletions pkg/proc/native/proc_freebsd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
33 changes: 20 additions & 13 deletions pkg/proc/native/proc_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
if thread.CurrentBreakpoint.Breakpoint != nil {
if err := thread.StepInstruction(); err != nil {
return err
for _, dbp := range procgrp.procs {
if valid, _ := dbp.Valid(); valid {
for _, thread := range dbp.threads {
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 {
if err := thread.resume(); err != nil && err != sys.ESRCH {
return err
for _, dbp := range procgrp.procs {
if valid, _ := dbp.Valid(); valid {
for _, thread := range dbp.threads {
if err := thread.resume(); err != nil && err != sys.ESRCH {
return err
}
}
}
}
return nil
Expand Down
13 changes: 5 additions & 8 deletions pkg/proc/native/proc_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
22 changes: 0 additions & 22 deletions pkg/proc/target.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,28 +323,6 @@ func (p *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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me we could simply keep this, call it in the process group detach, and then we wouldn't need detachWithoutGroup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. The process group doesn't have access to the target object and (*processGroup).Detach is in a different package and can't call private methods of proc.Target.

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.
Expand Down
Loading