proc: when stepping set condition on thread ID if there is no curg (#3475)

If there is no current goroutine when 'next', 'step' or 'stepout' are
used set a condition that the thread ID should stay the same instead.
This makes stepping work for multithreaded C programs or Go programs
that have threads started by cgo code.

Fixes #3262

pkg/astutil/astutil.go

@@ -30,11 +30,17 @@ func Int(n int64) *ast.BasicLit {
 }
 
 // And returns an expression evaluating 'x && y'.
-func And(x, y ast.Expr) *ast.BinaryExpr {
+func And(x, y ast.Expr) ast.Expr {
+	if x == nil || y == nil {
+		return nil
+	}
 	return &ast.BinaryExpr{Op: token.LAND, X: x, Y: y}
 }
 
 // Or returns an expression evaluating 'x || y'.
-func Or(x, y ast.Expr) *ast.BinaryExpr {
+func Or(x, y ast.Expr) ast.Expr {
+	if x == nil || y == nil {
+		return nil
+	}
 	return &ast.BinaryExpr{Op: token.LOR, X: x, Y: y}
 }

pkg/proc/core/core_test.go

@@ -299,7 +299,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.Memory(), nil, *mainFrame).EvalExpression("msg", proc.LoadConfig{MaxStringLen: 64})
+	msg, err := proc.FrameToScope(p, p.Memory(), nil, p.CurrentThread().ThreadID(), *mainFrame).EvalExpression("msg", proc.LoadConfig{MaxStringLen: 64})
 	if err != nil {
 		t.Fatalf("Couldn't EvalVariable(msg, ...): %v", err)
 	}
@@ -434,7 +434,7 @@ mainSearch:
 		t.Fatal("could not find main.main frame")
 	}
 
-	scope := proc.FrameToScope(p, p.Memory(), nil, *mainFrame)
+	scope := proc.FrameToScope(p, p.Memory(), nil, p.CurrentThread().ThreadID(), *mainFrame)
 	loadConfig := proc.LoadConfig{FollowPointers: true, MaxVariableRecurse: 1, MaxStringLen: 64, MaxArrayValues: 64, MaxStructFields: -1}
 	v1, err := scope.EvalExpression("t", loadConfig)
 	assertNoError(err, t, "EvalVariable(t)")

pkg/proc/eval.go

@@ -34,6 +34,7 @@ type EvalScope struct {
 	Regs     op.DwarfRegisters
 	Mem      MemoryReadWriter // Target's memory
 	g        *G
+	threadID int
 	BinInfo  *BinaryInfo
 	target   *Target
 	loadCfg  *LoadConfig
@@ -78,6 +79,7 @@ func ConvertEvalScope(dbp *Target, gid int64, frame, deferCall int) (*EvalScope,
 		return nil, err
 	}
 	ct := dbp.CurrentThread()
+	threadID := ct.ThreadID()
 	g, err := FindGoroutine(dbp, gid)
 	if err != nil {
 		return nil, err
@@ -90,6 +92,9 @@ func ConvertEvalScope(dbp *Target, gid int64, frame, deferCall int) (*EvalScope,
 
 	var locs []Stackframe
 	if g != nil {
+		if g.Thread != nil {
+			threadID = g.Thread.ThreadID()
+		}
 		locs, err = GoroutineStacktrace(dbp, g, frame+1, opts)
 	} else {
 		locs, err = ThreadStacktrace(dbp, ct, frame+1)
@@ -115,7 +120,7 @@ func ConvertEvalScope(dbp *Target, gid int64, frame, deferCall int) (*EvalScope,
 		return d.EvalScope(dbp, ct)
 	}
 
-	return FrameToScope(dbp, dbp.Memory(), g, locs[frame:]...), nil
+	return FrameToScope(dbp, dbp.Memory(), g, threadID, locs[frame:]...), nil
 }
 
 // FrameToScope returns a new EvalScope for frames[0].
@@ -123,7 +128,7 @@ func ConvertEvalScope(dbp *Target, gid int64, frame, deferCall int) (*EvalScope,
 // 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, thread MemoryReadWriter, g *G, frames ...Stackframe) *EvalScope {
+func FrameToScope(t *Target, thread MemoryReadWriter, g *G, threadID int, 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.
@@ -138,7 +143,7 @@ func FrameToScope(t *Target, thread MemoryReadWriter, g *G, frames ...Stackframe
 		thread = cacheMemory(thread, minaddr, int(maxaddr-minaddr))
 	}
 
-	s := &EvalScope{Location: frames[0].Call, Regs: frames[0].Regs, Mem: thread, g: g, BinInfo: t.BinInfo(), 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(), threadID: threadID}
 	s.PC = frames[0].lastpc
 	return s
 }
@@ -152,7 +157,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.ProcessMemory(), nil, locations...), nil
+	return FrameToScope(t, thread.ProcessMemory(), nil, thread.ThreadID(), locations...), nil
 }
 
 // GoroutineScope returns an EvalScope for the goroutine running on the given thread.
@@ -168,7 +173,11 @@ func GoroutineScope(t *Target, thread Thread) (*EvalScope, error) {
 	if err != nil {
 		return nil, err
 	}
-	return FrameToScope(t, thread.ProcessMemory(), g, locations...), nil
+	threadID := 0
+	if g.Thread != nil {
+		threadID = g.Thread.ThreadID()
+	}
+	return FrameToScope(t, thread.ProcessMemory(), g, threadID, locations...), nil
 }
 
 // EvalExpression returns the value of the given expression.
@@ -634,7 +643,7 @@ func (scope *EvalScope) evalAST(t ast.Expr) (*Variable, error) {
 				if scope.g == nil {
 					typ, err := scope.BinInfo.findType("runtime.g")
 					if err != nil {
-						return nil, fmt.Errorf("blah: %v", err)
+						return nil, fmt.Errorf("could not find runtime.g: %v", err)
 					}
 					gvar := newVariable("curg", fakeAddressUnresolv, typ, scope.BinInfo, scope.Mem)
 					gvar.loaded = true
@@ -646,6 +655,8 @@ func (scope *EvalScope) evalAST(t ast.Expr) (*Variable, error) {
 				return scope.g.variable.clone(), nil
 			} else if maybePkg.Name == "runtime" && node.Sel.Name == "frameoff" {
 				return newConstant(constant.MakeInt64(scope.frameOffset), scope.Mem), nil
+			} else if maybePkg.Name == "runtime" && node.Sel.Name == "threadid" {
+				return newConstant(constant.MakeInt64(int64(scope.threadID)), scope.Mem), nil
 			} else if v, err := scope.findGlobal(maybePkg.Name, node.Sel.Name); err == nil {
 				return v, nil
 			}

pkg/proc/proc_test.go

@@ -1199,7 +1199,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.Memory(), nil, frame)
+			scope = proc.FrameToScope(p, p.Memory(), nil, 0, frame)
 		}
 	} else {
 		scope, err = proc.GoroutineScope(p, p.CurrentThread())
@@ -3101,7 +3101,7 @@ func TestIssue871(t *testing.T) {
 			var frame proc.Stackframe
 			frame, err = findFirstNonRuntimeFrame(p)
 			if err == nil {
-				scope = proc.FrameToScope(p, p.Memory(), nil, frame)
+				scope = proc.FrameToScope(p, p.Memory(), nil, 0, frame)
 			}
 		} else {
 			scope, err = proc.GoroutineScope(p, p.CurrentThread())
@@ -3514,7 +3514,7 @@ func TestIssue1034(t *testing.T) {
 		assertNoError(grp.Continue(), t, "Continue()")
 		frames, err := proc.GoroutineStacktrace(p, p.SelectedGoroutine(), 10, 0)
 		assertNoError(err, t, "Stacktrace")
-		scope := proc.FrameToScope(p, p.Memory(), nil, frames[2:]...)
+		scope := proc.FrameToScope(p, p.Memory(), nil, 0, frames[2:]...)
 		args, _ := scope.FunctionArguments(normalLoadConfig)
 		assertNoError(err, t, "FunctionArguments()")
 		if len(args) > 0 {
@@ -5277,8 +5277,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.Memory(), gos[i], frames[j:]...)
+					scope = proc.FrameToScope(p, p.Memory(), gos[i], 0, frames[j:]...)
-					cscope = proc.FrameToScope(c, c.Memory(), cgos[i], cframes[j:]...)
+					cscope = proc.FrameToScope(c, c.Memory(), cgos[i], 0, cframes[j:]...)
 				}
 			}
 		}

pkg/proc/stack_sigtramp.go

@@ -14,7 +14,7 @@ import (
 // readSigtrampgoContext reads runtime.sigtrampgo context at the specified address
 func (it *stackIterator) readSigtrampgoContext() (*op.DwarfRegisters, error) {
 	logger := logflags.DebuggerLogger()
-	scope := FrameToScope(it.target, it.mem, it.g, it.frame)
+	scope := FrameToScope(it.target, it.mem, it.g, 0, it.frame)
 	bi := it.bi
 
 	findvar := func(name string) *Variable {

pkg/proc/stackwatch.go

@@ -38,7 +38,7 @@ func (t *Target) setStackWatchBreakpoints(scope *EvalScope, watchpoint *Breakpoi
 		return err
 	}
 
-	sameGCond := sameGoroutineCondition(scope.g)
+	sameGCond := sameGoroutineCondition(scope.BinInfo, scope.g, 0)
 	retFrameCond := astutil.And(sameGCond, frameoffCondition(&retframe))
 
 	var deferpc uint64

pkg/proc/target_exec.go

@@ -444,10 +444,22 @@ func (grp *TargetGroup) Step() (err error) {
 
 // sameGoroutineCondition returns an expression that evaluates to true when
 // the current goroutine is g.
-func sameGoroutineCondition(g *G) ast.Expr {
+func sameGoroutineCondition(bi *BinaryInfo, g *G, threadID int) ast.Expr {
 	if g == nil {
+		if len(bi.Images[0].compileUnits) == 0 {
+			// It's unclear what the right behavior is here. We are probably
+			// debugging a process without debug info, this means we can't properly
+			// create a same goroutine condition (we don't have a description for the
+			// runtime.g type). If we don't set the condition then 'next' (and step,
+			// stepout) will work for single-threaded programs (in limited
+			// circumstances) but fail in presence of any concurrency.
+			// If we set a thread ID condition even single threaded programs can fail
+			// due to goroutine migration, but sometimes it will work even with
+			// concurrency.
 			return nil
 		}
+		return astutil.Eql(astutil.PkgVar("runtime", "threadid"), astutil.Int(int64(threadID)))
+	}
 	return astutil.Eql(astutil.Sel(astutil.PkgVar("runtime", "curg"), "goid"), astutil.Int(int64(g.ID)))
 }
 
@@ -492,7 +504,7 @@ func (grp *TargetGroup) StepOut() error {
 		return grp.Continue()
 	}
 
-	sameGCond := sameGoroutineCondition(selg)
+	sameGCond := sameGoroutineCondition(dbp.BinInfo(), selg, curthread.ThreadID())
 
 	if backward {
 		if err := stepOutReverse(dbp, topframe, retframe, sameGCond); err != nil {
@@ -544,7 +556,7 @@ func (grp *TargetGroup) StepInstruction() (err error) {
 		if g.Thread == nil {
 			// Step called on parked goroutine
 			if _, err := dbp.SetBreakpoint(0, g.PC, NextBreakpoint,
-				sameGoroutineCondition(dbp.SelectedGoroutine())); err != nil {
+				sameGoroutineCondition(dbp.BinInfo(), dbp.SelectedGoroutine(), thread.ThreadID())); err != nil {
 				return err
 			}
 			return grp.Continue()
@@ -637,7 +649,7 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error {
 		}
 	}
 
-	sameGCond := sameGoroutineCondition(selg)
+	sameGCond := sameGoroutineCondition(dbp.BinInfo(), selg, curthread.ThreadID())
 
 	var firstPCAfterPrologue uint64
 
@@ -667,10 +679,7 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error {
 		return err
 	}
 
-	var sameFrameCond ast.Expr
+	sameFrameCond := astutil.And(sameGCond, frameoffCondition(&topframe))
-	if sameGCond != nil {
-		sameFrameCond = astutil.And(sameGCond, frameoffCondition(&topframe))
-	}
 
 	if stepInto && !backward {
 		err := setStepIntoBreakpoints(dbp, topframe.Current.Fn, text, topframe, sameGCond)
@@ -818,7 +827,7 @@ func stepIntoCallback(curthread Thread, p *Target) (bool, error) {
 	// here we either set a breakpoint into the destination of the CALL
 	// instruction or we determined that the called function is hidden,
 	// either way we need to resume execution
-	if err = setStepIntoBreakpoint(p, fn, text, sameGoroutineCondition(g)); err != nil {
+	if err = setStepIntoBreakpoint(p, fn, text, sameGoroutineCondition(p.BinInfo(), g, curthread.ThreadID())); err != nil {
 		return false, err
 	}
 
@@ -1241,10 +1250,13 @@ 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" {
+	if binx, isbin := n.(*ast.BinaryExpr); isbin && binx.Op == token.EQL {
+		x := exprToString(binx.X)
+		if x == "runtime.curg.goid" || x == "runtime.threadid" {
 			w.ret, w.err = evalBreakpointCondition(w.tgt, w.thread, n.(ast.Expr))
 			return nil
 		}
+	}
 	return w
 }
 

pkg/proc/variables_test.go

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

service/debugger/debugger.go

@@ -1863,7 +1863,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.Selected, d.target.Selected.Memory(), nil, rawlocs[i:]...)
+			scope := proc.FrameToScope(d.target.Selected, d.target.Selected.Memory(), nil, 0, rawlocs[i:]...)
 			locals, err := scope.LocalVariables(*cfg)
 			if err != nil {
 				return nil, err