diff --git a/_fixtures/longstrings.go b/_fixtures/longstrings.go new file mode 100644 index 00000000..8ad14dbe --- /dev/null +++ b/_fixtures/longstrings.go @@ -0,0 +1,22 @@ +package main + +import ( + "runtime" +) + +func buildString(length int) string { + s := "" + for i := 0; i < length; i++ { + s = s + "x" + } + return s +} + +func main() { + s513 := buildString(513) + s1025 := buildString(1025) + s4097 := buildString(4097) + nested := map[int]string{513: s513, 1025: s1025, 4097: s4097} + runtime.Breakpoint() + _ = nested +} diff --git a/_fixtures/testvariables2.go b/_fixtures/testvariables2.go index a8a4ced7..57174831 100644 --- a/_fixtures/testvariables2.go +++ b/_fixtures/testvariables2.go @@ -329,7 +329,7 @@ func main() { longstr := "very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789" m5 := map[C]int{{longstr}: 1} m6 := map[string]int{longstr: 123} - m7 := map[C]C{{longstr}: {longstr}} + m7 := map[C]C{{longstr}: {"hello"}} cl := C{s: longstr} var nilstruct *astruct = nil diff --git a/service/dap/server.go b/service/dap/server.go index 9403913d..d5e1aeff 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -165,18 +165,31 @@ type dapClientCapabilites struct { supportsProgressReporting bool } -// 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? +// DefaultLoadConfig controls how variables are loaded from the target's memory. +// These limits are conservative to minimize performace overhead for bulk loading. +// With dlv-dap, users do not have a way to adjust these. +// Instead we are focusing in interacive loading with nested reloads, array/map +// paging and context-specific string limits. var DefaultLoadConfig = proc.LoadConfig{ FollowPointers: true, MaxVariableRecurse: 1, - MaxStringLen: 64, - MaxArrayValues: 64, - MaxStructFields: -1, + // TODO(polina): consider 1024 limit instead: + // - vscode+C appears to use 1024 as the load limit + // - vscode viewlet hover truncates at 1023 characters + MaxStringLen: 512, + MaxArrayValues: 64, + MaxStructFields: -1, } +const ( + // When a user examines a single string, we can relax the loading limit. + maxSingleStringLen = 4 << 10 // 4096 + // Results of a call are single-use and transient. We need to maximize + // what is presented. A common use case of a call injection is to + // stringify complex data conveniently. + maxStringLenInCallRetVars = 1 << 10 // 1024 +) + // NewServer creates a new DAP Server. It takes an opened Listener // via config and assumes its ownership. config.DisconnectChan has to be set; // it will be closed by the server when the client fails to connect, @@ -1794,9 +1807,9 @@ func (s *Server) childrenToDAPVariables(v *fullyQualifiedVariable) ([]dap.Variab Value: val, } if keyref != 0 { // key is a type to be expanded - if len(key) > DefaultLoadConfig.MaxStringLen { + if len(key) > maxMapKeyValueLen { // Truncate and make unique - kvvar.Name = fmt.Sprintf("%s... @ %#x", key[0:DefaultLoadConfig.MaxStringLen], keyv.Addr) + kvvar.Name = fmt.Sprintf("%s... @ %#x", key[0:maxMapKeyValueLen], keyv.Addr) } kvvar.VariablesReference = keyref kvvar.IndexedVariables = getIndexedVariableCount(keyv) @@ -1890,9 +1903,12 @@ func (s *Server) convertVariableToString(v *proc.Variable) string { return val } -// defaultMaxValueLen is the max length of a string representation of a compound or reference -// type variable. -const defaultMaxValueLen = 1 << 8 // 256 +const ( + // Limit the length of a string representation of a compound or reference type variable. + maxVarValueLen = 1 << 8 // 256 + // Limit the length of an inlined map key. + maxMapKeyValueLen = 64 +) // Flags for convertVariableWithOpts option. type convertVariableFlags uint8 @@ -2048,8 +2064,8 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s // By default, only values of variables that have children can be truncated. // If showFullValue is set, then all value strings are not truncated. canTruncateValue := showFullValue&opts == 0 - if len(value) > defaultMaxValueLen && canTruncateValue && canHaveRef { - value = value[:defaultMaxValueLen] + "..." + if len(value) > maxVarValueLen && canTruncateValue && canHaveRef { + value = value[:maxVarValueLen] + "..." } return value, variablesReference } @@ -2113,9 +2129,9 @@ func (s *Server) onEvaluateRequest(request *dap.EvaluateRequest) { case "repl", "variables", "hover", "clipboard": if exprVar.Kind == reflect.String { if strVal := constant.StringVal(exprVar.Value); exprVar.Len > int64(len(strVal)) { - // reload string value with a bigger limit. + // Reload the string value with a bigger limit. loadCfg := DefaultLoadConfig - loadCfg.MaxStringLen = 4 << 10 + loadCfg.MaxStringLen = maxSingleStringLen if v, err := s.debugger.EvalVariableInScope(goid, frame, 0, request.Arguments.Expression, loadCfg); err != nil { s.log.Debugf("Failed to load more for %v: %v", request.Arguments.Expression, err) } else { @@ -2152,13 +2168,11 @@ func (s *Server) doCall(goid, frame int, expr string) (*api.DebuggerState, []*pr } // The return values of injected function calls are volatile. // Load as much useful data as possible. - // - String: a common use case of call injection is to stringify complex - // data conveniently. That can easily exceed the default limit, 64. // TODO: investigate whether we need to increase other limits. For example, // the return value is a pointer to a temporary object, which can become // invalid by other injected function calls. Do we care about such use cases? loadCfg := DefaultLoadConfig - loadCfg.MaxStringLen = 1 << 10 + loadCfg.MaxStringLen = maxStringLenInCallRetVars // TODO(polina): since call will resume execution of all goroutines, // we should do this asynchronously and send a continued event to the diff --git a/service/dap/server_test.go b/service/dap/server_test.go index e010be34..b1670435 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -682,7 +682,7 @@ func checkVar(t *testing.T, got *dap.VariablesResponse, i int, name, evalName, v } } if i < 0 { - t.Errorf("\ngot %#v\nwant Variables[i].Name=%q", got, name) + t.Errorf("\ngot %#v\nwant Variables[i].Name=%q (not found)", got, name) return 0 } @@ -1379,7 +1379,7 @@ func TestScopesAndVariablesRequests2(t *testing.T) { checkVarExact(t, locals, -1, "emptyslice", "emptyslice", "[]string len: 0, cap: 0, []", "[]string", noChildren) checkVarExact(t, locals, -1, "nilslice", "nilslice", "[]int len: 0, cap: 0, nil", "[]int", noChildren) // reflect.Kind == String - checkVarExact(t, locals, -1, "longstr", "longstr", `"very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more"`, "string", noChildren) + checkVarExact(t, locals, -1, "longstr", "longstr", longstr, "string", noChildren) // reflect.Kind == Struct checkVarExact(t, locals, -1, "zsvar", "zsvar", "struct {} {}", "struct {}", noChildren) // reflect.Kind == UnsafePointer @@ -1476,8 +1476,12 @@ func TestVariablesLoading(t *testing.T) { }, { execute: func() { // Change default config values to trigger certain unloaded corner cases + saveDefaultConfig := DefaultLoadConfig DefaultLoadConfig.MaxStructFields = 5 - defer func() { DefaultLoadConfig.MaxStructFields = -1 }() + DefaultLoadConfig.MaxStringLen = 64 + defer func() { + DefaultLoadConfig = saveDefaultConfig + }() client.StackTraceRequest(1, 0, 0) client.ExpectStackTraceResponse(t) @@ -1489,7 +1493,8 @@ func TestVariablesLoading(t *testing.T) { locals := client.ExpectVariablesResponse(t) // String partially missing based on LoadConfig.MaxStringLen - checkVarExact(t, locals, -1, "longstr", "longstr", "\"very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more\"", "string", noChildren) + // See also TestVariableLoadingOfLongStrings + checkVarExact(t, locals, -1, "longstr", "longstr", longstrLoaded64, "string", noChildren) checkArrayChildren := func(t *testing.T, longarr *dap.VariablesResponse, parentName string, start int) { t.Helper() @@ -2827,7 +2832,7 @@ func TestEvaluateRequest(t *testing.T) { // From testvariables2 fixture const ( // As defined in the code - longstrFull = `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"` + longstr = `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"` // Loaded with MaxStringLen=64 longstrLoaded64 = `"very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more"` longstrLoaded64re = `\"very long string 0123456789a0123456789b0123456789c0123456789d012\.\.\.\+73 more\"` @@ -2880,28 +2885,27 @@ func TestVariableValueTruncation(t *testing.T) { // Compound map keys may be truncated even further // As the keys are always inside of a map container, - // this applies to variable requests. + // this applies to variables requests only, not evalute requests. // key - compound, value - scalar (inlined key:value display) => truncate key if too long - ref := checkVarExact(t, locals, -1, "m5", "m5", "map[main.C]int [{s: "+longstrLoaded64+"}: 1, ]", "map[main.C]int", hasChildren) + ref := checkVarExact(t, locals, -1, "m5", "m5", "map[main.C]int [{s: "+longstr+"}: 1, ]", "map[main.C]int", hasChildren) if ref > 0 { client.VariablesRequest(ref) // Key format: ... @
checkVarRegex(t, client.ExpectVariablesResponse(t), 0, `main\.C {s: "very long string 0123456789.+\.\.\. @ 0x[0-9a-f]+`, `m5\[\(\*\(\*"main\.C"\)\(0x[0-9a-f]+\)\)\]`, "1", `int`, hasChildren) } - // key - scalar, value - scalar (inlined key:value display) => not truncated - ref = checkVarExact(t, locals, -1, "m6", "m6", "map[string]int ["+longstrLoaded64+": 123, ]", "map[string]int", hasChildren) + // key - scalar, value - scalar (inlined key:value display) => key not truncated + ref = checkVarExact(t, locals, -1, "m6", "m6", "map[string]int ["+longstr+": 123, ]", "map[string]int", hasChildren) if ref > 0 { client.VariablesRequest(ref) - checkVarRegex(t, client.ExpectVariablesResponse(t), 0, longstrLoaded64re, `m6[\(\*\(\*\"string\"\)\(0x[0-9a-f]+\)\)]`, "123", "string: int", noChildren) + checkVarExact(t, client.ExpectVariablesResponse(t), 0, longstr, `m6[`+longstr+`]`, "123", "string: int", noChildren) } - // key - compound, value - compound (array-like display) => not truncated - ref = checkVarExact(t, locals, -1, "m7", "m7", "map[main.C]main.C [{s: "+longstrLoaded64+"}: {s: "+longstrLoaded64+"}, ]", "map[main.C]main.C", hasChildren) + // key - compound, value - compound (array-like display) => key not truncated + ref = checkVarExact(t, locals, -1, "m7", "m7", "map[main.C]main.C [{s: "+longstr+"}: {s: \"hello\"}, ]", "map[main.C]main.C", hasChildren) if ref > 0 { client.VariablesRequest(ref) m7 := client.ExpectVariablesResponse(t) - checkVarRegex(t, m7, 0, "[key 0]", `\(\*\(\*\"main\.C\"\)\(0x[0-9a-f]+\)\)`, `main\.C {s: `+longstrLoaded64re+`}`, `main\.C`, hasChildren) - checkVarRegex(t, m7, 1, "[val 0]", `\(\*\(\*\"main\.C\"\)\(0x[0-9a-f]+\)\)`, `main\.C {s: `+longstrLoaded64re+`}`, `main\.C`, hasChildren) + checkVarRegex(t, m7, 0, "[key 0]", `\(\*\(\*\"main\.C\"\)\(0x[0-9a-f]+\)\)`, `main\.C {s: `+longstr+`}`, `main\.C`, hasChildren) } }, disconnect: true, @@ -2909,16 +2913,17 @@ func TestVariableValueTruncation(t *testing.T) { }) } -// TestVariableLoadingOfLargeStrings tests that different string loading limits +// TestVariableLoadingOfLongStrings tests that different string loading limits // apply that depending on the context. -func TestVariableLoadingOfLargeStrings(t *testing.T) { - runTest(t, "testvariables2", func(client *daptest.Client, fixture protest.Fixture) { +func TestVariableLoadingOfLongStrings(t *testing.T) { + protest.MustSupportFunctionCalls(t, testBackend) + runTest(t, "longstrings", func(client *daptest.Client, fixture protest.Fixture) { runDebugSessionWithBPs(t, client, "launch", // Launch func() { client.LaunchRequest("exec", fixture.Path, !stopOnEntry) }, - // Breakpoints are set within the program + // Breakpoint set within the program fixture.Source, []int{}, []onBreakpoint{{ execute: func() { @@ -2927,29 +2932,56 @@ func TestVariableLoadingOfLargeStrings(t *testing.T) { client.VariablesRequest(1001) // Locals locals := client.ExpectVariablesResponse(t) - // Limits vary for evaluate requests - for _, evalContext := range []string{"", "watch", "repl", "variables", "hover", "clipboard", "somethingelse"} { - t.Run(evalContext, func(t *testing.T) { - // long string by itself (limits vary) - client.EvaluateRequest("longstr", 0, evalContext) - got := client.ExpectEvaluateResponse(t) - want := longstrLoaded64 - switch evalContext { - case "repl", "variables", "hover", "clipboard": - want = longstrFull - } - checkEval(t, got, want, false) + // Limits vary for evaluate requests with different contexts + tests := []struct { + context string + limit int + }{ + {"", DefaultLoadConfig.MaxStringLen}, + {"watch", DefaultLoadConfig.MaxStringLen}, + {"repl", maxSingleStringLen}, + {"hover", maxSingleStringLen}, + {"variables", maxSingleStringLen}, + {"clipboard", maxSingleStringLen}, + {"somethingelse", DefaultLoadConfig.MaxStringLen}, + } + for _, tc := range tests { + t.Run(tc.context, func(t *testing.T) { + // Long string by itself (limits vary) + client.EvaluateRequest("s4097", 0, tc.context) + want := fmt.Sprintf(`"x+\.\.\.\+%d more"`, 4097-tc.limit) + checkEvalRegex(t, client.ExpectEvaluateResponse(t), want, noChildren) - // long string as a child (same limits) - client.EvaluateRequest("(cl).s", 0, evalContext) - got2 := client.ExpectEvaluateResponse(t) - checkEval(t, got2, want, false) + // Evaluated container variables return values with minimally loaded + // strings, which are further truncated for displaying, so we + // can't test for loading limit except in contexts where an untruncated + // value is returned. + client.EvaluateRequest("&s4097", 0, tc.context) + switch tc.context { + case "variables", "clipboard": + want = fmt.Sprintf(`\*"x+\.\.\.\+%d more`, 4097-DefaultLoadConfig.MaxStringLen) + default: + want = fmt.Sprintf(`\*"x{%d}\.\.\.`, maxVarValueLen-2) + } + checkEvalRegex(t, client.ExpectEvaluateResponse(t), want, hasChildren) }) } - // Variables requests are not affected. - checkVarExact(t, locals, -1, "longstr", "longstr", longstrLoaded64, "string", noChildren) - checkVarExact(t, locals, -1, "cl", "cl", `main.C {s: `+longstrLoaded64+`}`, "main.C", hasChildren) + // Long strings returned from calls are subject to a different limit, + // same limit regardless of context + for _, context := range []string{"", "watch", "repl", "variables", "hover", "clipboard", "somethingelse"} { + t.Run(context, func(t *testing.T) { + client.EvaluateRequest(`call buildString(4097)`, 1000, context) + want := fmt.Sprintf(`"x+\.\.\.\+%d more"`, 4097-maxStringLenInCallRetVars) + got := client.ExpectEvaluateResponse(t) + checkEvalRegex(t, got, want, hasChildren) + }) + } + + // Variables requests use the most conservative loading limit + checkVarRegex(t, locals, -1, "s513", "s513", `"x{512}\.\.\.\+1 more"`, "string", noChildren) + // Container variables are subject to additional stricter value truncation that drops +more part + checkVarRegex(t, locals, -1, "nested", "nested", `map\[int\]string \[513: \"x+\.\.\.`, "string", hasChildren) }, disconnect: true, }}) @@ -3086,13 +3118,6 @@ func TestEvaluateCallRequest(t *testing.T) { client.EvaluateRequest("call ", 1000, "watch") got = client.ExpectEvaluateResponse(t) checkEval(t, got, "\"this is a variable named `call`\"", noChildren) - // Long string as a return value - client.EvaluateRequest(`call stringsJoin(longstrs, ",")`, 1000, "variables") // full string - got = client.ExpectEvaluateResponse(t) - checkEval(t, got, `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"`, hasChildren) - client.EvaluateRequest(`call stringsJoin(longstrs, ",")`, 1000, "watch") // full string - got = client.ExpectEvaluateResponse(t) - checkEval(t, got, `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"`, hasChildren) // Call error client.EvaluateRequest("call call1(one)", 1000, "watch")