proc: correctly mark closure variables as shadowed (#1674)

If a closure captures a variable but also defines a variable of the
same name in its root scope the shadowed flag would, sometimes, not be
appropriately applied to the captured variable.

This change:

1. sorts the variable list by depth *and* declaration line, so that
closure captured variables always appear before other root-scope
variables, regardless of the order used by the compiler

2. marks variable with the same name as shadowed even if there is only
one scope at play.

This fixes the problem but as a side effect:

1. programs compiled with Go prior to version 1.9 will have the
shadowed flag applied arbitrarily (previously the shadowed flag was not
applied at all)
2. programs compiled with Go prior to versoin 1.11 will still exhibit
the bug, as they do not have DeclLine information.

Fixes #1672

Documentation/KnownBugs.md

@@ -1,6 +1,5 @@
 # Known Bugs
 
-- When a function defines two (or more) variables with the same name delve is unable to distinguish between them: `locals` will print both variables, `print` will randomly pick one. See [Issue #106](https://github.com/go-delve/delve/issues/106).
 - Delve does not currently support 32bit systems. This will usually manifest as a compiler error in `proc/disasm.go`. See [Issue #20](https://github.com/go-delve/delve/issues/20).
 - When Delve is compiled with versions of go prior to 1.7.0 it is not possible to set a breakpoint on a function in a remote package using the `Receiver.MethodName` syntax. See [Issue #528](https://github.com/go-delve/delve/issues/528).
-- When running Delve on binaries compiled with a version of go prior to 1.9.0 `locals` will print all local variables, including ones that are out of scope. If there are multiple variables defined with the same name in the current function `print` will not be able to select the correct one for the current line.
+- When running Delve on binaries compiled with a version of go prior to 1.9.0 `locals` will print all local variables, including ones that are out of scope, the shadowed flag will be applied arbitrarily. If there are multiple variables defined with the same name in the current function `print` will not be able to select the correct one for the current line.

_fixtures/scopetest.go

@@ -142,6 +142,18 @@ func TestClosureScope() {
 	f(3)
 	f1(b)
 }
+func TestClosureShadow() {
+	v := 1
+	closure := func() {
+		f1(v) // v int = 1
+		v := 2
+		f1(v) // v int = 1, v int = 2
+		for i := 0; i < 1; i++ {
+			f1(v) // v int = 1, v int = 2, i int = 0
+		}
+	}
+	closure()
+}
 func main() {
 	ch <- 12
 	TestNestedFor()
@@ -162,4 +174,5 @@ func main() {
 	TestDiscontiguousRanges()
 	TestDiscontiguousRanges()
 	TestClosureScope()
+	TestClosureShadow()
 }

pkg/proc/eval.go

@@ -105,7 +105,6 @@ func (scope *EvalScope) Locals() ([]*Variable, error) {
 	var vars []*Variable
 	var depths []int
 	varReader := reader.Variables(scope.image().dwarf, scope.Fn.offset, reader.ToRelAddr(scope.PC, scope.image().StaticBase), scope.Line, true, false)
-	hasScopes := false
 	for varReader.Next() {
 		entry := varReader.Entry()
 		val, err := extractVarInfoFromEntry(scope.BinInfo, scope.image(), scope.Regs, scope.Mem, entry)
@@ -127,9 +126,6 @@ func (scope *EvalScope) Locals() ([]*Variable, error) {
 			}
 		}
 		depths = append(depths, depth)
-		if depth > 1 {
-			hasScopes = true
-		}
 	}
 
 	if err := varReader.Err(); err != nil {
@@ -140,9 +136,7 @@ func (scope *EvalScope) Locals() ([]*Variable, error) {
 		return vars, nil
 	}
 
-	if hasScopes {
-		sort.Stable(&variablesByDepth{vars, depths})
-	}
+	sort.Stable(&variablesByDepthAndDeclLine{vars, depths})
 
 	lvn := map[string]*Variable{} // lvn[n] is the last variable we saw named n
 
@@ -160,13 +154,11 @@ func (scope *EvalScope) Locals() ([]*Variable, error) {
 			v.DeclLine = declLine
 			vars[i] = v
 		}
-		if hasScopes {
 		if otherv := lvn[v.Name]; otherv != nil {
 			otherv.Flags |= VariableShadowed
 		}
 		lvn[v.Name] = v
 	}
-	}
 
 	return vars, nil
 }

pkg/proc/scope_test.go

@@ -58,9 +58,10 @@ func TestScopeWithEscapedVariable(t *testing.T) {
 // the = and the initial value are optional and can only be specified if the
 // type is an integer type, float32, float64 or bool.
 //
-// If multiple variables with the same name are specified
-// LocalVariables+FunctionArguments should return them in the same order and
-// EvalExpression should return the last one.
+// If multiple variables with the same name are specified:
+// 1. LocalVariables+FunctionArguments should return them in the same order and
+//    every variable except the last one should be marked as shadowed
+// 2. EvalExpression should return the last one.
 func TestScope(t *testing.T) {
 	if ver, _ := goversion.Parse(runtime.Version()); ver.Major >= 0 && !ver.AfterOrEqual(goversion.GoVersion{1, 9, -1, 0, 0, ""}) {
 		return
@@ -94,16 +95,12 @@ func TestScope(t *testing.T) {
 
 			scope, _ := scopeCheck.checkLocalsAndArgs(p, t)
 
-			var prev *varCheck
 			for i := range scopeCheck.varChecks {
 				vc := &scopeCheck.varChecks[i]
-				if prev != nil && prev.name != vc.name {
-					prev.checkInScope(scopeCheck.line, scope, t)
+				if vc.shdw {
+					continue
 				}
-				prev = vc
-			}
-			if prev != nil {
-				prev.checkInScope(scopeCheck.line, scope, t)
+				vc.checkInScope(scopeCheck.line, scope, t)
 			}
 
 			scopeCheck.ok = true
@@ -130,6 +127,7 @@ type varCheck struct {
 	name     string
 	typ      string
 	kind     reflect.Kind
+	shdw     bool // this variable should be shadowed
 	hasVal   bool
 	intVal   int64
 	uintVal  uint64
@@ -229,7 +227,12 @@ func (check *scopeCheck) Parse(descr string, t *testing.T) {
 			if err != nil {
 				t.Fatalf("could not parse scope comment %q: %v", descr, err)
 			}
+		}
+	}
 
+	for i := 1; i < len(check.varChecks); i++ {
+		if check.varChecks[i-1].name == check.varChecks[i].name {
+			check.varChecks[i-1].shdw = true
 		}
 	}
 }
@@ -295,6 +298,10 @@ func (varCheck *varCheck) check(line int, v *proc.Variable, t *testing.T, ctxt s
 		t.Errorf("%d: wrong type for %s (%s), got %s, expected %s", line, v.Name, ctxt, typ, varCheck.typ)
 	}
 
+	if varCheck.shdw && v.Flags&proc.VariableShadowed == 0 {
+		t.Errorf("%d: expected shadowed %s variable", line, v.Name)
+	}
+
 	if !varCheck.hasVal {
 		return
 	}

pkg/proc/variables.go

@@ -1946,16 +1946,21 @@ func (ctyp *constantType) describe(n int64) string {
 	return ""
 }
 
-type variablesByDepth struct {
+type variablesByDepthAndDeclLine struct {
 	vars   []*Variable
 	depths []int
 }
 
-func (v *variablesByDepth) Len() int { return len(v.vars) }
+func (v *variablesByDepthAndDeclLine) Len() int { return len(v.vars) }
 
-func (v *variablesByDepth) Less(i int, j int) bool { return v.depths[i] < v.depths[j] }
+func (v *variablesByDepthAndDeclLine) Less(i int, j int) bool {
+	if v.depths[i] == v.depths[j] {
+		return v.vars[i].DeclLine < v.vars[j].DeclLine
+	}
+	return v.depths[i] < v.depths[j]
+}
 
-func (v *variablesByDepth) Swap(i int, j int) {
+func (v *variablesByDepthAndDeclLine) Swap(i int, j int) {
 	v.depths[i], v.depths[j] = v.depths[j], v.depths[i]
 	v.vars[i], v.vars[j] = v.vars[j], v.vars[i]
 }