From f90134eb4db1c423e24fddfbc6eff41b288e6297 Mon Sep 17 00:00:00 2001 From: aarzilli Date: Sat, 8 Aug 2020 15:06:14 +0200 Subject: [PATCH] proc: prevent internal breakpoint conditions from failing An internal breakpoint condition shouldn't ever error: * use a ThreadContext to evaluate conditions if a goroutine isn't available * evaluate runtime.curg to a fake g variable containing only `goid == 0` when there is no current goroutine Fixes #2113 --- _fixtures/issue2113.go | 35 ++++++++++++++++++ pkg/proc/breakpoints.go | 5 ++- pkg/proc/eval.go | 11 +++++- pkg/proc/proc_test.go | 81 ++++++++++++++++++++++++++++++++++++++++- pkg/proc/target_exec.go | 4 +- pkg/proc/variables.go | 8 ++++ 6 files changed, 139 insertions(+), 5 deletions(-) create mode 100644 _fixtures/issue2113.go diff --git a/_fixtures/issue2113.go b/_fixtures/issue2113.go new file mode 100644 index 0000000000..04f095ef5c --- /dev/null +++ b/_fixtures/issue2113.go @@ -0,0 +1,35 @@ +package main + +import ( + "fmt" + "runtime" + "sync" +) + +func coroutine(i int, start, finish *sync.WaitGroup) { + defer finish.Done() + + j := i * 2 + + if i == 99 { + runtime.Breakpoint() + start.Done() + } else { + start.Wait() + } + + fmt.Println("hello ", i, j) + fmt.Println("goodbye", i, j) +} + +func main() { + i := 0 + var start, finish sync.WaitGroup + start.Add(1) + for ; i < 100; i++ { + finish.Add(1) + go coroutine(i, &start, &finish) + } + finish.Wait() + println(i) +} diff --git a/pkg/proc/breakpoints.go b/pkg/proc/breakpoints.go index b331dbf09d..bcd7cb2736 100644 --- a/pkg/proc/breakpoints.go +++ b/pkg/proc/breakpoints.go @@ -196,7 +196,10 @@ func evalBreakpointCondition(thread Thread, cond ast.Expr) (bool, error) { } scope, err := GoroutineScope(thread) if err != nil { - return true, err + scope, err = ThreadScope(thread) + if err != nil { + return true, err + } } v, err := scope.evalAST(cond) if err != nil { diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index 022f0550c8..74221b956b 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -729,7 +729,16 @@ func (scope *EvalScope) evalAST(t ast.Expr) (*Variable, error) { if maybePkg, ok := node.X.(*ast.Ident); ok { if maybePkg.Name == "runtime" && node.Sel.Name == "curg" { if scope.g == nil { - return nilVariable, nil + typ, err := scope.BinInfo.findType("runtime.g") + if err != nil { + return nil, fmt.Errorf("blah: %v", err) + } + gvar := newVariable("curg", fakeAddress, typ, scope.BinInfo, scope.Mem) + gvar.loaded = true + gvar.Flags = VariableFakeAddress + gvar.Children = append(gvar.Children, *newConstant(constant.MakeInt64(0), scope.Mem)) + gvar.Children[0].Name = "goid" + return gvar, nil } return scope.g.variable.clone(), nil } else if maybePkg.Name == "runtime" && node.Sel.Name == "frameoff" { diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index 2858740c8f..4cf74b2dde 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -8,6 +8,7 @@ import ( "go/constant" "go/token" "io/ioutil" + "math/rand" "net" "net/http" "os" @@ -1106,7 +1107,7 @@ func TestProcessReceivesSIGCHLD(t *testing.T) { func TestIssue239(t *testing.T) { withTestProcess("is sue239", t, func(p *proc.Target, fixture protest.Fixture) { setFileBreakpoint(p, t, fixture.Source, 17) - assertNoError(p.Continue(), t, fmt.Sprintf("Continue()")) + assertNoError(p.Continue(), t, "Continue()") }) } @@ -4938,3 +4939,81 @@ func TestRequestManualStopWhileStopped(t *testing.T) { t.Logf("done") }) } + +func TestStepOutPreservesGoroutine(t *testing.T) { + // Checks that StepOut preserves the currently selected goroutine. + if runtime.GOOS == "freebsd" { + t.Skip("XXX - not working") + } + rand.Seed(time.Now().Unix()) + withTestProcess("issue2113", t, func(p *proc.Target, fixture protest.Fixture) { + assertNoError(p.Continue(), t, "Continue()") + + logState := func() { + g := p.SelectedGoroutine() + var goid int = -42 + if g != nil { + goid = g.ID + } + pc := currentPC(p, t) + f, l, fn := p.BinInfo().PCToLine(pc) + var fnname string = "???" + if fn != nil { + fnname = fn.Name + } + t.Logf("goroutine %d at %s:%d in %s", goid, f, l, fnname) + } + + logState() + + gs, _, err := proc.GoroutinesInfo(p, 0, 0) + assertNoError(err, t, "GoroutinesInfo") + candg := []*proc.G{} + bestg := []*proc.G{} + for _, g := range gs { + frames, err := g.Stacktrace(20, 0) + assertNoError(err, t, "Stacktrace") + for _, frame := range frames { + if frame.Call.Fn != nil && frame.Call.Fn.Name == "main.coroutine" { + candg = append(candg, g) + if g.Thread != nil && frames[0].Call.Fn != nil && strings.HasPrefix(frames[0].Call.Fn.Name, "runtime.") { + bestg = append(bestg, g) + } + break + } + } + } + var pickg *proc.G + if len(bestg) > 0 { + pickg = bestg[rand.Intn(len(bestg))] + t.Logf("selected goroutine %d (best)\n", pickg.ID) + } else { + pickg = candg[rand.Intn(len(candg))] + t.Logf("selected goroutine %d\n", pickg.ID) + + } + goid := pickg.ID + assertNoError(p.SwitchGoroutine(pickg), t, "SwitchGoroutine") + + logState() + + err = p.StepOut() + if err != nil { + _, isexited := err.(proc.ErrProcessExited) + if !isexited { + assertNoError(err, t, "StepOut()") + } else { + return + } + } + + logState() + + g2 := p.SelectedGoroutine() + if g2 == nil { + t.Fatalf("no selected goroutine after stepout") + } else if g2.ID != goid { + t.Fatalf("unexpected selected goroutine %d", g2.ID) + } + }) +} diff --git a/pkg/proc/target_exec.go b/pkg/proc/target_exec.go index 4b0fb82e89..885e4d9c9c 100644 --- a/pkg/proc/target_exec.go +++ b/pkg/proc/target_exec.go @@ -377,7 +377,7 @@ func (dbp *Target) StepOut() error { if topframe.Ret != 0 { topframe, retframe := skipAutogeneratedWrappersOut(selg, curthread, &topframe, &retframe) retFrameCond := astutil.And(sameGCond, frameoffCondition(retframe)) - bp, err := allowDuplicateBreakpoint(dbp.SetBreakpoint(topframe.Ret, NextBreakpoint, retFrameCond)) + bp, err := allowDuplicateBreakpoint(dbp.SetBreakpoint(retframe.Current.PC, NextBreakpoint, retFrameCond)) if err != nil { return err } @@ -620,7 +620,7 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error { // For inlined functions there is no need to do this, the set of PCs // returned by the AllPCsBetween call above already cover all instructions // of the containing function. - bp, err := dbp.SetBreakpoint(topframe.Ret, NextBreakpoint, retFrameCond) + bp, err := dbp.SetBreakpoint(retframe.Current.PC, NextBreakpoint, retFrameCond) if _, isexists := err.(BreakpointExistsError); isexists { if bp.Kind == NextBreakpoint { // If the return address shares the same address with one of the lines diff --git a/pkg/proc/variables.go b/pkg/proc/variables.go index 85ede40f3b..35d01ae9d3 100644 --- a/pkg/proc/variables.go +++ b/pkg/proc/variables.go @@ -1039,6 +1039,14 @@ func (v *Variable) structMember(memberName string) (*Variable, error) { return v.clone(), nil } vname := v.Name + if v.loaded && (v.Flags&VariableFakeAddress) != 0 { + for i := range v.Children { + if v.Children[i].Name == memberName { + return &v.Children[i], nil + } + } + return nil, fmt.Errorf("%s has no member %s", vname, memberName) + } switch v.Kind { case reflect.Chan: v = v.clone()