service/dap: auto-loading for fully missing children of nested vars (#2455)

* service/dap: auto-loading for fully missing pointers, structs, maps, slices and arrays

* Add call test

* Add TODO

Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>

_fixtures/testvariables2.go

@@ -315,6 +315,7 @@ func main() {
 	zsvmap := map[string]struct{}{"testkey": struct{}{}}
 	var tm truncatedMap
 	tm.v = []map[string]astruct{m1}
+	var rettm = func() truncatedMap { return tm }
 
 	emptyslice := []string{}
 	emptymap := make(map[string]string)
@@ -362,5 +363,5 @@ func main() {
 	longslice := make([]int, 100, 100)
 
 	runtime.Breakpoint()
-	fmt.Println(i1, i2, i3, p1, pp1, amb1, s1, s3, a0, a1, p2, p3, s2, as1, str1, f1, fn1, fn2, nilslice, nilptr, ch1, chnil, m1, mnil, m2, m3, m4, m5, upnil, up1, i4, i5, i6, err1, err2, errnil, iface1, iface2, ifacenil, arr1, parr, cpx1, const1, iface3, iface4, recursive1, recursive1.x, iface5, iface2fn1, iface2fn2, bencharr, benchparr, mapinf, mainMenu, b, b2, sd, anonstruct1, anonstruct2, anoniface1, anonfunc, mapanonstruct1, ifacearr, efacearr, ni8, ni16, ni32, ni64, pinf, ninf, nan, zsvmap, zsslice, zsvar, tm, errtypednil, emptyslice, emptymap, byteslice, runeslice, bytearray, runearray, longstr, nilstruct, as2, as2.NonPointerRecieverMethod, s4, iface2map, issue1578, ll, unread, w2, w3, w4, w5, longarr, longslice)
+	fmt.Println(i1, i2, i3, p1, pp1, amb1, s1, s3, a0, a1, p2, p3, s2, as1, str1, f1, fn1, fn2, nilslice, nilptr, ch1, chnil, m1, mnil, m2, m3, m4, m5, upnil, up1, i4, i5, i6, err1, err2, errnil, iface1, iface2, ifacenil, arr1, parr, cpx1, const1, iface3, iface4, recursive1, recursive1.x, iface5, iface2fn1, iface2fn2, bencharr, benchparr, mapinf, mainMenu, b, b2, sd, anonstruct1, anonstruct2, anoniface1, anonfunc, mapanonstruct1, ifacearr, efacearr, ni8, ni16, ni32, ni64, pinf, ninf, nan, zsvmap, zsslice, zsvar, tm, rettm, errtypednil, emptyslice, emptymap, byteslice, runeslice, bytearray, runearray, longstr, nilstruct, as2, as2.NonPointerRecieverMethod, s4, iface2map, issue1578, ll, unread, w2, w3, w4, w5, longarr, longslice)
 }

service/dap/server.go

@@ -149,6 +149,7 @@ var defaultArgs = launchAttachArgs{
 
 // DefaultLoadConfig controls how variables are loaded from the target's memory, borrowing the
 // default value from the original vscode-go debug adapter and rpc server.
+// With dlv-dap, users currently do not have a way to adjust these.
 // TODO(polina): Support setting config via launch/attach args or only rely on on-demand loading?
 var DefaultLoadConfig = proc.LoadConfig{
 	FollowPointers:     true,
@@ -1496,33 +1497,88 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
 	if v.Unreadable != nil {
 		return
 	}
-	typeName := api.PrettyTypeName(v.DwarfType)
 
-	// As per https://github.com/go-delve/delve/blob/master/Documentation/api/ClientHowto.md#looking-into-variables,
+	// Some of the types might be fully or partially not loaded based on LoadConfig.
-	// some of the types might be fully or partially not loaded based on LoadConfig. For now, clearly
+	// Those that are fully missing (e.g. due to hitting MaxVariableRecurse), can be reloaded in place.
-	// communicate when values are not fully loaded. TODO(polina): look into loading on demand.
+	// Those that are partially missing (e.g. MaxArrayValues from a large array), need a more creative solution
+	// that is still to be determined. For now, clearly communicate when that happens with additional value labels.
+	// TODO(polina): look into layered/paged loading for truncated strings, arrays, maps and structs.
+	var reloadVariable = func(v *proc.Variable, qualifiedNameOrExpr string) (value string) {
+		// We might be loading variables from the frame that's not topmost, so use
+		// frame-independent address-based expression, not fully-qualified name as per
+		// https://github.com/go-delve/delve/blob/master/Documentation/api/ClientHowto.md#looking-into-variables.
+		// TODO(polina): Get *proc.Variable object from debugger instead. Export and set v.loaded to false
+		// and call v.loadValue gain. It's more efficient, and it's guaranteed to keep working with generics.
+		value = api.ConvertVar(v).SinglelineString()
+		typeName := api.PrettyTypeName(v.DwarfType)
+		loadExpr := fmt.Sprintf("*(*%q)(%#x)", typeName, v.Addr)
+		s.log.Debugf("loading %s (type %s) with %s", qualifiedNameOrExpr, typeName, loadExpr)
+		// Make sure we can load the pointers directly, not by updating just the child
+		// This is not really necessary now because users have no way of setting FollowPointers to false.
+		config := DefaultLoadConfig
+		config.FollowPointers = true
+		vLoaded, err := s.debugger.EvalVariableInScope(-1, 0, 0, loadExpr, config)
+		if err != nil {
+			value += fmt.Sprintf(" - FAILED TO LOAD: %s", err)
+		} else {
+			v.Children = vLoaded.Children
+			value = api.ConvertVar(v).SinglelineString()
+		}
+		return value
+	}
 
 	switch v.Kind {
 	case reflect.UnsafePointer:
 		// Skip child reference
 	case reflect.Ptr:
 		if v.DwarfType != nil && len(v.Children) > 0 && v.Children[0].Addr != 0 && v.Children[0].Kind != reflect.Invalid {
-			if v.Children[0].OnlyAddr {
+			if v.Children[0].OnlyAddr { // Not loaded
-				value = "(not loaded) " + value
+				if v.Addr == 0 {
-			} else {
+					// This is equvalent to the following with the cli:
+					//    (dlv) p &a7
+					//    (**main.FooBar)(0xc0000a3918)
+					//
+					// TODO(polina): what is more appropriate?
+					// Option 1: leave it unloaded because it is a special case
+					// Option 2: load it, but then we have to load the child, not the parent, unlike all others
+					// TODO(polina): see if reloadVariable can be reused here
+					cTypeName := api.PrettyTypeName(v.Children[0].DwarfType)
+					cLoadExpr := fmt.Sprintf("*(*%q)(%#x)", cTypeName, v.Children[0].Addr)
+					s.log.Debugf("loading *(%s) (type %s) with %s", qualifiedNameOrExpr, cTypeName, cLoadExpr)
+					cLoaded, err := s.debugger.EvalVariableInScope(-1, 0, 0, cLoadExpr, DefaultLoadConfig)
+					if err != nil {
+						value += fmt.Sprintf(" - FAILED TO LOAD: %s", err)
+					} else {
+						cLoaded.Name = v.Children[0].Name // otherwise, this will be the pointer expression
+						v.Children = []proc.Variable{*cLoaded}
+						value = api.ConvertVar(v).SinglelineString()
+					}
+				} else {
+					value = reloadVariable(v, qualifiedNameOrExpr)
+				}
+			}
+			if !v.Children[0].OnlyAddr {
 				variablesReference = maybeCreateVariableHandle(v)
 			}
 		}
 	case reflect.Slice, reflect.Array:
 		if v.Len > int64(len(v.Children)) { // Not fully loaded
-			value = fmt.Sprintf("(loaded %d/%d) ", len(v.Children), v.Len) + value
+			if v.Base != 0 && len(v.Children) == 0 { // Fully missing
+				value = reloadVariable(v, qualifiedNameOrExpr)
+			} else { // Partially missing (TODO)
+				value = fmt.Sprintf("(loaded %d/%d) ", len(v.Children), v.Len) + value
+			}
 		}
 		if v.Base != 0 && len(v.Children) > 0 {
 			variablesReference = maybeCreateVariableHandle(v)
 		}
 	case reflect.Map:
 		if v.Len > int64(len(v.Children)/2) { // Not fully loaded
-			value = fmt.Sprintf("(loaded %d/%d) ", len(v.Children)/2, v.Len) + value
+			if len(v.Children) == 0 { // Fully missing
+				value = reloadVariable(v, qualifiedNameOrExpr)
+			} else { // Partially missing (TODO)
+				value = fmt.Sprintf("(loaded %d/%d) ", len(v.Children)/2, v.Len) + value
+			}
 		}
 		if v.Base != 0 && len(v.Children) > 0 {
 			variablesReference = maybeCreateVariableHandle(v)
@@ -1531,27 +1587,20 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
 		// TODO(polina): implement auto-loading here
 	case reflect.Interface:
 		if v.Addr != 0 && len(v.Children) > 0 && v.Children[0].Kind != reflect.Invalid && v.Children[0].Addr != 0 {
-			if v.Children[0].OnlyAddr { // Not fully loaded
+			if v.Children[0].OnlyAddr { // Not loaded
-				// We might be loading variables from the frame that's not topmost, so use
+				value = reloadVariable(v, qualifiedNameOrExpr)
-				// frame-independent address-based expression, not fully-qualified name.
-				loadExpr := fmt.Sprintf("*(*%q)(%#x)", typeName, v.Addr)
-				s.log.Debugf("loading %s (type %s) with %s", qualifiedNameOrExpr, typeName, loadExpr)
-				vLoaded, err := s.debugger.EvalVariableInScope(-1, 0, 0, loadExpr, DefaultLoadConfig)
-				if err != nil {
-					value += fmt.Sprintf(" - FAILED TO LOAD: %s", err)
-				} else {
-					v.Children = vLoaded.Children
-					value = api.ConvertVar(v).SinglelineString()
-				}
 			}
-			// Provide a reference to the child only if it fully loaded
 			if !v.Children[0].OnlyAddr {
 				variablesReference = maybeCreateVariableHandle(v)
 			}
 		}
 	case reflect.Struct:
 		if v.Len > int64(len(v.Children)) { // Not fully loaded
-			value = fmt.Sprintf("(loaded %d/%d) ", len(v.Children), v.Len) + value
+			if len(v.Children) == 0 { // Fully missing
+				value = reloadVariable(v, qualifiedNameOrExpr)
+			} else { // Partially missing (TODO)
+				value = fmt.Sprintf("(loaded %d/%d) ", len(v.Children), v.Len) + value
+			}
 		}
 		if len(v.Children) > 0 {
 			variablesReference = maybeCreateVariableHandle(v)

service/dap/server_test.go

@@ -33,8 +33,12 @@ const noChildren bool = false
 var testBackend string
 
 func TestMain(m *testing.M) {
+	logOutputVal := ""
+	if _, isTeamCityTest := os.LookupEnv("TEAMCITY_VERSION"); isTeamCityTest {
+		logOutputVal = "debugger,dap"
+	}
 	var logOutput string
-	flag.StringVar(&logOutput, "log-output", "", "configures log output")
+	flag.StringVar(&logOutput, "log-output", logOutputVal, "configures log output")
 	flag.Parse()
 	logflags.Setup(logOutput != "", logOutput, "")
 	protest.DefaultTestBackend(&testBackend)
@@ -1424,7 +1428,7 @@ func TestVariablesLoading(t *testing.T) {
 						expectChildren(t, sd, "sd", 5)
 					}
 
-					// Struct fully missing due to hitting LoadConfig.MaxVariableRecurse (also tests evaluateName)
+					// Fully missing struct auto-loaded when reaching LoadConfig.MaxVariableRecurse (also tests evaluateName corner case)
 					ref = expectVarRegex(t, locals, -1, "c1", "c1", `main\.cstruct {pb: \*main\.bstruct {a: \(\*main\.astruct\)\(0x[0-9a-f]+\)}, sa: []\*main\.astruct len: 3, cap: 3, [\*\(\*main\.astruct\)\(0x[0-9a-f]+\),\*\(\*main\.astruct\)\(0x[0-9a-f]+\),\*\(\*main.astruct\)\(0x[0-9a-f]+\)]}`, hasChildren)
 					if ref > 0 {
 						client.VariablesRequest(ref)
@@ -1437,16 +1441,16 @@ func TestVariablesLoading(t *testing.T) {
 							expectChildren(t, c1sa, "c1.sa", 3)
 							ref = expectVarRegex(t, c1sa, 0, `\[0\]`, `c1\.sa\[0\]`, `\*\(\*main\.astruct\)\(0x[0-9a-f]+\)`, hasChildren)
 							if ref > 0 {
+								// Auto-loading of fully missing struc children happens here
 								client.VariablesRequest(ref)
 								c1sa0 := client.ExpectVariablesResponse(t)
 								expectChildren(t, c1sa0, "c1.sa[0]", 1)
-								// TODO(polina): there should be children here once we support auto loading
+								expectVarExact(t, c1sa0, 0, "", "(*c1.sa[0])", "main.astruct {A: 1, B: 2}", hasChildren)
-								expectVarRegex(t, c1sa0, 0, "", `\(\*c1\.sa\[0\]\)`, `\(loaded 0/2\) \(\*main\.astruct\)\(0x[0-9a-f]+\)`, noChildren)
 							}
 						}
 					}
 
-					// Struct fully missing due to hitting LoadConfig.MaxVariableRecurse (also tests evaluteName)
+					// Fully missing struct auto-loaded when hitting LoadConfig.MaxVariableRecurse (also tests evaluteName corner case)
 					ref = expectVarRegex(t, locals, -1, "aas", "aas", `\[\]main\.a len: 1, cap: 1, \[{aas: \[\]main\.a len: 1, cap: 1, \[\(\*main\.a\)\(0x[0-9a-f]+\)\]}\]`, hasChildren)
 					if ref > 0 {
 						client.VariablesRequest(ref)
@@ -1459,16 +1463,22 @@ func TestVariablesLoading(t *testing.T) {
 							expectChildren(t, aas0, "aas[0]", 1)
 							ref = expectVarRegex(t, aas0, 0, "aas", `aas\[0\]\.aas`, `\[\]main\.a len: 1, cap: 1, \[\(\*main\.a\)\(0x[0-9a-f]+\)\]`, hasChildren)
 							if ref > 0 {
+								// Auto-loading of fully missing struct children happens here
 								client.VariablesRequest(ref)
 								aas0aas := client.ExpectVariablesResponse(t)
 								expectChildren(t, aas0aas, "aas[0].aas", 1)
-								// TODO(polina): there should be a child here once we support auto loading - test for "aas[0].aas[0].aas"
+								ref = expectVarRegex(t, aas0aas, 0, "[0]", `aas\[0\]\.aas\[0\]`, `main\.a {aas: \[\]main\.a len: 1, cap: 1, \[\(\*main\.a\)\(0x[0-9a-f]+\)\]}`, hasChildren)
-								expectVarRegex(t, aas0aas, 0, "[0]", `aas\[0\]\.aas\[0\]`, `\(loaded 0/1\) \(\*main\.a\)\(0x[0-9a-f]+\)`, noChildren)
+								if ref > 0 {
+									client.VariablesRequest(ref)
+									aas0aas0 := client.ExpectVariablesResponse(t)
+									expectChildren(t, aas0aas, "aas[0].aas[0]", 1)
+									expectVarRegex(t, aas0aas0, 0, "aas", `aas\[0\]\.aas\[0\]\.aas`, `\[\]main\.a len: 1, cap: 1, \[\(\*main\.a\)\(0x[0-9a-f]+\)\]`, hasChildren)
+								}
 							}
 						}
 					}
 
-					// Map fully missing due to hitting LoadConfig.MaxVariableRecurse (also tests evaluateName)
+					// Fully missing map auto-loaded when hitting LoadConfig.MaxVariableRecurse (also tests evaluateName corner case)
 					ref = expectVarExact(t, locals, -1, "tm", "tm", "main.truncatedMap {v: []map[string]main.astruct len: 1, cap: 1, [[...]]}", hasChildren)
 					if ref > 0 {
 						client.VariablesRequest(ref)
@@ -1476,13 +1486,54 @@ func TestVariablesLoading(t *testing.T) {
 						expectChildren(t, tm, "tm", 1)
 						ref = expectVarExact(t, tm, 0, "v", "tm.v", "[]map[string]main.astruct len: 1, cap: 1, [[...]]", hasChildren)
 						if ref > 0 {
+							// Auto-loading of fully missing map chidlren happens here, but they get trancated at MaxArrayValuess
 							client.VariablesRequest(ref)
 							tmV := client.ExpectVariablesResponse(t)
 							expectChildren(t, tmV, "tm.v", 1)
-							// TODO(polina): there should be children here once we support auto loading - test for "tm.v[0]["gutters"]"
+							ref = expectVarRegex(t, tmV, 0, `\[0\]`, `tm\.v\[0\]`, `map\[string\]main\.astruct \[.+\.\.\.\+2 more\]`, hasChildren)
-							expectVarExact(t, tmV, 0, "[0]", "tm.v[0]", "(loaded 0/66) map[string]main.astruct [...]", noChildren)
+							if ref > 0 {
+								client.VariablesRequest(ref)
+								tmV0 := client.ExpectVariablesResponse(t)
+								expectChildren(t, tmV0, "tm.v[0]", 64)
+							}
 						}
 					}
+
+					// Auto-loading works with call return variables as well
+					protest.MustSupportFunctionCalls(t, testBackend)
+					client.EvaluateRequest("call rettm()", 1000, "repl")
+					got := client.ExpectEvaluateResponse(t)
+					ref = expectEval(t, got, "main.truncatedMap {v: []map[string]main.astruct len: 1, cap: 1, [[...]]}", hasChildren)
+					if ref > 0 {
+						client.VariablesRequest(ref)
+						rv := client.ExpectVariablesResponse(t)
+						expectChildren(t, rv, "rv", 1)
+						ref = expectVarExact(t, rv, 0, "~r0", "", "main.truncatedMap {v: []map[string]main.astruct len: 1, cap: 1, [[...]]}", hasChildren)
+						if ref > 0 {
+							client.VariablesRequest(ref)
+							tm := client.ExpectVariablesResponse(t)
+							expectChildren(t, tm, "tm", 1)
+							ref = expectVarExact(t, tm, 0, "v", "", "[]map[string]main.astruct len: 1, cap: 1, [[...]]", hasChildren)
+							if ref > 0 {
+								// Auto-loading of fully missing map chidlren happens here, but they get trancated at MaxArrayValuess
+								client.VariablesRequest(ref)
+								tmV := client.ExpectVariablesResponse(t)
+								expectChildren(t, tmV, "tm.v", 1)
+								// TODO(polina): this evaluate name is not usable - it should be empty
+								ref = expectVarRegex(t, tmV, 0, `\[0\]`, `\[0\]`, `map\[string\]main\.astruct \[.+\.\.\.\+2 more\]`, hasChildren)
+								if ref > 0 {
+									client.VariablesRequest(ref)
+									tmV0 := client.ExpectVariablesResponse(t)
+									expectChildren(t, tmV0, "tm.v[0]", 64)
+								}
+							}
+						}
+					}
+
+					// TODO(polina): need fully missing array/slice test case
+
+					// Zero slices, structs and maps are not treated as fully missing
+					// See zsvar, zsslice,, emptyslice, emptymap, a0
 				},
 				disconnect: true,
 			}})
@@ -1541,9 +1592,26 @@ func TestVariablesLoading(t *testing.T) {
 							}
 						}
 
-						// Pointer value not loaded with LoadConfig.FollowPointers=false
+						// Pointer values loaded even with LoadConfig.FollowPointers=false
-						expectVarRegex(t, locals, -1, "a7", "a7", `\(not loaded\) \(\*main\.FooBar\)\(0x[0-9a-f]+\)`, noChildren)
+
+						expectVarExact(t, locals, -1, "a7", "a7", "*main.FooBar {Baz: 5, Bur: \"strum\"}", hasChildren)
+
+						// Auto-loading works on results of evaluate expressions as well
+						client.EvaluateRequest("a7", frame, "repl")
+						expectEval(t, client.ExpectEvaluateResponse(t), "*main.FooBar {Baz: 5, Bur: \"strum\"}", hasChildren)
+
+						client.EvaluateRequest("&a7", frame, "repl")
+						pa7 := client.ExpectEvaluateResponse(t)
+						ref = expectEvalRegex(t, pa7, `\*\(\*main\.FooBar\)\(0x[0-9a-f]+\)`, hasChildren)
+						if ref > 0 {
+							client.VariablesRequest(ref)
+							a7 := client.ExpectVariablesResponse(t)
+							expectVarExact(t, a7, 0, "a7", "(*(&a7))", "*main.FooBar {Baz: 5, Bur: \"strum\"}", hasChildren)
+						}
 					}
+
+					// Frame-independent loading expressions allow us to auto-load
+					// variables in any frame, not just topmost.
 					loadvars(1000 /*first topmost frame*/)
 					// step into another function
 					client.StepInRequest(1)
@@ -1988,7 +2056,14 @@ func TestEvaluateRequest(t *testing.T) {
 					// Type casts of integer constants into any pointer type and vice versa
 					client.EvaluateRequest("(*int)(2)", 1000, "this context will be ignored")
 					got = client.ExpectEvaluateResponse(t)
-					expectEval(t, got, "(not loaded) (*int)(0x2)", noChildren)
+					ref = expectEvalRegex(t, got, `\*\(unreadable .+\)`, hasChildren)
+					if ref > 0 {
+						client.VariablesRequest(ref)
+						val := client.ExpectVariablesResponse(t)
+						expectChildren(t, val, "*(*int)(2)", 1)
+						expectVarRegex(t, val, 0, "^$", `\(\*\(\(\*int\)\(2\)\)\)`, `\(unreadable .+\)`, noChildren)
+						validateEvaluateName(t, client, val, 0)
+					}
 
 					// Type casts between string, []byte and []rune
 					client.EvaluateRequest("[]byte(\"ABC€\")", 1000, "this context will be ignored")