proc: move breakpoint condition evaluation out of backends (#2628)

* proc: move breakpoint condition evaluation out of backends

Moves breakpoint condition evaluation from the point where breakpoints
are set, inside ContinueOnce, to (*Target).Continue.

This accomplishes three things:

1. the breakpoint evaluation method needs not be exported anymore
2. breakpoint condition evaluation can be done with a full scope,
   containing a Target object, something that wasn't possible before
   because ContinueOnce doesn't have access to the Target object.
3. moves breakpoint condition evaluation out of the critical section
   where some of the threads of the target process might be still
   running.

* proc/native: handle process death during stop() on Windows

It is possible that the thread dies while we are inside the stop()
function. This results in an Access is denied error being returned by
SuspendThread being called on threads that no longer exist.

Delay the reporting the error from SuspendThread until the end of
stop() and only report it if the thread still exists at that point.

Fixes flakyness with TestIssue1101 that was exacerbated by moving
breakpoint condition evaluation outside of the backends.

_scripts/make.go

@@ -353,10 +353,30 @@ func testStandard() {
 		fmt.Println("\nTesting RR backend")
 		testCmdIntl("basic", "", "rr", "normal")
 	}
-	if TestIncludePIE && (runtime.GOOS == "linux" || (runtime.GOOS == "windows" && goversion.VersionAfterOrEqual(runtime.Version(), 1, 15))) {
+	if TestIncludePIE {
-		fmt.Println("\nTesting PIE buildmode, default backend")
+		dopie := false
-		testCmdIntl("basic", "", "default", "pie")
+		switch runtime.GOOS {
-		testCmdIntl("core", "", "default", "pie")
+		case "linux":
+			dopie = true
+		case "windows":
+			// only on Go 1.15 or later, with CGO_ENABLED and gcc found in path
+			if goversion.VersionAfterOrEqual(runtime.Version(), 1, 15) {
+				out, err := exec.Command("go", "env", "CGO_ENABLED").CombinedOutput()
+				if err != nil {
+					panic(err)
+				}
+				if strings.TrimSpace(string(out)) == "1" {
+					if _, err = exec.LookPath("gcc"); err == nil {
+						dopie = true
+					}
+				}
+			}
+		}
+		if dopie {
+			fmt.Println("\nTesting PIE buildmode, default backend")
+			testCmdIntl("basic", "", "default", "pie")
+			testCmdIntl("core", "", "default", "pie")
+		}
 	}
 	if runtime.GOOS == "linux" && inpath("rr") {
 		fmt.Println("\nTesting PIE buildmode, RR backend")

pkg/proc/breakpoints.go

@@ -183,19 +183,18 @@ type returnBreakpointInfo struct {
 }
 
 // CheckCondition evaluates bp's condition on thread.
-func (bp *Breakpoint) CheckCondition(thread Thread) BreakpointState {
+func (bp *Breakpoint) checkCondition(tgt *Target, thread Thread, bpstate *BreakpointState) {
-	bpstate := BreakpointState{Breakpoint: bp, Active: false, Stepping: false, SteppingInto: false, CondError: nil}
+	*bpstate = BreakpointState{Breakpoint: bp, Active: false, Stepping: false, SteppingInto: false, CondError: nil}
 	for _, breaklet := range bp.Breaklets {
-		bpstate.checkCond(breaklet, thread)
+		bpstate.checkCond(tgt, breaklet, thread)
 	}
-	return bpstate
 }
 
-func (bpstate *BreakpointState) checkCond(breaklet *Breaklet, thread Thread) {
+func (bpstate *BreakpointState) checkCond(tgt *Target, breaklet *Breaklet, thread Thread) {
 	var condErr error
 	active := true
 	if breaklet.Cond != nil {
-		active, condErr = evalBreakpointCondition(thread, breaklet.Cond)
+		active, condErr = evalBreakpointCondition(tgt, thread, breaklet.Cond)
 	}
 
 	if condErr != nil && bpstate.CondError == nil {
@@ -334,13 +333,13 @@ func (bp *Breakpoint) UserBreaklet() *Breaklet {
 	return nil
 }
 
-func evalBreakpointCondition(thread Thread, cond ast.Expr) (bool, error) {
+func evalBreakpointCondition(tgt *Target, thread Thread, cond ast.Expr) (bool, error) {
 	if cond == nil {
 		return true, nil
 	}
-	scope, err := GoroutineScope(nil, thread)
+	scope, err := GoroutineScope(tgt, thread)
 	if err != nil {
-		scope, err = ThreadScope(nil, thread)
+		scope, err = ThreadScope(tgt, thread)
 		if err != nil {
 			return true, err
 		}

pkg/proc/core/core_test.go

@@ -297,7 +297,7 @@ func TestCore(t *testing.T) {
 	if mainFrame == nil {
 		t.Fatalf("Couldn't find main in stack %v", panickingStack)
 	}
-	msg, err := proc.FrameToScope(p, p.BinInfo(), p.Memory(), nil, *mainFrame).EvalVariable("msg", proc.LoadConfig{MaxStringLen: 64})
+	msg, err := proc.FrameToScope(p, p.Memory(), nil, *mainFrame).EvalVariable("msg", proc.LoadConfig{MaxStringLen: 64})
 	if err != nil {
 		t.Fatalf("Couldn't EvalVariable(msg, ...): %v", err)
 	}
@@ -427,7 +427,7 @@ mainSearch:
 		t.Fatal("could not find main.main frame")
 	}
 
-	scope := proc.FrameToScope(p, p.BinInfo(), p.Memory(), nil, *mainFrame)
+	scope := proc.FrameToScope(p, p.Memory(), nil, *mainFrame)
 	loadConfig := proc.LoadConfig{FollowPointers: true, MaxVariableRecurse: 1, MaxStringLen: 64, MaxArrayValues: 64, MaxStructFields: -1}
 	v1, err := scope.EvalVariable("t", loadConfig)
 	assertNoError(err, t, "EvalVariable(t)")

pkg/proc/eval.go

@@ -101,7 +101,7 @@ func ConvertEvalScope(dbp *Target, gid, frame, deferCall int) (*EvalScope, error
 		return d.EvalScope(dbp, ct)
 	}
 
-	return FrameToScope(dbp, dbp.BinInfo(), dbp.Memory(), g, locs[frame:]...), nil
+	return FrameToScope(dbp, dbp.Memory(), g, locs[frame:]...), nil
 }
 
 // FrameToScope returns a new EvalScope for frames[0].
@@ -109,7 +109,7 @@ func ConvertEvalScope(dbp *Target, gid, frame, deferCall int) (*EvalScope, error
 // frames[0].Regs.SP() and frames[1].Regs.CFA will be cached.
 // Otherwise all memory between frames[0].Regs.SP() and frames[0].Regs.CFA
 // will be cached.
-func FrameToScope(t *Target, bi *BinaryInfo, thread MemoryReadWriter, g *G, frames ...Stackframe) *EvalScope {
+func FrameToScope(t *Target, thread MemoryReadWriter, g *G, frames ...Stackframe) *EvalScope {
 	// Creates a cacheMem that will preload the entire stack frame the first
 	// time any local variable is read.
 	// Remember that the stack grows downward in memory.
@@ -124,7 +124,7 @@ func FrameToScope(t *Target, bi *BinaryInfo, thread MemoryReadWriter, g *G, fram
 		thread = cacheMemory(thread, minaddr, int(maxaddr-minaddr))
 	}
 
-	s := &EvalScope{Location: frames[0].Call, Regs: frames[0].Regs, Mem: thread, g: g, BinInfo: bi, target: t, frameOffset: frames[0].FrameOffset()}
+	s := &EvalScope{Location: frames[0].Call, Regs: frames[0].Regs, Mem: thread, g: g, BinInfo: t.BinInfo(), target: t, frameOffset: frames[0].FrameOffset()}
 	s.PC = frames[0].lastpc
 	return s
 }
@@ -138,7 +138,7 @@ func ThreadScope(t *Target, thread Thread) (*EvalScope, error) {
 	if len(locations) < 1 {
 		return nil, errors.New("could not decode first frame")
 	}
-	return FrameToScope(t, thread.BinInfo(), thread.ProcessMemory(), nil, locations...), nil
+	return FrameToScope(t, thread.ProcessMemory(), nil, locations...), nil
 }
 
 // GoroutineScope returns an EvalScope for the goroutine running on the given thread.
@@ -154,7 +154,7 @@ func GoroutineScope(t *Target, thread Thread) (*EvalScope, error) {
 	if err != nil {
 		return nil, err
 	}
-	return FrameToScope(t, thread.BinInfo(), thread.ProcessMemory(), g, locations...), nil
+	return FrameToScope(t, thread.ProcessMemory(), g, locations...), nil
 }
 
 // EvalExpression returns the value of the given expression.

pkg/proc/gdbserial/gdbserver.go

@@ -1806,7 +1806,7 @@ func (t *gdbThread) SetCurrentBreakpoint(adjustPC bool) error {
 				return err
 			}
 		}
-		t.CurrentBreakpoint = bp.CheckCondition(t)
+		t.CurrentBreakpoint.Breakpoint = bp
 	}
 	return nil
 }

pkg/proc/moduledata.go

@@ -13,7 +13,7 @@ type moduleData struct {
 }
 
 func loadModuleData(bi *BinaryInfo, mem MemoryReadWriter) ([]moduleData, error) {
-	scope := globalScope(bi, bi.Images[0], mem)
+	scope := globalScope(nil, bi, bi.Images[0], mem)
 	var md *Variable
 	md, err := scope.findGlobal("runtime", "firstmoduledata")
 	if err != nil {
@@ -130,7 +130,7 @@ func resolveNameOff(bi *BinaryInfo, mds []moduleData, typeAddr, off uint64, mem
 }
 
 func reflectOffsMapAccess(bi *BinaryInfo, off uint64, mem MemoryReadWriter) (*Variable, error) {
-	scope := globalScope(bi, bi.Images[0], mem)
+	scope := globalScope(nil, bi, bi.Images[0], mem)
 	reflectOffs, err := scope.findGlobal("runtime", "reflectOffs")
 	if err != nil {
 		return nil, err

pkg/proc/native/proc_windows.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/go-delve/delve/pkg/proc"
 	"github.com/go-delve/delve/pkg/proc/internal/ebpf"
+	"github.com/go-delve/delve/pkg/proc/winutil"
 )
 
 // osProcessDetails holds Windows specific information.
@@ -437,11 +438,17 @@ func (dbp *nativeProcess) stop(trapthread *nativeThread) (*nativeThread, error)
 		return nil, err
 	}
 
+	context := winutil.NewCONTEXT()
+
 	for _, thread := range dbp.threads {
+		thread.os.delayErr = nil
 		if !thread.os.dbgUiRemoteBreakIn {
-			_, err := _SuspendThread(thread.os.hThread)
+			// Wait before reporting the error, the thread could be removed when we
-			if err != nil {
+			// call waitForDebugEvent in the next loop.
-				return nil, err
+			_, thread.os.delayErr = _SuspendThread(thread.os.hThread)
+			if thread.os.delayErr == nil {
+				// This call will block until the thread has stopped.
+				_ = _GetThreadContext(thread.os.hThread, context)
 			}
 		}
 	}
@@ -467,6 +474,31 @@ func (dbp *nativeProcess) stop(trapthread *nativeThread) (*nativeThread, error)
 		}
 	}
 
+	// Check if trapthread still exist, if the process is dying it could have
+	// been removed while we were stopping the other threads.
+	trapthreadFound := false
+	for _, thread := range dbp.threads {
+		if thread.ID == trapthread.ID {
+			trapthreadFound = true
+		}
+		if thread.os.delayErr != nil && thread.os.delayErr != syscall.Errno(0x5) {
+			// Do not report Access is denied error, it is caused by the thread
+			// having already died but we haven't been notified about it yet.
+			return nil, thread.os.delayErr
+		}
+	}
+
+	if !trapthreadFound {
+		// trapthread exited during stop, pick another one
+		trapthread = nil
+		for _, thread := range dbp.threads {
+			if thread.CurrentBreakpoint.Breakpoint != nil && thread.os.delayErr == nil {
+				trapthread = thread
+				break
+			}
+		}
+	}
+
 	return trapthread, nil
 }
 

pkg/proc/native/threads.go

@@ -153,9 +153,7 @@ func (t *nativeThread) SetCurrentBreakpoint(adjustPC bool) error {
 		}
 	}
 
-	if bp != nil {
+	t.CurrentBreakpoint.Breakpoint = bp
-		t.CurrentBreakpoint = bp.CheckCondition(t)
-	}
 	return nil
 }
 

pkg/proc/native/threads_windows.go

@@ -18,6 +18,7 @@ type waitStatus sys.WaitStatus
 type osSpecificDetails struct {
 	hThread            syscall.Handle
 	dbgUiRemoteBreakIn bool // whether thread is an auxiliary DbgUiRemoteBreakIn thread created by Windows
+	delayErr           error
 }
 
 func (t *nativeThread) singleStep() error {

pkg/proc/proc_test.go

@@ -1149,7 +1149,7 @@ func evalVariableOrError(p *proc.Target, symbol string) (*proc.Variable, error)
 		var frame proc.Stackframe
 		frame, err = findFirstNonRuntimeFrame(p)
 		if err == nil {
-			scope = proc.FrameToScope(p, p.BinInfo(), p.Memory(), nil, frame)
+			scope = proc.FrameToScope(p, p.Memory(), nil, frame)
 		}
 	} else {
 		scope, err = proc.GoroutineScope(p, p.CurrentThread())
@@ -3036,7 +3036,7 @@ func TestIssue871(t *testing.T) {
 			var frame proc.Stackframe
 			frame, err = findFirstNonRuntimeFrame(p)
 			if err == nil {
-				scope = proc.FrameToScope(p, p.BinInfo(), p.Memory(), nil, frame)
+				scope = proc.FrameToScope(p, p.Memory(), nil, frame)
 			}
 		} else {
 			scope, err = proc.GoroutineScope(p, p.CurrentThread())
@@ -3482,7 +3482,7 @@ func TestIssue1034(t *testing.T) {
 		assertNoError(p.Continue(), t, "Continue()")
 		frames, err := p.SelectedGoroutine().Stacktrace(10, 0)
 		assertNoError(err, t, "Stacktrace")
-		scope := proc.FrameToScope(p, p.BinInfo(), p.Memory(), nil, frames[2:]...)
+		scope := proc.FrameToScope(p, p.Memory(), nil, frames[2:]...)
 		args, _ := scope.FunctionArguments(normalLoadConfig)
 		assertNoError(err, t, "FunctionArguments()")
 		if len(args) > 0 {
@@ -3613,9 +3613,13 @@ func TestIssue1101(t *testing.T) {
 			exitErr = p.Continue()
 		}
 		if pexit, exited := exitErr.(proc.ErrProcessExited); exited {
-			if pexit.Status != 2 && testBackend != "lldb" {
+			if pexit.Status != 2 && testBackend != "lldb" && (runtime.GOOS != "linux" || runtime.GOARCH != "386") {
-				// looks like there's a bug with debugserver on macOS that sometimes
+				// Looks like there's a bug with debugserver on macOS that sometimes
 				// will report exit status 0 instead of the proper exit status.
+				//
+				// Also it seems that sometimes on linux/386 we will not receive the
+				// exit status. This happens if the process exits at the same time as it
+				// receives a signal.
 				t.Fatalf("process exited status %d (expected 2)", pexit.Status)
 			}
 		} else {
@@ -5141,8 +5145,8 @@ func TestDump(t *testing.T) {
 					t.Errorf("Frame mismatch %d.%d\nlive:\t%s\ncore:\t%s", gos[i].ID, j, convertFrame(p.BinInfo().Arch, &frames[j]), convertFrame(p.BinInfo().Arch, &cframes[j]))
 				}
 				if frames[j].Call.Fn != nil && frames[j].Call.Fn.Name == "main.main" {
-					scope = proc.FrameToScope(p, p.BinInfo(), p.Memory(), gos[i], frames[j:]...)
+					scope = proc.FrameToScope(p, p.Memory(), gos[i], frames[j:]...)
-					cscope = proc.FrameToScope(c, c.BinInfo(), c.Memory(), cgos[i], cframes[j:]...)
+					cscope = proc.FrameToScope(c, c.Memory(), cgos[i], cframes[j:]...)
 				}
 			}
 		}
@@ -5263,6 +5267,7 @@ func TestCompositeMemoryWrite(t *testing.T) {
 }
 
 func TestVariablesWithExternalLinking(t *testing.T) {
+	protest.MustHaveCgo(t)
 	// Tests that macOSDebugFrameBugWorkaround works.
 	// See:
 	//  https://github.com/golang/go/issues/25841

pkg/proc/target.go

@@ -203,7 +203,7 @@ func (t *Target) IsCgo() bool {
 	if t.iscgo != nil {
 		return *t.iscgo
 	}
-	scope := globalScope(t.BinInfo(), t.BinInfo().Images[0], t.Memory())
+	scope := globalScope(t, t.BinInfo(), t.BinInfo().Images[0], t.Memory())
 	iscgov, err := scope.findGlobal("runtime", "iscgo")
 	if err == nil {
 		iscgov.loadValue(loadFullValue)
@@ -333,7 +333,7 @@ func setAsyncPreemptOff(p *Target, v int64) {
 		return
 	}
 	logger := p.BinInfo().logger
-	scope := globalScope(p.BinInfo(), p.BinInfo().Images[0], p.Memory())
+	scope := globalScope(p, p.BinInfo(), p.BinInfo().Images[0], p.Memory())
 	debugv, err := scope.findGlobal("runtime", "debug")
 	if err != nil || debugv.Unreadable != nil {
 		logger.Warnf("could not find runtime/debug variable (or unreadable): %v %v", err, debugv.Unreadable)

pkg/proc/target_exec.go

@@ -70,9 +70,17 @@ func (dbp *Target) Continue() error {
 			return nil
 		}
 		dbp.ClearCaches()
-		trapthread, stopReason, err := dbp.proc.ContinueOnce()
+		trapthread, stopReason, contOnceErr := dbp.proc.ContinueOnce()
 		dbp.StopReason = stopReason
-		if err != nil {
+
+		threads := dbp.ThreadList()
+		for _, thread := range threads {
+			if thread.Breakpoint().Breakpoint != nil {
+				thread.Breakpoint().Breakpoint.checkCondition(dbp, thread, thread.Breakpoint())
+			}
+		}
+
+		if contOnceErr != nil {
 			// Attempt to refresh status of current thread/current goroutine, see
 			// Issue #2078.
 			// Errors are ignored because depending on why ContinueOnce failed this
@@ -84,17 +92,15 @@ func (dbp *Target) Continue() error {
 					dbp.selectedGoroutine, _ = GetG(curth)
 				}
 			}
-			if pe, ok := err.(ErrProcessExited); ok {
+			if pe, ok := contOnceErr.(ErrProcessExited); ok {
 				dbp.exitStatus = pe.Status
 			}
-			return err
+			return contOnceErr
 		}
 		if dbp.StopReason == StopLaunched {
 			dbp.ClearSteppingBreakpoints()
 		}
 
-		threads := dbp.ThreadList()
-
 		callInjectionDone, callErr := callInjectionProtocol(dbp, threads)
 		// callErr check delayed until after pickCurrentThread, which must always
 		// happen, otherwise the debugger could be left in an inconsistent
@@ -190,7 +196,7 @@ func (dbp *Target) Continue() error {
 				return conditionErrors(threads)
 			}
 		case curbp.Active:
-			onNextGoroutine, err := onNextGoroutine(curthread, dbp.Breakpoints())
+			onNextGoroutine, err := onNextGoroutine(dbp, curthread, dbp.Breakpoints())
 			if err != nil {
 				return err
 			}
@@ -1015,7 +1021,7 @@ func stepOutReverse(p *Target, topframe, retframe Stackframe, sameGCond ast.Expr
 }
 
 // onNextGoroutine returns true if this thread is on the goroutine requested by the current 'next' command
-func onNextGoroutine(thread Thread, breakpoints *BreakpointMap) (bool, error) {
+func onNextGoroutine(tgt *Target, thread Thread, breakpoints *BreakpointMap) (bool, error) {
 	var breaklet *Breaklet
 breakletSearch:
 	for i := range breakpoints.M {
@@ -1038,12 +1044,13 @@ breakletSearch:
 	// function or by returning from the function:
 	//   runtime.curg.goid == X && (runtime.frameoff == Y || runtime.frameoff == Z)
 	// Here we are only interested in testing the runtime.curg.goid clause.
-	w := onNextGoroutineWalker{thread: thread}
+	w := onNextGoroutineWalker{tgt: tgt, thread: thread}
 	ast.Walk(&w, breaklet.Cond)
 	return w.ret, w.err
 }
 
 type onNextGoroutineWalker struct {
+	tgt    *Target
 	thread Thread
 	ret    bool
 	err    error
@@ -1051,7 +1058,7 @@ type onNextGoroutineWalker struct {
 
 func (w *onNextGoroutineWalker) Visit(n ast.Node) ast.Visitor {
 	if binx, isbin := n.(*ast.BinaryExpr); isbin && binx.Op == token.EQL && exprToString(binx.X) == "runtime.curg.goid" {
-		w.ret, w.err = evalBreakpointCondition(w.thread, n.(ast.Expr))
+		w.ret, w.err = evalBreakpointCondition(w.tgt, w.thread, n.(ast.Expr))
 		return nil
 	}
 	return w

pkg/proc/variables.go

@@ -581,8 +581,8 @@ func (err *IsNilErr) Error() string {
 	return fmt.Sprintf("%s is nil", err.name)
 }
 
-func globalScope(bi *BinaryInfo, image *Image, mem MemoryReadWriter) *EvalScope {
+func globalScope(tgt *Target, bi *BinaryInfo, image *Image, mem MemoryReadWriter) *EvalScope {
-	return &EvalScope{Location: Location{}, Regs: op.DwarfRegisters{StaticBase: image.StaticBase}, Mem: mem, g: nil, BinInfo: bi, frameOffset: 0}
+	return &EvalScope{Location: Location{}, Regs: op.DwarfRegisters{StaticBase: image.StaticBase}, Mem: mem, g: nil, BinInfo: bi, target: tgt, frameOffset: 0}
 }
 
 func newVariableFromThread(t Thread, name string, addr uint64, dwarfType godwarf.Type) *Variable {
@@ -947,8 +947,8 @@ func (v *Variable) fieldVariable(name string) *Variable {
 var errTracebackAncestorsDisabled = errors.New("tracebackancestors is disabled")
 
 // Ancestors returns the list of ancestors for g.
-func Ancestors(p Process, g *G, n int) ([]Ancestor, error) {
+func Ancestors(p *Target, g *G, n int) ([]Ancestor, error) {
-	scope := globalScope(p.BinInfo(), p.BinInfo().Images[0], p.Memory())
+	scope := globalScope(p, p.BinInfo(), p.BinInfo().Images[0], p.Memory())
 	tbav, err := scope.EvalExpression("runtime.debug.tracebackancestors", loadSingleValue)
 	if err == nil && tbav.Unreadable == nil && tbav.Kind == reflect.Int {
 		tba, _ := constant.Int64Val(tbav.Value)

service/debugger/debugger.go

@@ -1752,7 +1752,7 @@ func (d *Debugger) convertStacktrace(rawlocs []proc.Stackframe, cfg *proc.LoadCo
 		}
 		if cfg != nil && rawlocs[i].Current.Fn != nil {
 			var err error
-			scope := proc.FrameToScope(d.target, d.target.BinInfo(), d.target.Memory(), nil, rawlocs[i:]...)
+			scope := proc.FrameToScope(d.target, d.target.Memory(), nil, rawlocs[i:]...)
 			locals, err := scope.LocalVariables(*cfg)
 			if err != nil {
 				return nil, err

service/test/variables_test.go

@@ -92,7 +92,7 @@ func evalScope(p *proc.Target) (*proc.EvalScope, error) {
 	if err != nil {
 		return nil, err
 	}
-	return proc.FrameToScope(p, p.BinInfo(), p.Memory(), nil, frame), nil
+	return proc.FrameToScope(p, p.Memory(), nil, frame), nil
 }
 
 func evalVariable(p *proc.Target, symbol string, cfg proc.LoadConfig) (*proc.Variable, error) {
@@ -471,7 +471,7 @@ func TestLocalVariables(t *testing.T) {
 				var frame proc.Stackframe
 				frame, err = findFirstNonRuntimeFrame(p)
 				if err == nil {
-					scope = proc.FrameToScope(p, p.BinInfo(), p.Memory(), nil, frame)
+					scope = proc.FrameToScope(p, p.Memory(), nil, frame)
 				}
 			} else {
 				scope, err = proc.GoroutineScope(p, p.CurrentThread())