Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proc: use .closureptr for stepping through range-over-func statements #3763

Merged
merged 2 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 21 additions & 26 deletions _fixtures/rangeoverfunc.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,20 @@ B:
fmt.Println(result)
}

func TestRecur(n int) {
result := []int{}
if n > 0 {
TestRecur(n - 1)
}
for _, x := range OfSliceIndex([]int{10, 20, 30}) {
result = append(result, x)
if n == 3 {
TestRecur(0)
}
}
fmt.Println(result)
}

func main() {
TestTrickyIterAll()
TestTrickyIterAll2()
Expand All @@ -240,34 +254,23 @@ func main() {
TestLongReturnWrapper()
TestGotoA1()
TestGotoB1()
TestRecur(3)
}

type Seq[T any] func(yield func(T) bool)
type Seq2[T1, T2 any] func(yield func(T1, T2) bool)

type TrickyIterator struct {
yield func(int, int) bool
}

func (ti *TrickyIterator) iterEcho(s []int) Seq2[int, int] {
return func(yield func(int, int) bool) {
for i, v := range s {
if !yield(i, v) {
ti.yield = yield
return
}
if ti.yield != nil && !ti.yield(i, v) {
return
}
}
ti.yield = yield
return
}
yield func()
}

func (ti *TrickyIterator) iterAll(s []int) Seq2[int, int] {
return func(yield func(int, int) bool) {
ti.yield = yield // Save yield for future abuse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to leave this in, but commented out, so it goes with the description of the change below.

// NOTE: storing the yield func in the iterator has been removed because
// it make the closure escape to the heap which breaks the .closureptr
// heuristic. Eventually we will need to figure out what to do when that
// happens.
// ti.yield = yield // Save yield for future abuse
for i, v := range s {
if !yield(i, v) {
return
Expand All @@ -277,14 +280,6 @@ func (ti *TrickyIterator) iterAll(s []int) Seq2[int, int] {
}
}

func (ti *TrickyIterator) iterZero(s []int) Seq2[int, int] {
return func(yield func(int, int) bool) {
ti.yield = yield // Save yield for future abuse
// Don't call it at all, maybe it won't escape
return
}
}

// OfSliceIndex returns a Seq2 over the elements of s. It is equivalent
// to range s.
func OfSliceIndex[T any, S ~[]T](s S) Seq2[int, T] {
Expand Down
91 changes: 62 additions & 29 deletions pkg/proc/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ const (
// If localsNoDeclLineCheck the declaration line isn't checked at
// all to determine if the variable is in scope.
localsNoDeclLineCheck

// If localsOnlyRangeBodyClosures is set simpleLocals only returns
// variables containing the range body closure.
localsOnlyRangeBodyClosures
)

// ConvertEvalScope returns a new EvalScope in the context of the
Expand Down Expand Up @@ -330,12 +334,10 @@ func (scope *EvalScope) Locals(flags localsFlags, wantedName string) ([]*Variabl
scopes = append(scopes, vars0)

if scope.rangeFrames == nil {
scope.rangeFrames, err = rangeFuncStackTrace(scope.target, scope.g)
err := scope.setupRangeFrames()
if err != nil {
return nil, err
}
scope.rangeFrames = scope.rangeFrames[1:]
scope.enclosingRangeScopes = make([]*EvalScope, len(scope.rangeFrames))
}
for i, scope2 := range scope.enclosingRangeScopes {
if i == len(scope.enclosingRangeScopes)-1 {
Expand Down Expand Up @@ -373,6 +375,17 @@ func (scope *EvalScope) Locals(flags localsFlags, wantedName string) ([]*Variabl
return vars, nil
}

func (scope *EvalScope) setupRangeFrames() error {
var err error
scope.rangeFrames, err = rangeFuncStackTrace(scope.target, scope.g)
if err != nil {
return err
}
scope.rangeFrames = scope.rangeFrames[1:]
scope.enclosingRangeScopes = make([]*EvalScope, len(scope.rangeFrames))
return nil
}

// simpleLocals returns all local variables in 'scope'.
// This function does not try to merge the scopes of range-over-func closure
// bodies with their enclosing function, for that use (*EvalScope).Locals or
Expand Down Expand Up @@ -406,23 +419,7 @@ func (scope *EvalScope) simpleLocals(flags localsFlags, wantedName string) ([]*V

// look for dictionary entry
if scope.dictAddr == 0 {
for _, entry := range varEntries {
name, _ := entry.Val(dwarf.AttrName).(string)
if name == goDictionaryName {
dictVar, err := extractVarInfoFromEntry(scope.target, scope.BinInfo, scope.image(), scope.Regs, scope.Mem, entry.Tree, 0)
if err != nil {
logflags.DebuggerLogger().Errorf("could not load %s variable: %v", name, err)
} else if dictVar.Unreadable != nil {
logflags.DebuggerLogger().Errorf("could not load %s variable: %v", name, dictVar.Unreadable)
} else {
scope.dictAddr, err = readUintRaw(dictVar.mem, dictVar.Addr, int64(scope.BinInfo.Arch.PtrSize()))
if err != nil {
logflags.DebuggerLogger().Errorf("could not load %s variable: %v", name, err)
}
}
break
}
}
scope.dictAddr = readLocalPtrVar(dwarfTree, goDictionaryName, scope.target, scope.BinInfo, scope.image(), scope.Regs, scope.Mem)
}

vars := make([]*Variable, 0, len(varEntries))
Expand All @@ -434,20 +431,19 @@ func (scope *EvalScope) simpleLocals(flags localsFlags, wantedName string) ([]*V
if name != wantedName && name != "&"+wantedName {
continue
}
default:
if name == goDictionaryName || name == goClosurePtr || strings.HasPrefix(name, "#state") || strings.HasPrefix(name, "&#state") || strings.HasPrefix(name, "#next") || strings.HasPrefix(name, "&#next") || strings.HasPrefix(name, "#yield") {
case flags&localsOnlyRangeBodyClosures != 0:
if !strings.HasPrefix(name, "#yield") && !strings.HasPrefix(name, "&#yield") {
continue
}
}
if scope.Fn.rangeParentName() != "" {
// Skip return values and closure variables for range-over-func closure bodies
if strings.HasPrefix(name, "~") {
continue
}
if entry.Val(godwarf.AttrGoClosureOffset) != nil {
default:
if name == goDictionaryName || name == goClosurePtr || strings.HasPrefix(name, "#yield") || strings.HasPrefix(name, "&#yield") {
continue
}
}
if scope.Fn.rangeParentName() != "" && (strings.HasPrefix(name, "~") || entry.Val(godwarf.AttrGoClosureOffset) != nil) {
// Skip unnamed parameters and closure variables for range-over-func closure bodies
continue
}
val, err := extractVarInfoFromEntry(scope.target, scope.BinInfo, scope.image(), scope.Regs, scope.Mem, entry.Tree, scope.dictAddr)
if err != nil {
// skip variables that we can't parse yet
Expand Down Expand Up @@ -522,6 +518,31 @@ func afterLastArgAddr(vars []*Variable) uint64 {
return 0
}

// readLocalPtrVar reads the value of the local pointer variable vname. This
// is a low level helper function, it does not support nested scopes, range
// resolution across range bodies, type parameters, &c...
func readLocalPtrVar(dwarfTree *godwarf.Tree, vname string, tgt *Target, bi *BinaryInfo, image *Image, regs op.DwarfRegisters, mem MemoryReadWriter) uint64 {
for _, entry := range dwarfTree.Children {
name, _ := entry.Val(dwarf.AttrName).(string)
if name == vname {
v, err := extractVarInfoFromEntry(tgt, bi, image, regs, mem, entry, 0)
if err != nil {
logflags.DebuggerLogger().Errorf("could not load %s variable: %v", name, err)
} else if v.Unreadable != nil {
logflags.DebuggerLogger().Errorf("could not load %s variable: %v", name, v.Unreadable)
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you get rid of this else clause and the break at the end of the block since this branch unconditionally returns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean moving the break into the if and else if branches? Or how?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, it would actually be noisier to change this, it's fine as-is.

r, err := readUintRaw(v.mem, v.Addr, int64(bi.Arch.PtrSize()))
if err != nil {
logflags.DebuggerLogger().Errorf("could not load %s variable: %v", name, err)
}
return r
}
break
}
}
return 0
}

// setValue writes the value of srcv to dstv.
// - If srcv is a numerical literal constant and srcv is of a compatible type
// the necessary type conversion is performed.
Expand Down Expand Up @@ -887,6 +908,8 @@ func (stack *evalStack) resume(g *G) {
scope.Regs.FrameBase = stack.fboff + int64(scope.g.stack.hi)
scope.Regs.CFA = scope.frameOffset + int64(scope.g.stack.hi)
stack.curthread = g.Thread
scope.rangeFrames = nil
scope.enclosingRangeScopes = nil

finished := funcCallStep(scope, stack, g.Thread)
if finished {
Expand Down Expand Up @@ -1002,6 +1025,16 @@ func (stack *evalStack) executeOp() {
case *evalop.PushFrameoff:
stack.push(newConstant(constant.MakeInt64(scope.frameOffset), scope.Mem))

case *evalop.PushRangeParentOffset:
if scope.rangeFrames == nil {
stack.err = scope.setupRangeFrames()
}
if len(scope.rangeFrames) > 0 {
stack.push(newConstant(constant.MakeInt64(scope.rangeFrames[len(scope.rangeFrames)-2].FrameOffset()), scope.Mem))
} else {
stack.push(newConstant(constant.MakeInt64(0), scope.Mem))
}

case *evalop.PushThreadID:
stack.push(newConstant(constant.MakeInt64(int64(scope.threadID)), scope.Mem))

Expand Down
3 changes: 3 additions & 0 deletions pkg/proc/evalop/evalcompile.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,9 @@ func (ctx *compileCtx) compileAST(t ast.Expr) error {
case x.Name == "runtime" && node.Sel.Name == "threadid":
ctx.pushOp(&PushThreadID{})

case x.Name == "runtime" && node.Sel.Name == "rangeParentOffset":
ctx.pushOp(&PushRangeParentOffset{})

default:
ctx.pushOp(&PushPackageVarOrSelect{Name: x.Name, Sel: node.Sel.Name})
}
Expand Down
6 changes: 6 additions & 0 deletions pkg/proc/evalop/ops.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ type PushThreadID struct {

func (*PushThreadID) depthCheck() (npop, npush int) { return 0, 1 }

// PushRangeParentOffset pushes the frame offset of the range-over-func closure body parent.
type PushRangeParentOffset struct {
}

func (*PushRangeParentOffset) depthCheck() (npop, npush int) { return 0, 1 }

// PushConst pushes a constant on the stack.
type PushConst struct {
Value constant.Value
Expand Down
Loading