Skip to content

Commit

Permalink
proc: prevent internal breakpoint conditions from failing
Browse files Browse the repository at this point in the history
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
  • Loading branch information
aarzilli committed Sep 1, 2020
1 parent 5a5d5f9 commit f90134e
Show file tree
Hide file tree
Showing 6 changed files with 139 additions and 5 deletions.
35 changes: 35 additions & 0 deletions _fixtures/issue2113.go
Original file line number Diff line number Diff line change
@@ -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)
}
5 changes: 4 additions & 1 deletion pkg/proc/breakpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 10 additions & 1 deletion pkg/proc/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down
81 changes: 80 additions & 1 deletion pkg/proc/proc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"go/constant"
"go/token"
"io/ioutil"
"math/rand"
"net"
"net/http"
"os"
Expand Down Expand Up @@ -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()")
})
}

Expand Down Expand Up @@ -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)
}
})
}
4 changes: 2 additions & 2 deletions pkg/proc/target_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions pkg/proc/variables.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit f90134e

Please sign in to comment.