Repository: go-delve/delve
Pull request #2418 (2021-04-07 02:02:26)

dap: use larger string type variable load limits in 'repl', 'variables' context
When evaluate requests are triggered in the context of 'repl' (DEBUG CONSOLE in VSCode) or 'variables' (Copy Value from VARIABLES section in VSCode), they are triggered by one-off human action and loading more data isn't too bad.

Variable auto-loading for strings or arrays is nice but currently it's unclear to me how this should be integrated in the DEBUG CONSOLE or with the Copy Value feature. Until we have better ideas and tools, let's go with these larger limits.

Unfortunately, the "Copy Value" from WATCH section triggers evaluate requests with "watch" context and we don't want to load large data automatically for "watch" yet. We can consider clipboard capability instead later. (Followup change)

Also applied a more generous max string limit when evaluating function calls - since the results can be volatile, it's not guaranteed that we'd be able to reload after the injected call is complete, and the current string limit (64) is too small in many cases.

Updates golang/vscode-go#1318
Updates golang/vscode-go#126

hyangah: cc @polinasok @suzmue 
polinasok: I found another issue for this: https://github.com/golang/vscode-go/issues/126
hyangah: Moved the reloading part back to onEvaluateRequest to make it clear it doesn't use variables from `FunctionArguments`. 
Also, unconditionally set the string limit to a larger value when handling function call. Should we adjust other configuration to avoid accidentally loading too much data? 

Have no idea how this change makes other tests fail...  though.
suzmue: >TestAttachStopOnEntry has an unrelated bug that makes it fail occasionally (there's a PR to fix it), TestFrameEvaluation is flaky on arm64 due to cgo involvement.
I don't know about TestPreSetBreakpoint, someone familiar with it should take a look.


The TestPreSetBreakpoint test is flaky and needs to be fixed, but is unrelated to this change. The threads request arrived after the debugger had stopped so the test did not get the dummy response it was expecting.
polinasok: > > TestAttachStopOnEntry has an unrelated bug that makes it fail occasionally (there's a PR to fix it), TestFrameEvaluation is flaky on arm64 due to cgo involvement.
> > I don't know about TestPreSetBreakpoint, someone familiar with it should take a look.
> 
> The TestPreSetBreakpoint test is flaky and needs to be fixed, but is unrelated to this change. The threads request arrived after the debugger had stopped so the test did not get the dummy response it was expecting.

Yup, I saw that too. Are you looking into it or should I?
suzmue: > > > TestAttachStopOnEntry has an unrelated bug that makes it fail occasionally (there's a PR to fix it), TestFrameEvaluation is flaky on arm64 due to cgo involvement.
> > I don't know about TestPreSetBreakpoint, someone familiar with it should take a look.
> > 
> > The TestPreSetBreakpoint test is flaky and needs to be fixed, but is unrelated to this change. The threads request arrived after the debugger had stopped so the test did not get the dummy response it was expecting.
> > 
> 

> Yup, I saw that too. Are you looking into it or should I?
> 

Sent a PR https://github.com/go-delve/delve/pull/2483
polinasok: Oh and I think the PR description and title need to be updated to focus on strings only.
hyangah: 
> I am not familiar with clipboard mode. I am not sure how it is different from Copy Value and such.

Once we enable the clipboard mode, `Copy Value` will trigger clipboard context evaluate requests, Not 'watch' or 'variables' context any more.

> > Assuming users want injected function call to stringify some values, should we just go with a generous return value loading limit and present as much data as we could up front? What do you think?
> 
> I think this PR already applies relaxed limits to all call instances, including WATCH. I don't know how to determine what the magic value is for "as much data as we could". I did like your earlier suggestion to start small and expand based on user feedback. We could wait for this feedback to come on its own, or try to solicit it actively. It is always easier to relax things than to tighten them.

1K doesn't seem a lot. From google search, I see users choose values [300, ridiculously-large-value] ranges.

hyangah: Looks like an unrelated test failure
![Screen Shot 2021-05-18 at 11 09 14 AM](https://user-images.githubusercontent.com/4999471/118676540-7db11980-b7c9-11eb-8c47-7046d35e862f.png)

derekparker: @polinasok I'll wait for your approval before final review / merge.

Patch:
diff --git a/_fixtures/fncall.go b/_fixtures/fncall.go
index ba001be4..949e4010 100644
--- a/_fixtures/fncall.go
+++ b/_fixtures/fncall.go
@@ -178,7 +178,7 @@ func main() {
 	a2 := a2struct{Y: 7}
 	var pa2 *astruct
 	var str string = "old string value"
-
+	longstrs := []string{"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"}
 	var vable_a VRcvrable = a
 	var vable_pa VRcvrable = pa
 	var pable_pa PRcvrable = pa
@@ -200,5 +200,5 @@ func main() {
 	d.Method()
 	d.Base.Method()
 	x.CallMe()
-	fmt.Println(one, two, zero, call, call0, call2, callexit, callpanic, callbreak, callstacktrace, stringsJoin, intslice, stringslice, comma, a.VRcvr, a.PRcvr, pa, vable_a, vable_pa, pable_pa, fn2clos, fn2glob, fn2valmeth, fn2ptrmeth, fn2nil, ga, escapeArg, a2, square, intcallpanic, onetwothree, curriedAdd, getAStruct, getAStructPtr, getVRcvrableFromAStruct, getPRcvrableFromAStructPtr, getVRcvrableFromAStructPtr, pa2, noreturncall, str, d, x, x2.CallMe(5))
+	fmt.Println(one, two, zero, call, call0, call2, callexit, callpanic, callbreak, callstacktrace, stringsJoin, intslice, stringslice, comma, a.VRcvr, a.PRcvr, pa, vable_a, vable_pa, pable_pa, fn2clos, fn2glob, fn2valmeth, fn2ptrmeth, fn2nil, ga, escapeArg, a2, square, intcallpanic, onetwothree, curriedAdd, getAStruct, getAStructPtr, getVRcvrableFromAStruct, getPRcvrableFromAStructPtr, getVRcvrableFromAStructPtr, pa2, noreturncall, str, d, x, x2.CallMe(5), longstrs)
 }
diff --git a/service/dap/server.go b/service/dap/server.go
index 13b4bf8e..d61ae55e 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -1632,7 +1632,7 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
 	}
 	value = api.ConvertVar(v).SinglelineString()
 	if v.Unreadable != nil {
-		return
+		return value, variablesReference
 	}
 
 	// Some of the types might be fully or partially not loaded based on LoadConfig.
@@ -1721,7 +1721,7 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
 			variablesReference = maybeCreateVariableHandle(v)
 		}
 	case reflect.String:
-		// TODO(polina): implement auto-loading here
+		// 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 loaded
@@ -1761,7 +1761,7 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
 			variablesReference = maybeCreateVariableHandle(v)
 		}
 	}
-	return
+	return value, variablesReference
 }
 
 // onEvaluateRequest handles 'evalute' requests.
