From 7fd47749ef0d33a4d809a00cefbe931e6901753b Mon Sep 17 00:00:00 2001 From: aarzilli Date: Sun, 15 Apr 2018 14:11:27 +0200 Subject: [PATCH] proc: Flag shadowed arguments as shadowed Fixes #951 --- _fixtures/issue951.go | 21 +++++++++++++++ pkg/proc/eval.go | 12 +-------- pkg/proc/proc.go | 2 +- pkg/proc/proc_test.go | 32 +++++++++++++++++++++++ pkg/proc/variables.go | 61 ++++++++++++++++++++++++++++++++++++------- service/api/types.go | 6 +++++ 6 files changed, 112 insertions(+), 22 deletions(-) create mode 100644 _fixtures/issue951.go diff --git a/_fixtures/issue951.go b/_fixtures/issue951.go new file mode 100644 index 00000000..5d50a066 --- /dev/null +++ b/_fixtures/issue951.go @@ -0,0 +1,21 @@ +package main + +import ( + "fmt" + "runtime" +) + +func main() { + shadow(42) +} + +func shadow(i int) { + for i := 0; i < 5; i++ { + for i := 20; i < 25; i++ { + runtime.Breakpoint() + fmt.Println("another shadow", i) + } + fmt.Println("shadow", i) + } + fmt.Println("arg", i) +} diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index d927fb4e..3c2fcd78 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -2,7 +2,6 @@ package proc import ( "bytes" - "debug/dwarf" "encoding/binary" "errors" "fmt" @@ -564,7 +563,7 @@ func (scope *EvalScope) evalIdent(node *ast.Ident) (*Variable, error) { return nilVariable, nil } - vars, err := scope.variablesByTag(dwarf.TagVariable, nil) + vars, err := scope.Locals() if err != nil { return nil, err } @@ -573,15 +572,6 @@ func (scope *EvalScope) evalIdent(node *ast.Ident) (*Variable, error) { return vars[i], nil } } - args, err := scope.variablesByTag(dwarf.TagFormalParameter, nil) - if err != nil { - return nil, err - } - for i := range args { - if args[i].Name == node.Name { - return args[i], nil - } - } // if it's not a local variable then it could be a package variable w/o explicit package name if scope.Fn != nil { diff --git a/pkg/proc/proc.go b/pkg/proc/proc.go index a68a280f..82b92959 100644 --- a/pkg/proc/proc.go +++ b/pkg/proc/proc.go @@ -524,7 +524,7 @@ func FrameToScope(bi *BinaryInfo, thread MemoryReadWriter, g *G, frames ...Stack // Creates a cacheMem that will preload the entire stack frame the first // time any local variable is read. - // Remember that the stack grows from downward in memory. + // Remember that the stack grows downward in memory. minaddr := frames[0].Regs.SP() var maxaddr uint64 if len(frames) > 1 && frames[0].SystemStack == frames[1].SystemStack { diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index 872b03e7..f0871982 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -3665,3 +3665,35 @@ func TestInlineStepOut(t *testing.T) { {contStepout, 18}, }) } + +func TestIssue951(t *testing.T) { + if ver, _ := goversion.Parse(runtime.Version()); ver.Major >= 0 && !ver.AfterOrEqual(goversion.GoVersion{1, 9, -1, 0, 0, ""}) { + t.Skip("scopes not implemented in <=go1.8") + } + + withTestProcess("issue951", t, func(p proc.Process, fixture protest.Fixture) { + assertNoError(proc.Continue(p), t, "Continue()") + scope, err := proc.GoroutineScope(p.CurrentThread()) + assertNoError(err, t, "GoroutineScope") + args, err := scope.FunctionArguments(normalLoadConfig) + assertNoError(err, t, "FunctionArguments") + t.Logf("%#v", args[0]) + if args[0].Flags&proc.VariableShadowed == 0 { + t.Error("argument is not shadowed") + } + vars, err := scope.LocalVariables(normalLoadConfig) + assertNoError(err, t, "LocalVariables") + shadowed, notShadowed := 0, 0 + for i := range vars { + t.Logf("var %d: %#v\n", i, vars[i]) + if vars[i].Flags&proc.VariableShadowed != 0 { + shadowed++ + } else { + notShadowed++ + } + } + if shadowed != 1 || notShadowed != 1 { + t.Errorf("Wrong number of shadowed/non-shadowed local variables: %d %d", shadowed, notShadowed) + } + }) +} diff --git a/pkg/proc/variables.go b/pkg/proc/variables.go index 79c3498d..3261a2ea 100644 --- a/pkg/proc/variables.go +++ b/pkg/proc/variables.go @@ -58,6 +58,10 @@ const ( VariableShadowed // VariableConstant means this variable is a constant value VariableConstant + // VariableArgument means this variable is a function argument + VariableArgument + // VariableReturnArgument means this variable is a function return value + VariableReturnArgument ) // Variable represents a variable. It contains the address, name, @@ -616,12 +620,38 @@ func (scope *EvalScope) SetVariable(name, value string) error { // LocalVariables returns all local variables from the current function scope. func (scope *EvalScope) LocalVariables(cfg LoadConfig) ([]*Variable, error) { - return scope.variablesByTag(dwarf.TagVariable, &cfg) + vars, err := scope.Locals() + if err != nil { + return nil, err + } + vars = filterVariables(vars, func(v *Variable) bool { + return (v.Flags & (VariableArgument | VariableReturnArgument)) == 0 + }) + loadValues(vars, cfg) + return vars, nil } // FunctionArguments returns the name, value, and type of all current function arguments. func (scope *EvalScope) FunctionArguments(cfg LoadConfig) ([]*Variable, error) { - return scope.variablesByTag(dwarf.TagFormalParameter, &cfg) + vars, err := scope.Locals() + if err != nil { + return nil, err + } + vars = filterVariables(vars, func(v *Variable) bool { + return (v.Flags & (VariableArgument | VariableReturnArgument)) != 0 + }) + loadValues(vars, cfg) + return vars, nil +} + +func filterVariables(vars []*Variable, pred func(v *Variable) bool) []*Variable { + r := make([]*Variable, 0, len(vars)) + for i := range vars { + if pred(vars[i]) { + r = append(r, vars[i]) + } + } + return r } // PackageVariables returns the name, value, and type of all package variables in the application. @@ -825,6 +855,12 @@ func (v *Variable) maybeDereference() *Variable { } } +func loadValues(vars []*Variable, cfg LoadConfig) { + for i := range vars { + vars[i].loadValueInternal(0, cfg) + } +} + // Extracts the value of the variable at the given address. func (v *Variable) loadValue(cfg LoadConfig) { v.loadValueInternal(0, cfg) @@ -1876,20 +1912,17 @@ func (v *variablesByDepth) Swap(i int, j int) { } // Fetches all variables of a specific type in the current function scope -func (scope *EvalScope) variablesByTag(tag dwarf.Tag, cfg *LoadConfig) ([]*Variable, error) { +func (scope *EvalScope) Locals() ([]*Variable, error) { if scope.Fn == nil { return nil, errors.New("unable to find function context") } var vars []*Variable var depths []int - varReader := reader.Variables(scope.BinInfo.dwarf, scope.Fn.offset, scope.PC, scope.Line, tag == dwarf.TagVariable) + varReader := reader.Variables(scope.BinInfo.dwarf, scope.Fn.offset, scope.PC, scope.Line, true) hasScopes := false for varReader.Next() { entry := varReader.Entry() - if entry.Tag != tag { - continue - } val, err := scope.extractVarInfoFromEntry(entry) if err != nil { // skip variables that we can't parse yet @@ -1897,6 +1930,17 @@ func (scope *EvalScope) variablesByTag(tag dwarf.Tag, cfg *LoadConfig) ([]*Varia } vars = append(vars, val) depth := varReader.Depth() + if entry.Tag == dwarf.TagFormalParameter { + if depth <= 1 { + depth = 0 + } + isret, _ := entry.Val(dwarf.AttrVarParam).(bool) + if isret { + val.Flags |= VariableReturnArgument + } else { + val.Flags |= VariableArgument + } + } depths = append(depths, depth) if depth > 1 { hasScopes = true @@ -1933,9 +1977,6 @@ func (scope *EvalScope) variablesByTag(tag dwarf.Tag, cfg *LoadConfig) ([]*Varia } lvn[v.Name] = v } - if cfg != nil { - v.loadValue(*cfg) - } } return vars, nil diff --git a/service/api/types.go b/service/api/types.go index d4691972..21a961bf 100644 --- a/service/api/types.go +++ b/service/api/types.go @@ -170,6 +170,12 @@ const ( // VariableConstant means this variable is a constant value VariableConstant + + // VariableArgument means this variable is a function argument + VariableArgument + + // VariableReturnArgument means this variable is a function return value + VariableReturnArgument ) // Variable describes a variable.