From 48aa163c1c6aaaa6a15da7ec25411bfa499ac41a Mon Sep 17 00:00:00 2001 From: aarzilli Date: Wed, 29 May 2024 17:25:40 +0200 Subject: [PATCH] proc: refactor identifier evaluation for range-over-func support Because of how range-over-func is implemented it is difficult to determine the set of visible local variables during expression compilation (i.e. it is difficulto to keep the HasLocal function correct). This commit moves that logic from expression compilation to expression evaluation. Updates #3733 --- pkg/proc/eval.go | 265 ++++++++++++++++----------------- pkg/proc/evalop/evalcompile.go | 47 +----- pkg/proc/evalop/ops.go | 27 ++-- pkg/proc/proc_test.go | 2 + pkg/proc/variables_test.go | 12 +- 5 files changed, 157 insertions(+), 196 deletions(-) diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index eba84c7bc4..ad18070ac2 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -203,81 +203,6 @@ func (s scopeToEvalLookup) FindTypeExpr(expr ast.Expr) (godwarf.Type, error) { return s.BinInfo.findTypeExpr(expr) } -func (scope scopeToEvalLookup) HasLocal(name string) bool { - if scope.Fn == nil { - return false - } - - flags := reader.VariablesOnlyVisible - if scope.BinInfo.Producer() != "" && goversion.ProducerAfterOrEqual(scope.BinInfo.Producer(), 1, 15) { - flags |= reader.VariablesTrustDeclLine - } - - dwarfTree, err := scope.image().getDwarfTree(scope.Fn.offset) - if err != nil { - return false - } - - varEntries := reader.Variables(dwarfTree, scope.PC, scope.Line, flags) - for _, entry := range varEntries { - curname, _ := entry.Val(dwarf.AttrName).(string) - if curname == name { - return true - } - if len(curname) > 0 && curname[0] == '&' { - if curname[1:] == name { - return true - } - } - } - return false -} - -func (scope scopeToEvalLookup) HasGlobal(pkgName, varName string) bool { - hasGlobalInternal := func(name string) bool { - for _, pkgvar := range scope.BinInfo.packageVars { - if pkgvar.name == name || strings.HasSuffix(pkgvar.name, "/"+name) { - return true - } - } - for _, fn := range scope.BinInfo.Functions { - if fn.Name == name || strings.HasSuffix(fn.Name, "/"+name) { - return true - } - } - for _, ctyp := range scope.BinInfo.consts { - for _, cval := range ctyp.values { - if cval.fullName == name || strings.HasSuffix(cval.fullName, "/"+name) { - return true - } - } - } - return false - } - - if pkgName == "" { - if scope.Fn == nil { - return false - } - return hasGlobalInternal(scope.Fn.PackageName() + "." + varName) - } - - for _, pkgPath := range scope.BinInfo.PackageMap[pkgName] { - if hasGlobalInternal(pkgPath + "." + varName) { - return true - } - } - return hasGlobalInternal(pkgName + "." + varName) -} - -func (scope scopeToEvalLookup) LookupRegisterName(name string) (int, bool) { - s := validRegisterName(name) - if s == "" { - return 0, false - } - return scope.BinInfo.Arch.RegisterNameToDwarf(s) -} - func (scope scopeToEvalLookup) HasBuiltin(name string) bool { return supportedBuiltins[name] != nil } @@ -680,7 +605,20 @@ func (scope *EvalScope) findGlobal(pkgName, varName string) (*Variable, error) { if err != nil || v != nil { return v, err } - return nil, fmt.Errorf("could not find symbol value for %s.%s", pkgName, varName) + return nil, &errCouldNotFindSymbol{fmt.Sprintf("%s.%s", pkgName, varName)} +} + +type errCouldNotFindSymbol struct { + name string +} + +func (e *errCouldNotFindSymbol) Error() string { + return fmt.Sprintf("could not find symbol %s", e.name) +} + +func isSymbolNotFound(e error) bool { + _, ok := e.(*errCouldNotFindSymbol) + return ok } func (scope *EvalScope) findGlobalInternal(name string) (*Variable, error) { @@ -967,30 +905,7 @@ func (stack *evalStack) executeOp() { stack.push(newConstant(op.Value, scope.Mem)) case *evalop.PushLocal: - var vars []*Variable - var err error - if op.Frame != 0 { - frameScope, err2 := ConvertEvalScope(scope.target, -1, int(op.Frame), 0) - if err2 != nil { - stack.err = err2 - return - } - vars, err = frameScope.Locals(0) - } else { - vars, err = scope.Locals(0) - } - if err != nil { - stack.err = err - return - } - found := false - for i := range vars { - if vars[i].Name == op.Name && vars[i].Flags&VariableShadowed == 0 { - stack.push(vars[i]) - found = true - break - } - } + found := stack.pushLocal(scope, op.Name, op.Frame) if !found { stack.err = fmt.Errorf("could not find symbol value for %s", op.Name) } @@ -998,52 +913,36 @@ func (stack *evalStack) executeOp() { case *evalop.PushNil: stack.push(nilVariable) - case *evalop.PushRegister: - reg := scope.Regs.Reg(uint64(op.Regnum)) - if reg == nil { - stack.err = fmt.Errorf("could not find symbol value for %s", op.Regname) + case *evalop.PushPackageVarOrSelect: + v, err := scope.findGlobal(op.Name, op.Sel) + if err != nil && !isSymbolNotFound(err) { + stack.err = err return } - reg.FillBytes() - - var typ godwarf.Type - if len(reg.Bytes) <= 8 { - typ = godwarf.FakeBasicType("uint", 64) + if v != nil { + stack.push(v) } else { - var err error - typ, err = scope.BinInfo.findType("string") - if err != nil { - stack.err = err + if op.NameIsString { + stack.err = fmt.Errorf("%q (type string) is not a struct", op.Name) return } - } + found := stack.pushIdent(scope, op.Name) + if stack.err != nil { + return + } + if found { + scope.evalStructSelector(&evalop.Select{Name: op.Sel}, stack) + } else { + stack.err = fmt.Errorf("could not find symbol value for %s", op.Name) + } - v := newVariable(op.Regname, 0, typ, scope.BinInfo, scope.Mem) - if v.Kind == reflect.String { - v.Len = int64(len(reg.Bytes) * 2) - v.Base = fakeAddressUnresolv } - v.Addr = fakeAddressUnresolv - v.Flags = VariableCPURegister - v.reg = reg - stack.push(v) - case *evalop.PushPackageVar: - pkgName := op.PkgName - replaceName := false - if pkgName == "" { - replaceName = true - pkgName = scope.Fn.PackageName() - } - v, err := scope.findGlobal(pkgName, op.Name) - if err != nil { - stack.err = err - return - } - if replaceName { - v.Name = op.Name + case *evalop.PushIdent: + found := stack.pushIdent(scope, op.Name) + if !found { + stack.err = fmt.Errorf("could not find symbol value for %s", op.Name) } - stack.push(v) case *evalop.PushLen: v := stack.peek() @@ -1134,6 +1033,98 @@ func (stack *evalStack) executeOp() { stack.opidx++ } +func (stack *evalStack) pushLocal(scope *EvalScope, name string, frame int64) (found bool) { + var vars []*Variable + var err error + if frame != 0 { + frameScope, err2 := ConvertEvalScope(scope.target, -1, int(frame), 0) + if err2 != nil { + stack.err = err2 + return + } + vars, err = frameScope.Locals(0) + } else { + vars, err = scope.Locals(0) + } + if err != nil { + stack.err = err + return + } + found = false + for i := range vars { + if vars[i].Name == name && vars[i].Flags&VariableShadowed == 0 { + stack.push(vars[i]) + found = true + break + } + } + return found +} + +func (stack *evalStack) pushIdent(scope *EvalScope, name string) (found bool) { + found = stack.pushLocal(scope, name, 0) + if found || stack.err != nil { + return found + } + v, err := scope.findGlobal(scope.Fn.PackageName(), name) + if err != nil && !isSymbolNotFound(err) { + stack.err = err + return false + } + if v != nil { + v.Name = name + stack.push(v) + return true + } + + switch name { + case "true", "false": + stack.push(newConstant(constant.MakeBool(name == "true"), scope.Mem)) + return true + case "nil": + stack.push(nilVariable) + return true + } + + regname := validRegisterName(name) + if regname == "" { + return false + } + regnum, ok := scope.BinInfo.Arch.RegisterNameToDwarf(regname) + if !ok { + return false + } + + reg := scope.Regs.Reg(uint64(regnum)) + if reg == nil { + return + } + reg.FillBytes() + + var typ godwarf.Type + if len(reg.Bytes) <= 8 { + typ = godwarf.FakeBasicType("uint", 64) + } else { + var err error + typ, err = scope.BinInfo.findType("string") + if err != nil { + stack.err = err + return false + } + } + + v = newVariable(regname, 0, typ, scope.BinInfo, scope.Mem) + if v.Kind == reflect.String { + v.Len = int64(len(reg.Bytes) * 2) + v.Base = fakeAddressUnresolv + } + v.Addr = fakeAddressUnresolv + v.Flags = VariableCPURegister + v.reg = reg + stack.push(v) + return true +} + func (scope *EvalScope) evalAST(t ast.Expr) (*Variable, error) { ops, err := evalop.CompileAST(scopeToEvalLookup{scope}, t) if err != nil { diff --git a/pkg/proc/evalop/evalcompile.go b/pkg/proc/evalop/evalcompile.go index 79a8f30586..1f27e711a5 100644 --- a/pkg/proc/evalop/evalcompile.go +++ b/pkg/proc/evalop/evalcompile.go @@ -30,10 +30,7 @@ type compileCtx struct { type evalLookup interface { FindTypeExpr(ast.Expr) (godwarf.Type, error) - HasLocal(string) bool - HasGlobal(string, string) bool HasBuiltin(string) bool - LookupRegisterName(string) (int, bool) } // CompileAST compiles the expression t into a list of instructions. @@ -221,15 +218,8 @@ func (ctx *compileCtx) compileAST(t ast.Expr) error { case x.Name == "runtime" && node.Sel.Name == "threadid": ctx.pushOp(&PushThreadID{}) - case ctx.HasLocal(x.Name): - ctx.pushOp(&PushLocal{Name: x.Name}) - ctx.pushOp(&Select{node.Sel.Name}) - - case ctx.HasGlobal(x.Name, node.Sel.Name): - ctx.pushOp(&PushPackageVar{x.Name, node.Sel.Name}) - default: - return ctx.compileUnary(node.X, &Select{node.Sel.Name}) + ctx.pushOp(&PushPackageVarOrSelect{Name: x.Name, Sel: node.Sel.Name}) } case *ast.CallExpr: @@ -258,11 +248,7 @@ func (ctx *compileCtx) compileAST(t ast.Expr) error { if err != nil { return err } - if ctx.HasGlobal(s, node.Sel.Name) { - ctx.pushOp(&PushPackageVar{s, node.Sel.Name}) - return nil - } - return ctx.compileUnary(node.X, &Select{node.Sel.Name}) + ctx.pushOp(&PushPackageVarOrSelect{Name: s, Sel: node.Sel.Name, NameIsString: true}) default: return ctx.compileUnary(node.X, &Select{node.Sel.Name}) @@ -367,13 +353,10 @@ func (ctx *compileCtx) compileTypeCastOrFuncCall(node *ast.CallExpr) error { } return ctx.compileFunctionCall(node) case *ast.Ident: - if ctx.HasBuiltin(n.Name) { - return ctx.compileFunctionCall(node) - } - if ctx.HasGlobal("", n.Name) || ctx.HasLocal(n.Name) { - return ctx.compileFunctionCall(node) + if typ, _ := ctx.FindTypeExpr(n); typ != nil { + return ctx.compileTypeCast(node, fmt.Errorf("could not find symbol value for %s", n.Name)) } - return ctx.compileTypeCast(node, fmt.Errorf("could not find symbol value for %s", n.Name)) + return ctx.compileFunctionCall(node) case *ast.IndexExpr: // Ambiguous, could be a parametric type switch n.X.(type) { @@ -438,25 +421,7 @@ func (ctx *compileCtx) compileBuiltinCall(builtin string, args []ast.Expr) error } func (ctx *compileCtx) compileIdent(node *ast.Ident) error { - switch { - case ctx.HasLocal(node.Name): - ctx.pushOp(&PushLocal{Name: node.Name}) - case ctx.HasGlobal("", node.Name): - ctx.pushOp(&PushPackageVar{"", node.Name}) - case node.Name == "true" || node.Name == "false": - ctx.pushOp(&PushConst{constant.MakeBool(node.Name == "true")}) - case node.Name == "nil": - ctx.pushOp(&PushNil{}) - default: - found := false - if regnum, ok := ctx.LookupRegisterName(node.Name); ok { - ctx.pushOp(&PushRegister{regnum, node.Name}) - found = true - } - if !found { - return fmt.Errorf("could not find symbol value for %s", node.Name) - } - } + ctx.pushOp(&PushIdent{node.Name}) return nil } diff --git a/pkg/proc/evalop/ops.go b/pkg/proc/evalop/ops.go index 3cdb2ac3e9..01bad955dc 100644 --- a/pkg/proc/evalop/ops.go +++ b/pkg/proc/evalop/ops.go @@ -45,26 +45,29 @@ type PushLocal struct { func (*PushLocal) depthCheck() (npop, npush int) { return 0, 1 } -// PushNil pushes an untyped nil on the stack. -type PushNil struct { +// PushIdent pushes a local or global variable or a predefined identifier +// (true, false, etc) or the value of a register on the stack. +type PushIdent struct { + Name string } -func (*PushNil) depthCheck() (npop, npush int) { return 0, 1 } +func (*PushIdent) depthCheck() (npop, npush int) { return 0, 1 } -// PushRegister pushes the CPU register Regnum on the stack. -type PushRegister struct { - Regnum int - Regname string +// PushPackageVarOrSelect pushes the value of Name.Sel on the stack, which +// could either be a global variable (with the package name specified), or a +// field of a local variable. +type PushPackageVarOrSelect struct { + Name, Sel string + NameIsString bool } -func (*PushRegister) depthCheck() (npop, npush int) { return 0, 1 } +func (*PushPackageVarOrSelect) depthCheck() (npop, npush int) { return 0, 1 } -// PushPackageVar pushes a package variable on the stack. -type PushPackageVar struct { - PkgName, Name string // if PkgName == "" use current function's package +// PushNil pushes an untyped nil on the stack. +type PushNil struct { } -func (*PushPackageVar) depthCheck() (npop, npush int) { return 0, 1 } +func (*PushNil) depthCheck() (npop, npush int) { return 0, 1 } // PushLen pushes the length of the variable at the top of the stack into // the stack. diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index ef780617f6..22e078065f 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -143,6 +143,7 @@ func dataAtAddr(thread proc.MemoryReadWriter, addr uint64) ([]byte, error) { } func assertNoError(err error, t testing.TB, s string) { + t.Helper() if err != nil { _, file, line, _ := runtime.Caller(1) fname := filepath.Base(file) @@ -1246,6 +1247,7 @@ func evalVariableOrError(p *proc.Target, symbol string) (*proc.Variable, error) } func evalVariable(p *proc.Target, t testing.TB, symbol string) *proc.Variable { + t.Helper() v, err := evalVariableOrError(p, symbol) if err != nil { _, file, line, _ := runtime.Caller(1) diff --git a/pkg/proc/variables_test.go b/pkg/proc/variables_test.go index 277ce6ee64..b9da651e96 100644 --- a/pkg/proc/variables_test.go +++ b/pkg/proc/variables_test.go @@ -820,9 +820,9 @@ func getEvalExpressionTestCases() []varTest { {"afunc(2)", false, "", "", "", errors.New("function calls not allowed without using 'call'")}, {"(afunc)(2)", false, "", "", "", errors.New("function calls not allowed without using 'call'")}, {"(*afunc)(2)", false, "", "", "", errors.New("expression \"afunc\" (func()) can not be dereferenced")}, - {"unknownthing(2)", false, "", "", "", errors.New("could not evaluate function or type unknownthing: could not find symbol value for unknownthing")}, - {"(*unknownthing)(2)", false, "", "", "", errors.New("could not evaluate function or type (*unknownthing): could not find symbol value for unknownthing")}, - {"(*strings.Split)(2)", false, "", "", "", errors.New("could not evaluate function or type (*strings.Split): could not find symbol value for strings")}, + {"unknownthing(2)", false, "", "", "", errors.New("could not find symbol value for unknownthing")}, + {"(*unknownthing)(2)", false, "", "", "", errors.New("could not find symbol value for unknownthing")}, + {"(*strings.Split)(2)", false, "", "", "", errors.New("could not find symbol value for strings")}, // pretty printing special types {"tim1", false, `time.Time(1977-05-25T18:00:00Z)…`, `time.Time(1977-05-25T18:00:00Z)…`, "time.Time", nil}, @@ -1195,7 +1195,7 @@ func TestCallFunction(t *testing.T) { {`stringsJoin(nil, "")`, []string{`:string:""`}, nil}, {`stringsJoin(stringslice, comma)`, []string{`:string:"one,two,three"`}, nil}, {`stringsJoin(stringslice, "~~")`, []string{`:string:"one~~two~~three"`}, nil}, - {`stringsJoin(s1, comma)`, nil, errors.New(`error evaluating "s1" as argument 1 in function stringsJoin: could not find symbol value for s1`)}, + {`stringsJoin(s1, comma)`, nil, errors.New(`could not find symbol value for s1`)}, {`stringsJoin(intslice, comma)`, nil, errors.New("can not convert value of type []int to []string")}, {`noreturncall(2)`, nil, nil}, @@ -1295,11 +1295,11 @@ func TestCallFunction(t *testing.T) { } var testcasesBefore114After112 = []testCaseCallFunction{ - {`strings.Join(s1, comma)`, nil, errors.New(`error evaluating "s1" as argument 1 in function strings.Join: could not find symbol value for s1`)}, + {`strings.Join(s1, comma)`, nil, errors.New(`could not find symbol value for s1`)}, } var testcases114 = []testCaseCallFunction{ - {`strings.Join(s1, comma)`, nil, errors.New(`error evaluating "s1" as argument 1 in function strings.Join: could not find symbol value for s1`)}, + {`strings.Join(s1, comma)`, nil, errors.New(`could not find symbol value for s1`)}, } var testcases117 = []testCaseCallFunction{