@@ -1817,8 +1817,21 @@ func (s *Server) onEvaluateRequest(request *dap.EvaluateRequest) {
 			s.sendErrorResponseWithOpts(request.Request, UnableToEvaluateExpression, "Unable to evaluate expression", err.Error(), showErrorToUser)
 			return
 		}
+
+		if exprVar.Kind == reflect.String && (request.Arguments.Context == "repl" || request.Arguments.Context == "variables") {
+			if strVal := constant.StringVal(exprVar.Value); exprVar.Len > int64(len(strVal)) {
+				// reload string value with a bigger limit.
+				loadCfg := DefaultLoadConfig
+				loadCfg.MaxStringLen = 4 << 10
+				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 {
+					exprVar = v
+				}
+			}
+		}
 		// TODO(polina): as far as I can tell, evaluateName is ignored by vscode for expression variables.
-		// Should it be skipped alltogether for all levels?
+		// Should it be skipped altogether for all levels?
 		exprVal, exprRef := s.convertVariable(exprVar, fmt.Sprintf("(%s)", request.Arguments.Expression))
 		response.Body = dap.EvaluateResponseBody{Result: exprVal, VariablesReference: exprRef}
 	}
@@ -1839,12 +1852,22 @@ func (s *Server) doCall(goid, frame int, expr string) (*api.DebuggerState, []*pr
 	if err != nil {
 		return nil, nil, err
 	}
+	// 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
+
 	// TODO(polina): since call will resume execution of all goroutines,
 	// we should do this asynchronously and send a continued event to the
 	// editor, followed by a stop event when the call completes.
 	state, err := s.debugger.Command(&api.DebuggerCommand{
 		Name:                 api.Call,
-		ReturnInfoLoadConfig: api.LoadConfigFromProc(&DefaultLoadConfig),
+		ReturnInfoLoadConfig: api.LoadConfigFromProc(&loadCfg),
 		Expr:                 expr,
 		UnsafeCall:           false,
 		GoroutineID:          goid,
@@ -1869,7 +1892,7 @@ func (s *Server) doCall(goid, frame int, expr string) (*api.DebuggerState, []*pr
 			t.Line == stateBeforeCall.SelectedGoroutine.CurrentLoc.Line && t.CallReturn {
 			found = true
 			// The call completed. Get the return values.
-			retVars, err = s.debugger.FindThreadReturnValues(t.ID, DefaultLoadConfig)
+			retVars, err = s.debugger.FindThreadReturnValues(t.ID, loadCfg)
 			if err != nil {
 				return nil, nil, err
 			}


Test patch:
diff --git a/_fixtures/testvariables2.go b/_fixtures/testvariables2.go
index d350262b..d59cbebc 100644
--- a/_fixtures/testvariables2.go
+++ b/_fixtures/testvariables2.go
@@ -328,7 +328,7 @@ func main() {
 
 	longstr := "very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"
 	m5 := map[C]int{{longstr}: 1}
-
+	m6 := C{s: longstr}
 	var nilstruct *astruct = nil
 
 	val := A{val: 1} // val vs val.val
@@ -363,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, rettm, errtypednil, emptyslice, emptymap, byteslice, runeslice, bytearray, runearray, longstr, nilstruct, as2, as2.NonPointerRecieverMethod, s4, iface2map, issue1578, ll, unread, w2, w3, w4, w5, longarr, longslice, val)
+	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, val, m6)
 }
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index 233b7769..90fa38b6 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -1368,7 +1368,7 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 					expectVarExact(t, locals, -1, "emptyslice", "emptyslice", "[]string len: 0, cap: 0, []", "[]string", noChildren)
 					expectVarExact(t, locals, -1, "nilslice", "nilslice", "[]int len: 0, cap: 0, nil", "[]int", noChildren)
 					// reflect.Kind == String
-					expectVarExact(t, locals, -1, "longstr", "longstr", "\"very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more\"", "string", noChildren)
+					expectVarExact(t, locals, -1, "longstr", "longstr", `"very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more"`, "string", noChildren)
 					// reflect.Kind == Struct
 					expectVarExact(t, locals, -1, "zsvar", "zsvar", "struct {} {}", "struct {}", noChildren)
 					// reflect.Kind == UnsafePointer
@@ -1448,7 +1448,7 @@ func TestScopesRequestsOptimized(t *testing.T) {
 		protest.EnableOptimization)
 }
 
-// TestVariablesLoading exposes test cases where variables might be partiall or
+// TestVariablesLoading exposes test cases where variables might be partially or
 // fully unloaded.
 func TestVariablesLoading(t *testing.T) {
 	runTest(t, "testvariables2", func(client *daptest.Client, fixture protest.Fixture) {
@@ -2666,6 +2666,55 @@ func TestEvaluateRequest(t *testing.T) {
 	})
 }
 
+func TestEvaluateRequestLongStr(t *testing.T) {
+	runTest(t, "testvariables2", 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
+			fixture.Source, []int{},
+			[]onBreakpoint{{
+				execute: func() {
+
+					longstr := `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"`
+					longstrTruncated := `"very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more"`
+
+					handleStop(t, client, 1, "main.main", -1)
+
+					client.VariablesRequest(1001) // Locals
+					locals := client.ExpectVariablesResponse(t)
+
+					// reflect.Kind == String, load with longer load limit if evaluated in repl/variables context.
+					for _, evalContext := range []string{"", "watch", "repl", "variables", "somethingelse"} {
+						t.Run(evalContext, func(t *testing.T) {
+							// long string
+							client.EvaluateRequest("longstr", 0, evalContext)
+							got := client.ExpectEvaluateResponse(t)
+							want := longstrTruncated
+							if evalContext == "repl" || evalContext == "variables" {
+								want = longstr
+							}
+							expectEval(t, got, want, false)
+
+							// long string as a struct field
+							client.EvaluateRequest("(m6).s", 0, evalContext)
+							got2 := client.ExpectEvaluateResponse(t)
+							expectEval(t, got2, want, false)
+
+							// variables are not affected.
+							expectVarExact(t, locals, -1, "longstr", "longstr", longstrTruncated, "string", noChildren)
+							expectVarExact(t, locals, -1, "m6", "m6", `main.C {s: `+longstrTruncated+`}`, "main.C", hasChildren)
+						})
+					}
+
+				},
+				disconnect: true,
+			}})
+	})
+}
+
 func TestEvaluateCallRequest(t *testing.T) {
 	protest.MustSupportFunctionCalls(t, testBackend)
 	runTest(t, "fncall", func(client *daptest.Client, fixture protest.Fixture) {
@@ -2796,6 +2845,14 @@ func TestEvaluateCallRequest(t *testing.T) {
 					client.EvaluateRequest("call ", 1000, "watch")
 					got = client.ExpectEvaluateResponse(t)
 					expectEval(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)
+					expectEval(t, got, `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"`, hasChildren)
+					client.EvaluateRequest(`call stringsJoin(longstrs, ",")`, 1000, "watch") // full string
+					got = client.ExpectEvaluateResponse(t)
+					expectEval(t, got, `"very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"`, hasChildren)
+
 					// Call error
 					client.EvaluateRequest("call call1(one)", 1000, "watch")
 					erres := client.ExpectInvisibleErrorResponse(t)