From 6a0423a1e922652dc28ff980d9c61056c2e75231 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Mon, 21 Aug 2023 21:30:56 +0200 Subject: [PATCH] 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 | 10 ++++++++-- pkg/proc/core/core_test.go | 4 ++-- pkg/proc/eval.go | 35 +++++++++++++++++++++------------ pkg/proc/proc_test.go | 10 +++++----- pkg/proc/stack_sigtramp.go | 2 +- pkg/proc/stackwatch.go | 2 +- pkg/proc/target_exec.go | 38 ++++++++++++++++++++++++------------ pkg/proc/variables_test.go | 4 ++-- service/debugger/debugger.go | 2 +- 9 files changed, 68 insertions(+), 39 deletions(-) diff --git a/pkg/astutil/astutil.go b/pkg/astutil/astutil.go index 6d861826..9b39448e 100644 --- a/pkg/astutil/astutil.go +++ b/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} } diff --git a/pkg/proc/core/core_test.go b/pkg/proc/core/core_test.go index 2744bf44..63c33996 100644 --- a/pkg/proc/core/core_test.go +++ b/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)") diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index 220cb93a..0692e36c 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -31,12 +31,13 @@ const goDictionaryName = ".dict" // current location (PC), and canonical frame address. type EvalScope struct { Location - Regs op.DwarfRegisters - Mem MemoryReadWriter // Target's memory - g *G - BinInfo *BinaryInfo - target *Target - loadCfg *LoadConfig + Regs op.DwarfRegisters + Mem MemoryReadWriter // Target's memory + g *G + threadID int + BinInfo *BinaryInfo + target *Target + loadCfg *LoadConfig frameOffset int64 @@ -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 } diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index fb77c44b..633752f1 100644 --- a/pkg/proc/proc_test.go +++ b/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:]...) - cscope = proc.FrameToScope(c, c.Memory(), cgos[i], cframes[j:]...) + scope = proc.FrameToScope(p, p.Memory(), gos[i], 0, frames[j:]...) + cscope = proc.FrameToScope(c, c.Memory(), cgos[i], 0, cframes[j:]...) } } } diff --git a/pkg/proc/stack_sigtramp.go b/pkg/proc/stack_sigtramp.go index da742674..bbf0fe52 100644 --- a/pkg/proc/stack_sigtramp.go +++ b/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 { diff --git a/pkg/proc/stackwatch.go b/pkg/proc/stackwatch.go index 64f9dc02..99ee6db7 100644 --- a/pkg/proc/stackwatch.go +++ b/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 diff --git a/pkg/proc/target_exec.go b/pkg/proc/target_exec.go index 6a99d745..3f245407 100644 --- a/pkg/proc/target_exec.go +++ b/pkg/proc/target_exec.go @@ -444,9 +444,21 @@ 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 { - return 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 - if sameGCond != nil { - sameFrameCond = astutil.And(sameGCond, frameoffCondition(&topframe)) - } + 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,9 +1250,12 @@ 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.tgt, w.thread, n.(ast.Expr)) - return nil + 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 } diff --git a/pkg/proc/variables_test.go b/pkg/proc/variables_test.go index 82a63ce1..d074ee1f 100644 --- a/pkg/proc/variables_test.go +++ b/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()) diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go index 78a77929..c07eb1b5 100644 --- a/service/debugger/debugger.go +++ b/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