proc: move StepInstruction method to TargetGroup (#3488)

Move StepInstruction method to TargetGroup since it will need access to
the process group on Windows to call WaitForDebugEvent.

pkg/proc/core/core.go

@@ -359,12 +359,6 @@ func (t *thread) BinInfo() *proc.BinaryInfo {
 	return t.p.bi
 }
 
-// StepInstruction will only return an error for core files,
-// you cannot execute a core file.
-func (t *thread) StepInstruction() error {
-	return ErrContinueCore
-}
-
 // SetCurrentBreakpoint will always just return nil
 // for core files, as there are no breakpoints in core files.
 func (t *thread) SetCurrentBreakpoint(adjustPC bool) error {
@@ -429,7 +423,7 @@ func (*process) ContinueOnce(cctx *proc.ContinueOnceContext) (proc.Thread, proc.
 
 // StepInstruction will always return an error
 // as you cannot control execution of a core file.
-func (p *process) StepInstruction() error {
+func (p *process) StepInstruction(int) error {
 	return ErrContinueCore
 }
 

pkg/proc/fncall.go

@@ -88,7 +88,8 @@ type functionCallState struct {
 }
 
 type callContext struct {
-	p *Target
+	grp *TargetGroup
+	p   *Target
 
 	// checkEscape is true if the escape check should be performed.
 	// See service/api.DebuggerCommand.UnsafeCall in service/api/types.go.
@@ -183,6 +184,7 @@ func EvalExpressionWithCalls(grp *TargetGroup, g *G, expr string, retLoadCfg Loa
 	continueCompleted := make(chan *G)
 
 	scope.callCtx = &callContext{
+		grp:               grp,
 		p:                 t,
 		checkEscape:       checkEscape,
 		retLoadCfg:        retLoadCfg,
@@ -948,7 +950,8 @@ func funcCallStep(callScope *EvalScope, fncall *functionCallState, thread Thread
 			fncall.err = fmt.Errorf("could not restore SP: %v", err)
 		}
 		fncallLog("stepping thread %d", thread.ThreadID())
-		if err := stepInstructionOut(p, thread, debugCallName, debugCallName); err != nil {
+		
+		if err := stepInstructionOut(callScope.callCtx.grp, p, thread, debugCallName, debugCallName); err != nil {
 			fncall.err = fmt.Errorf("could not step out of %s: %v", debugCallName, err)
 		}
 		if bi.Arch.Name == "amd64" {

pkg/proc/gdbserial/gdbserver.go

@@ -852,7 +852,7 @@ func (p *gdbProcess) ContinueOnce(cctx *proc.ContinueOnceContext) (proc.Thread,
 		// step threads stopped at any breakpoint over their breakpoint
 		for _, thread := range p.threads {
 			if thread.CurrentBreakpoint.Breakpoint != nil {
-				if err := thread.StepInstruction(); err != nil {
+				if err := thread.stepInstruction(); err != nil {
 					return nil, proc.StopUnknown, err
 				}
 			}
@@ -1486,6 +1486,10 @@ func (p *gdbProcess) WriteMemory(addr uint64, data []byte) (written int, err err
 	return p.conn.writeMemory(addr, data)
 }
 
+func (p *gdbProcess) StepInstruction(threadID int) error {
+	return p.threads[threadID].stepInstruction()
+}
+
 func (t *gdbThread) ProcessMemory() proc.MemoryReadWriter {
 	return t.p
 }
@@ -1543,7 +1547,7 @@ func (t *gdbThread) Common() *proc.CommonThread {
 }
 
 // StepInstruction will step exactly 1 CPU instruction.
-func (t *gdbThread) StepInstruction() error {
+func (t *gdbThread) stepInstruction() error {
 	pc := t.regs.PC()
 	if bp, atbp := t.p.breakpoints.M[pc]; atbp && bp.WatchType == 0 {
 		err := t.p.conn.clearBreakpoint(pc, swBreakpoint, t.p.breakpointKind)

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)
+	StepInstruction(int) error
 	Detach(int, bool) error
 }
 

pkg/proc/native/nonative_darwin.go

@@ -123,7 +123,7 @@ func (t *nativeThread) resume() error {
 	panic(ErrNativeBackendDisabled)
 }
 
-func (t *nativeThread) singleStep() error {
+func (*processGroup) singleStep(*nativeThread) error {
 	panic(ErrNativeBackendDisabled)
 }
 

pkg/proc/native/proc_darwin.go

@@ -109,7 +109,8 @@ func Launch(cmd []string, wd string, flags proc.LaunchFlags, _ []string, _ strin
 		}
 	}
 
-	if err := dbp.resume(); err != nil {
+	procgrp := &processGroup{procs: []*nativeProcess{dbp}}
+	if err := procgrp.resume(); err != nil {
 		return nil, err
 	}
 
@@ -367,7 +368,7 @@ func (dbp *nativeProcess) trapWait(pid int) (*nativeThread, error) {
 				dbp.firstStart = false
 				return th, nil
 			}
-			if err := th.Continue(); err != nil {
+			if err := th.resume(); err != nil {
 				return nil, err
 			}
 			continue
@@ -421,14 +422,11 @@ func (dbp *nativeProcess) exitGuard(err error) error {
 }
 
 func (procgrp *processGroup) resume() error {
-	return procgrp.procs[0].resume()
+	dbp := procgrp.procs[0]
-}
-
-func (dbp *nativeProcess) 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 {
+			if err := procgrp.stepInstruction(thread); err != nil {
 				return err
 			}
 			thread.CurrentBreakpoint.Clear()

pkg/proc/native/proc_freebsd.go

@@ -430,7 +430,7 @@ 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 {
+			if err := procgrp.stepInstruction(thread); err != nil {
 				return err
 			}
 			thread.CurrentBreakpoint.Clear()

pkg/proc/native/proc_linux.go

@@ -490,7 +490,7 @@ func trapWaitInternal(procgrp *processGroup, pid int, options trapWaitOptions) (
 				dbp.threads[int(wpid)].os.running = false
 				return nil, nil
 			}
-			if err = th.Continue(); err != nil {
+			if err = th.resume(); err != nil {
 				if err == sys.ESRCH {
 					// thread died while we were adding it
 					delete(dbp.threads, th.ID)
@@ -498,7 +498,7 @@ func trapWaitInternal(procgrp *processGroup, pid int, options trapWaitOptions) (
 				}
 				return nil, fmt.Errorf("could not continue new thread %d %s", cloned, err)
 			}
-			if err = dbp.threads[int(wpid)].Continue(); err != nil {
+			if err = dbp.threads[int(wpid)].resume(); err != nil {
 				if err != sys.ESRCH {
 					return nil, fmt.Errorf("could not continue existing thread %d %s", wpid, err)
 				}
@@ -527,7 +527,7 @@ func trapWaitInternal(procgrp *processGroup, pid int, options trapWaitOptions) (
 			if tgt != nil {
 				// If tgt is nil we decided we are not interested in debugging this
 				// process, and we have already detached from it.
-				err = dbp.threads[dbp.pid].Continue()
+				err = dbp.threads[dbp.pid].resume()
 				if err != nil {
 					return nil, err
 				}
@@ -653,7 +653,7 @@ func (procgrp *processGroup) resume() error {
 		if valid, _ := dbp.Valid(); valid {
 			for _, thread := range dbp.threads {
 				if thread.CurrentBreakpoint.Breakpoint != nil {
-					if err := thread.StepInstruction(); err != nil {
+					if err := procgrp.stepInstruction(thread); err != nil {
 						return err
 					}
 					thread.CurrentBreakpoint.Clear()

pkg/proc/native/proc_windows.go

@@ -504,7 +504,7 @@ 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 {
+			if err := procgrp.stepInstruction(thread); err != nil {
 				return err
 			}
 			thread.CurrentBreakpoint.Clear()

pkg/proc/native/threads.go

@@ -22,13 +22,17 @@ type nativeThread struct {
 	common         proc.CommonThread
 }
 
+func (procgrp *processGroup) StepInstruction(threadID int) error {
+	return procgrp.stepInstruction(procgrp.procForThread(threadID).threads[threadID])
+}
+
 // StepInstruction steps a single instruction.
 //
 // Executes exactly one instruction and then returns.
 // If the thread is at a breakpoint, we first clear it,
 // execute the instruction, and then replace the breakpoint.
 // Otherwise we simply execute the next instruction.
-func (t *nativeThread) StepInstruction() (err error) {
+func (procgrp *processGroup) stepInstruction(t *nativeThread) (err error) {
 	t.singleStepping = true
 	defer func() {
 		t.singleStepping = false
@@ -63,7 +67,7 @@ func (t *nativeThread) StepInstruction() (err error) {
 		}()
 	}
 
-	err = t.singleStep()
+	err = procgrp.singleStep(t)
 	if err != nil {
 		if _, exited := err.(proc.ErrProcessExited); exited {
 			return err

pkg/proc/native/threads_darwin.go

@@ -51,7 +51,7 @@ func (t *nativeThread) stop() (err error) {
 	return
 }
 
-func (t *nativeThread) singleStep() error {
+func (procgrp *processGroup) singleStep(t *nativeThread) error {
 	kret := C.single_step(t.os.threadAct)
 	if kret != C.KERN_SUCCESS {
 		return fmt.Errorf("could not single step")
@@ -143,23 +143,3 @@ func (t *nativeThread) withDebugRegisters(f func(*amd64util.DebugRegisters) erro
 func (t *nativeThread) SoftExc() bool {
 	return false
 }
-
-// Continue the execution of this thread.
-//
-// If we are currently at a breakpoint, we'll clear it
-// first and then resume execution. Thread will continue until
-// it hits a breakpoint or is signaled.
-func (t *nativeThread) Continue() error {
-	pc, err := t.PC()
-	if err != nil {
-		return err
-	}
-	// Check whether we are stopped at a breakpoint, and
-	// if so, single step over it before continuing.
-	if _, ok := t.dbp.FindBreakpoint(pc, false); ok {
-		if err := t.StepInstruction(); err != nil {
-			return err
-		}
-	}
-	return t.resume()
-}

pkg/proc/native/threads_freebsd.go

@@ -17,7 +17,7 @@ type osSpecificDetails struct {
 	registers sys.Reg
 }
 
-func (t *nativeThread) singleStep() (err error) {
+func (procgrp *processGroup) singleStep(t *nativeThread) (err error) {
 	t.dbp.execPtraceFunc(func() { err = ptraceSetStep(t.ID) })
 	if err != nil {
 		return err

pkg/proc/native/threads_linux.go

@@ -50,7 +50,7 @@ func (t *nativeThread) resumeWithSig(sig int) (err error) {
 	return
 }
 
-func (t *nativeThread) singleStep() (err error) {
+func (procgrp *processGroup) singleStep(t *nativeThread) (err error) {
 	sig := 0
 	for {
 		t.dbp.execPtraceFunc(func() { err = ptraceSingleStep(t.ID, sig) })
@@ -123,23 +123,3 @@ func (t *nativeThread) ReadMemory(data []byte, addr uint64) (n int, err error) {
 func (t *nativeThread) SoftExc() bool {
 	return t.os.setbp
 }
-
-// Continue the execution of this thread.
-//
-// If we are currently at a breakpoint, we'll clear it
-// first and then resume execution. Thread will continue until
-// it hits a breakpoint or is signaled.
-func (t *nativeThread) Continue() error {
-	pc, err := t.PC()
-	if err != nil {
-		return err
-	}
-	// Check whether we are stopped at a breakpoint, and
-	// if so, single step over it before continuing.
-	if _, ok := t.dbp.FindBreakpoint(pc, false); ok {
-		if err := t.StepInstruction(); err != nil {
-			return err
-		}
-	}
-	return t.resume()
-}

pkg/proc/native/threads_windows.go

@@ -23,7 +23,7 @@ type osSpecificDetails struct {
 	setbp              bool
 }
 
-func (t *nativeThread) singleStep() error {
+func (procgrp *processGroup) singleStep(t *nativeThread) error {
 	context := newContext()
 	context.SetFlags(_CONTEXT_ALL)
 

pkg/proc/proc_test.go

@@ -324,7 +324,7 @@ func TestStep(t *testing.T) {
 		regs := getRegisters(p, t)
 		rip := regs.PC()
 
-		err := p.CurrentThread().StepInstruction()
+		err := grp.StepInstruction()
 		assertNoError(err, t, "Step()")
 
 		regs = getRegisters(p, t)
@@ -5765,7 +5765,7 @@ func TestSetYMMRegister(t *testing.T) {
 			0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44,
 			0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44,
 			0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44}))
-		assertNoError(p.CurrentThread().StepInstruction(), t, "SetpInstruction")
+		assertNoError(grp.StepInstruction(), t, "SetpInstruction")
 
 		xmm0 := getReg("after")
 

pkg/proc/target_exec.go

@@ -145,7 +145,7 @@ func (grp *TargetGroup) Continue() error {
 			if callErrThis != nil && callErr == nil {
 				callErr = callErrThis
 			}
-			hcbpErrThis := dbp.handleHardcodedBreakpoints(trapthread, threads)
+			hcbpErrThis := dbp.handleHardcodedBreakpoints(grp, trapthread, threads)
 			if hcbpErrThis != nil && hcbpErr == nil {
 				hcbpErr = hcbpErrThis
 			}
@@ -396,10 +396,10 @@ func disassembleCurrentInstruction(p Process, thread Thread, off int64) ([]AsmIn
 // function is neither fnname1 or fnname2.
 // This function is used to step out of runtime.Breakpoint as well as
 // runtime.debugCallV1.
-func stepInstructionOut(dbp *Target, curthread Thread, fnname1, fnname2 string) error {
+func stepInstructionOut(grp *TargetGroup, dbp *Target, curthread Thread, fnname1, fnname2 string) error {
 	defer dbp.ClearCaches()
 	for {
-		if err := curthread.StepInstruction(); err != nil {
+		if err := grp.procgrp.StepInstruction(curthread.ThreadID()); err != nil {
 			return err
 		}
 		loc, err := curthread.Location()
@@ -577,7 +577,7 @@ func (grp *TargetGroup) StepInstruction() (err error) {
 	if ok, err := dbp.Valid(); !ok {
 		return err
 	}
-	err = thread.StepInstruction()
+	err = grp.procgrp.StepInstruction(thread.ThreadID())
 	if err != nil {
 		return err
 	}
@@ -1285,7 +1285,7 @@ func (t *Target) clearHardcodedBreakpoints() {
 // program's text) and sets a fake breakpoint on them with logical id
 // hardcodedBreakpointID.
 // It checks trapthread and all threads that have SoftExc returning true.
-func (t *Target) handleHardcodedBreakpoints(trapthread Thread, threads []Thread) error {
+func (t *Target) handleHardcodedBreakpoints(grp *TargetGroup, trapthread Thread, threads []Thread) error {
 	mem := t.Memory()
 	arch := t.BinInfo().Arch
 	recorded, _ := t.recman.Recorded()
@@ -1366,7 +1366,7 @@ func (t *Target) handleHardcodedBreakpoints(trapthread Thread, threads []Thread)
 			// runtime.Breakpoint.
 			// On go < 1.8 it was sufficient to single-step twice on go1.8 a change
 			// to the compiler requires 4 steps.
-			if err := stepInstructionOut(t, thread, "runtime.breakpoint", "runtime.Breakpoint"); err != nil {
+			if err := stepInstructionOut(grp, t, thread, "runtime.breakpoint", "runtime.Breakpoint"); err != nil {
 				return err
 			}
 			setHardcodedBreakpoint(thread, loc)

pkg/proc/threads.go

@@ -27,7 +27,6 @@ type Thread interface {
 	BinInfo() *BinaryInfo
 	// ProcessMemory returns the process memory.
 	ProcessMemory() MemoryReadWriter
-	StepInstruction() error
 	// SetCurrentBreakpoint updates the current breakpoint of this thread, if adjustPC is true also checks for breakpoints that were just hit (this should only be passed true after a thread resume)
 	SetCurrentBreakpoint(adjustPC bool) error
 	// SoftExc returns true if this thread received a software exception during the last resume.