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

Jump to bottom

service/dap: detect and report unloaded variables #2353

Merged
merged 2 commits into from
Feb 23, 2021
Merged

service/dap: detect and report unloaded variables #2353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6468e40
Detect and report unloaded variables
polinasok Feb 23, 2021
19b9a98
Make DeepSource happy
polinasok Feb 23, 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
5 changes: 4 additions & 1 deletion _fixtures/testvariables2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,9 @@ func main() {
fmt.Println(amb1)
}

longarr := [100]int{}
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, 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)
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, 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)
}
109 changes: 75 additions & 34 deletions service/dap/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,17 @@ var defaultArgs = launchAttachArgs{
showGlobalVariables: false,
}

// 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.
// TODO(polina): Support setting config via launch/attach args or only rely on on-demand loading?
var DefaultLoadConfig = proc.LoadConfig{
FollowPointers: true,
MaxVariableRecurse: 1,
MaxStringLen: 64,
MaxArrayValues: 64,
MaxStructFields: -1,
}

// 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 disconnects or requests
Expand Down Expand Up @@ -825,19 +836,17 @@ func (s *Server) onScopesRequest(request *dap.ScopesRequest) {

goid := sf.(stackFrame).goroutineID
frame := sf.(stackFrame).frameIndex
// TODO(polina): Support setting config via launch/attach args
cfg := proc.LoadConfig{FollowPointers: true, MaxVariableRecurse: 1, MaxStringLen: 64, MaxArrayValues: 64, MaxStructFields: -1}

// Retrieve arguments
args, err := s.debugger.FunctionArguments(goid, frame, 0, cfg)
args, err := s.debugger.FunctionArguments(goid, frame, 0, DefaultLoadConfig)
if err != nil {
s.sendErrorResponse(request.Request, UnableToListArgs, "Unable to list args", err.Error())
return
}
argScope := &fullyQualifiedVariable{&proc.Variable{Name: "Arguments", Children: slicePtrVarToSliceVar(args)}, "", true}

// Retrieve local variables
locals, err := s.debugger.LocalVariables(goid, frame, 0, cfg)
locals, err := s.debugger.LocalVariables(goid, frame, 0, DefaultLoadConfig)
if err != nil {
s.sendErrorResponse(request.Request, UnableToListLocals, "Unable to list locals", err.Error())
return
Expand Down Expand Up @@ -866,7 +875,7 @@ func (s *Server) onScopesRequest(request *dap.ScopesRequest) {
return
}
currPkgFilter := fmt.Sprintf("^%s\\.", currPkg)
globals, err := s.debugger.PackageVariables(currPkgFilter, cfg)
globals, err := s.debugger.PackageVariables(currPkgFilter, DefaultLoadConfig)
if err != nil {
s.sendErrorResponse(request.Request, UnableToListGlobals, "Unable to list globals", err.Error())
return
Expand Down Expand Up @@ -908,8 +917,6 @@ func (s *Server) onVariablesRequest(request *dap.VariablesRequest) {
return
}
children := make([]dap.Variable, 0)
// TODO(polina): check and handle if variable loaded incompletely
// https://github.com/go-delve/delve/blob/master/Documentation/api/ClientHowto.md#looking-into-variables

switch v.Kind {
case reflect.Map:
Expand Down Expand Up @@ -1045,6 +1052,11 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
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.

switch v.Kind {
case reflect.UnsafePointer:
if len(v.Children) == 0 {
Expand All @@ -1061,18 +1073,32 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
value = "void"
} else {
value = fmt.Sprintf("<%s>(%#x)", typeName, v.Children[0].Addr)
variablesReference = maybeCreateVariableHandle(v)
if v.Children[0].OnlyAddr { // Not fully loaded
value += " (value not loaded)"
// Since child is not fully loaded, don't provide a reference to view it.
} else {
value = fmt.Sprintf("<%s>(%#x)", typeName, v.Children[0].Addr)
variablesReference = maybeCreateVariableHandle(v)
}
}
case reflect.Array:
value = "<" + typeName + ">"
if v.Len > int64(len(v.Children)) { // Not fully loaded
value = fmt.Sprintf("<%s> (length: %d, loaded: %d)", typeName, v.Len, len(v.Children))
} else {
value = fmt.Sprintf("<%s>", typeName)
}
if len(v.Children) > 0 {
variablesReference = maybeCreateVariableHandle(v)
}
case reflect.Slice:
if v.Base == 0 {
value = "nil <" + typeName + ">"
} else {
value = fmt.Sprintf("<%s> (length: %d, cap: %d)", typeName, v.Len, v.Cap)
if v.Len > int64(len(v.Children)) { // Not fully loaded
value = fmt.Sprintf("<%s> (length: %d, cap: %d, loaded: %d)", typeName, v.Len, v.Cap, len(v.Children))
} else {
value = fmt.Sprintf("<%s> (length: %d, cap: %d)", typeName, v.Len, v.Cap)
}
if len(v.Children) > 0 {
variablesReference = maybeCreateVariableHandle(v)
}
Expand All @@ -1081,15 +1107,19 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
if v.Base == 0 {
value = "nil <" + typeName + ">"
} else {
value = fmt.Sprintf("<%s> (length: %d)", typeName, v.Len)
if v.Len > int64(len(v.Children)/2) { // Not fully loaded
value = fmt.Sprintf("<%s> (length: %d, loaded: %d)", typeName, v.Len, len(v.Children)/2)
} else {
value = fmt.Sprintf("<%s> (length: %d)", typeName, v.Len)
}
if len(v.Children) > 0 {
variablesReference = maybeCreateVariableHandle(v)
}
}
case reflect.String:
vvalue := constant.StringVal(v.Value)
lenNotLoaded := v.Len - int64(len(vvalue))
if lenNotLoaded > 0 {
if lenNotLoaded > 0 { // Not fully loaded
vvalue += fmt.Sprintf("...+%d more", lenNotLoaded)
}
value = fmt.Sprintf("%q", vvalue)
Expand All @@ -1111,21 +1141,35 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
} else if len(v.Children) == 0 || v.Children[0].Kind == reflect.Invalid && v.Children[0].Addr == 0 {
value = "nil <" + typeName + ">"
} else {
value = "<" + typeName + "(" + v.Children[0].TypeString() + ")" + ">"
// TODO(polina): should we remove one level of indirection and skip "data"?
// Then we will have:
// Before:
// i: <interface{}(int)>
// data: 123
// After:
// i: <interface{}(int)> 123
// Before:
// i: <interface{}(main.MyStruct)>
// data: <main.MyStruct>
// field1: ...
// After:
// i: <interface{}(main.MyStruct)>
// field1: ...
if v.Children[0].OnlyAddr { // Not fully loaded
value = "<" + typeName + "(" + v.Children[0].TypeString() + ")" + ">" + " (data not loaded)"
// Since child is not fully loaded, don't provide a reference to view it.
} else {
value = "<" + typeName + "(" + v.Children[0].TypeString() + ")" + ">"
// TODO(polina): should we remove one level of indirection and skip "data"?
// Then we will have:
// Before:
// i: <interface{}(int)>
// data: 123
// After:
// i: <interface{}(int)> 123
// Before:
// i: <interface{}(main.MyStruct)>
// data: <main.MyStruct>
// field1: ...
// After:
// i: <interface{}(main.MyStruct)>
// field1: ...
variablesReference = maybeCreateVariableHandle(v)
}
}
case reflect.Struct:
if v.Len > int64(len(v.Children)) { // Not fully loaded
value = fmt.Sprintf("<%s> (fields: %d, loaded: %d)", typeName, v.Len, len(v.Children))
} else {
value = fmt.Sprintf("<%s>", typeName)
}
if len(v.Children) > 0 {
variablesReference = maybeCreateVariableHandle(v)
}
case reflect.Complex64, reflect.Complex128:
Expand All @@ -1142,7 +1186,7 @@ func (s *Server) convertVariableWithOpts(v *proc.Variable, qualifiedNameOrExpr s
v.Children[1].Kind = reflect.Float64
}
fallthrough
default: // Struct, complex, scalar
default: // complex, scalar
vvalue := api.VariableValueAsString(v)
if vvalue != "" {
value = vvalue
Expand Down Expand Up @@ -1177,9 +1221,6 @@ func (s *Server) onEvaluateRequest(request *dap.EvaluateRequest) {
goid = sf.(stackFrame).goroutineID
frame = sf.(stackFrame).frameIndex
}
// TODO(polina): Support config settings via launch/attach args vs auto-loading?
apiCfg := &api.LoadConfig{FollowPointers: true, MaxVariableRecurse: 1, MaxStringLen: 64, MaxArrayValues: 64, MaxStructFields: -1}
prcCfg := proc.LoadConfig{FollowPointers: true, MaxVariableRecurse: 1, MaxStringLen: 64, MaxArrayValues: 64, MaxStructFields: -1}

response := &dap.EvaluateResponse{Response: *newResponse(request.Request)}
isCall, err := regexp.MatchString(`^\s*call\s+\S+`, request.Arguments.Expression)
Expand All @@ -1201,7 +1242,7 @@ func (s *Server) onEvaluateRequest(request *dap.EvaluateRequest) {
}
state, err := s.debugger.Command(&api.DebuggerCommand{
Name: api.Call,
ReturnInfoLoadConfig: apiCfg,
ReturnInfoLoadConfig: api.LoadConfigFromProc(&DefaultLoadConfig),
Expr: strings.Replace(request.Arguments.Expression, "call ", "", 1),
UnsafeCall: false,
GoroutineID: goid,
Expand All @@ -1224,7 +1265,7 @@ func (s *Server) onEvaluateRequest(request *dap.EvaluateRequest) {
if t.GoroutineID == stateBeforeCall.SelectedGoroutine.ID &&
t.Line == stateBeforeCall.SelectedGoroutine.CurrentLoc.Line && t.CallReturn {
// The call completed. Get the return values.
retVars, err = s.debugger.FindThreadReturnValues(t.ID, prcCfg)
retVars, err = s.debugger.FindThreadReturnValues(t.ID, DefaultLoadConfig)
if err != nil {
s.sendErrorResponseWithOpts(request.Request, UnableToEvaluateExpression, "Unable to evaluate expression", err.Error(), showErrorToUser)
return
Expand Down Expand Up @@ -1264,7 +1305,7 @@ func (s *Server) onEvaluateRequest(request *dap.EvaluateRequest) {
}
}
} else { // {expression}
exprVar, err := s.debugger.EvalVariableInScope(goid, frame, 0, request.Arguments.Expression, prcCfg)
exprVar, err := s.debugger.EvalVariableInScope(goid, frame, 0, request.Arguments.Expression, DefaultLoadConfig)
if err != nil {
s.sendErrorResponseWithOpts(request.Request, UnableToEvaluateExpression, "Unable to evaluate expression", err.Error(), showErrorToUser)
return
Expand Down
Loading