Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
May 4, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1453fb8
service/dap: auto-loading for fully missing pointers, structs, maps, …
polinasok Apr 27, 2021
c747116
Add call test
polinasok Apr 27, 2021
5a7e76d
Add TODO
polinasok Apr 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion _fixtures/testvariables2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
93 changes: 70 additions & 23 deletions service/dap/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,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,
Expand Down Expand Up @@ -1331,33 +1332,86 @@ 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. For now, clearly
// communicate when values are not fully loaded. TODO(polina): look into loading on demand.
// Some of the types might be fully or partially not loaded based on LoadConfig.
// Those that are fully missing (e.g. due to hitting MaxVariableRecurse), can be reloaded in place.
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what a client would do, but this code lives in the same process as the debugger and has a (presumed valid) *proc.Variable object, so it can do better. It can set v.loaded to false and call v.loadValue again. None of this is exported at the moment, and it needs to be done while holding a the target mutex.

It's more efficient and it's guaranteed to keep working with generics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this code from the previous iteration that you blessed. I am happy to improve, but could we do that in a separate PR please? I added a TODO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

// 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.
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 {
value = "(not loaded) " + value
} else {
if v.Children[0].OnlyAddr { // Not loaded
if v.Addr == 0 {
// 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)
Expand All @@ -1366,27 +1420,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
// We might be loading variables from the frame that's not topmost, so use
// 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()
}
if v.Children[0].OnlyAddr { // Not loaded
value = reloadVariable(v, qualifiedNameOrExpr)
}
// 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)
Expand Down
101 changes: 88 additions & 13 deletions service/dap/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -1374,7 +1378,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)
Expand All @@ -1387,16 +1391,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
expectVarRegex(t, c1sa0, 0, "", `\(\*c1\.sa\[0\]\)`, `\(loaded 0/2\) \(\*main\.astruct\)\(0x[0-9a-f]+\)`, noChildren)
expectVarExact(t, c1sa0, 0, "", "(*c1.sa[0])", "main.astruct {A: 1, B: 2}", hasChildren)
}
}
}

// 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)
Expand All @@ -1409,30 +1413,77 @@ 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"
expectVarRegex(t, aas0aas, 0, "[0]", `aas\[0\]\.aas\[0\]`, `\(loaded 0/1\) \(\*main\.a\)\(0x[0-9a-f]+\)`, noChildren)
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)
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)
tm := client.ExpectVariablesResponse(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"]"
expectVarExact(t, tmV, 0, "[0]", "tm.v[0]", "(loaded 0/66) map[string]main.astruct [...]", noChildren)
ref = expectVarRegex(t, tmV, 0, `\[0\]`, `tm\.v\[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)
}
}
}

// 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,
}})
Expand Down Expand Up @@ -1491,9 +1542,26 @@ func TestVariablesLoading(t *testing.T) {
}
}

// Pointer value not loaded with LoadConfig.FollowPointers=false
expectVarRegex(t, locals, -1, "a7", "a7", `\(not loaded\) \(\*main\.FooBar\)\(0x[0-9a-f]+\)`, noChildren)
// Pointer values loaded even with LoadConfig.FollowPointers=false

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)
Expand Down Expand Up @@ -1938,7 +2006,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")
Expand Down