Repository: go-delve/delve
Pull request #2172 (2020-09-12 09:33:53)

proc/gdbserial: fix two protocol bugs
During the testing of the core dump generation feature two bugs were
discovered in gdbserial:

1. we don't check that both bytes of the checksum are read, if the
buffer only has one byte we can end up reading only one byte instead
of two and the second byte will mess up the parsing of the next
packet
2. binary encoded packets can start with an 'E' and not be errors, when
using binary responses add an extra check for the lenght of the
response before deciding that the response is an error.

Unfortunately this encoding is inherently ambiguous (we can't
distinguish a 3 byte response starting with 'E' from an error) so
binary requests that lead to short responses should be avoided.

Testing this is complicated, they will be tested implicitly by the
upcoming core dump test.



Patch:
diff --git a/pkg/proc/gdbserial/gdbserver_conn.go b/pkg/proc/gdbserial/gdbserver_conn.go
index 23e70faa..fbd3aea5 100644
--- a/pkg/proc/gdbserial/gdbserver_conn.go
+++ b/pkg/proc/gdbserial/gdbserver_conn.go
@@ -1084,14 +1084,14 @@ func (conn *gdbConn) recv(cmd []byte, context string, binary bool) (resp []byte,
 		}
 
 		// read checksum
-		_, err = conn.rdr.Read(conn.inbuf[:2])
+		_, err = io.ReadFull(conn.rdr, conn.inbuf[:2])
 		if err != nil {
 			return nil, err
 		}
 		if logflags.GdbWire() {
 			out := resp
 			partial := false
-			if idx := bytes.Index(out, []byte{'\n'}); idx >= 0 {
+			if idx := bytes.Index(out, []byte{'\n'}); idx >= 0 && !binary {
 				out = resp[:idx]
 				partial = true
 			}
@@ -1100,9 +1100,17 @@ func (conn *gdbConn) recv(cmd []byte, context string, binary bool) (resp []byte,
 				partial = true
 			}
 			if !partial {
-				conn.log.Debugf("-> %s%s", string(resp), string(conn.inbuf[:2]))
+				if binary {
+					conn.log.Debugf("-> %q%s", string(resp), string(conn.inbuf[:2]))
+				} else {
+					conn.log.Debugf("-> %s%s", string(resp), string(conn.inbuf[:2]))
+				}
 			} else {
-				conn.log.Debugf("-> %s...", string(out))
+				if binary {
+					conn.log.Debugf("-> %q...", string(out))
+				} else {
+					conn.log.Debugf("-> %s...", string(out))
+				}
 			}
 		}
 
@@ -1136,7 +1144,7 @@ func (conn *gdbConn) recv(cmd []byte, context string, binary bool) (resp []byte,
 		conn.inbuf, resp = wiredecode(resp, conn.inbuf)
 	}
 
-	if len(resp) == 0 || resp[0] == 'E' {
+	if len(resp) == 0 || (resp[0] == 'E' && !binary) || (resp[0] == 'E' && len(resp) == 3) {
 		cmdstr := ""
 		if cmd != nil {
 			cmdstr = string(cmd)
diff --git a/service/dap/server.go b/service/dap/server.go
index 9618802b..0669822b 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -18,6 +18,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 
 	"github.com/go-delve/delve/pkg/gobuild"
 	"github.com/go-delve/delve/pkg/logflags"
@@ -72,12 +73,15 @@ type launchAttachArgs struct {
 	stopOnEntry bool
 	// stackTraceDepth is the maximum length of the returned list of stack frames.
 	stackTraceDepth int
+	// showGlobalVariables indicates if global package variables should be loaded.
+	showGlobalVariables bool
 }
 
 // defaultArgs borrows the defaults for the arguments from the original vscode-go adapter.
 var defaultArgs = launchAttachArgs{
-	stopOnEntry:     false,
-	stackTraceDepth: 50,
+	stopOnEntry:         false,
+	stackTraceDepth:     50,
+	showGlobalVariables: false,
 }
 
 // NewServer creates a new DAP Server. It takes an opened Listener
@@ -457,13 +461,19 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 		return
 	}
 
-	stop, ok := request.Arguments["stopOnEntry"]
-	s.args.stopOnEntry = ok && stop == true
-
+	// If user-specified, overwrite the defaults for optional args.
+	stop, ok := request.Arguments["stopOnEntry"].(bool)
+	if ok {
+		s.args.stopOnEntry = stop
+	}
 	depth, ok := request.Arguments["stackTraceDepth"].(float64)
 	if ok && depth > 0 {
 		s.args.stackTraceDepth = int(depth)
 	}
+	globals, ok := request.Arguments["showGlobalVariables"].(bool)
+	if ok {
+		s.args.showGlobalVariables = globals
+	}
 
 	var targetArgs []string
 	args, ok := request.Arguments["args"]
@@ -732,18 +742,51 @@ func (s *Server) onScopesRequest(request *dap.ScopesRequest) {
 	// Retrieve local variables
 	locals, err := s.debugger.LocalVariables(goid, frame, 0, cfg)
 	if err != nil {
-		s.sendErrorResponse(request.Request, UnableToListLocals, "Unable to list local vars", err.Error())
+		s.sendErrorResponse(request.Request, UnableToListLocals, "Unable to list locals", err.Error())
 		return
 	}
 	locScope := &proc.Variable{Name: "Locals", Children: slicePtrVarToSliceVar(locals)}
 
 	// TODO(polina): Annotate shadowed variables
-	// TODO(polina): Retrieve global variables
 
 	scopeArgs := dap.Scope{Name: argScope.Name, VariablesReference: s.variableHandles.create(argScope)}
 	scopeLocals := dap.Scope{Name: locScope.Name, VariablesReference: s.variableHandles.create(locScope)}
 	scopes := []dap.Scope{scopeArgs, scopeLocals}
 
+	if s.args.showGlobalVariables {
+		// Limit what global variables we will return to the current package only.
+		// TODO(polina): This is how vscode-go currently does it to make
+		// the amount of the returned data manageable. In fact, this is
+		// considered so expensive even with the package filter, that
+		// the default for showGlobalVariables was recently flipped to
+		// not showing. If we delay loading of the globals until the corresponding
+		// scope is expanded, generating an explicit variable request,
+		// should we consider making all globals accessible with a scope per package?
+		// Or users can just rely on watch variables.
+		currPkg, err := s.debugger.CurrentPackage()
+		if err != nil {
+			s.sendErrorResponse(request.Request, UnableToListGlobals, "Unable to list globals", err.Error())
+			return
+		}
+		currPkgFilter := fmt.Sprintf("^%s\\.", currPkg)
+		globals, err := s.debugger.PackageVariables(s.debugger.CurrentThread().ThreadID(), currPkgFilter, cfg)
+		if err != nil {
+			s.sendErrorResponse(request.Request, UnableToListGlobals, "Unable to list globals", err.Error())
+			return
+		}
+		// Remove package prefix from the fully-qualified variable names.
+		// We will include the package info once in the name of the scope instead.
+		for i, g := range globals {
+			globals[i].Name = strings.TrimPrefix(g.Name, currPkg+".")
+		}
+
+		globScope := &proc.Variable{
+			Name:     fmt.Sprintf("Globals (package %s)", currPkg),
+			Children: slicePtrVarToSliceVar(globals),
+		}
+		scopeGlobals := dap.Scope{Name: globScope.Name, VariablesReference: s.variableHandles.create(globScope)}
+		scopes = append(scopes, scopeGlobals)
+	}
 	response := &dap.ScopesResponse{
 		Response: *newResponse(request.Request),
 		Body:     dap.ScopesResponseBody{Scopes: scopes},
diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go
index b95629e1..4f48ada9 100644
--- a/service/debugger/debugger.go
+++ b/service/debugger/debugger.go
@@ -1415,6 +1415,26 @@ func (d *Debugger) convertDefers(defers []*proc.Defer) []api.Defer {
 	return r
 }
 
+// CurrentPackage returns the fully qualified name of the
+// package corresponding to the function location of the
+// current thread.
+func (d *Debugger) CurrentPackage() (string, error) {
+	d.targetMutex.Lock()
+	defer d.targetMutex.Unlock()
+
+	if _, err := d.target.Valid(); err != nil {
+		return "", err
+	}
+	loc, err := d.target.CurrentThread().Location()
+	if err != nil {
+		return "", err
+	}
+	if loc.Fn == nil {
+		return "", fmt.Errorf("unable to determine current package due to unspecified function location")
+	}
+	return loc.Fn.PackageName(), nil
+}
+
 // FindLocation will find the location specified by 'locStr'.
 func (d *Debugger) FindLocation(goid, frame, deferredCall int, locStr string, includeNonExecutableLines bool) ([]api.Location, error) {
 	d.targetMutex.Lock()
@@ -1490,6 +1510,13 @@ func (d *Debugger) Recorded() (recorded bool, tracedir string) {
 	return d.target.Recorded()
 }
 
+// CurrentThread returns the current thread.
+func (d *Debugger) CurrentThread() proc.Thread {
+	d.targetMutex.Lock()
+	defer d.targetMutex.Unlock()
+	return d.target.CurrentThread()
+}
+
 // Checkpoint will set a checkpoint specified by the locspec.
 func (d *Debugger) Checkpoint(where string) (int, error) {
 	d.targetMutex.Lock()


Test patch:
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index 55ad100f..fb870627 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -341,6 +341,9 @@ func TestSetBreakpoint(t *testing.T) {
 
 		client.ScopesRequest(1000)
 		scopes := client.ExpectScopesResponse(t)
+		if len(scopes.Body.Scopes) > 2 {
+			t.Errorf("\ngot  %#v\nwant len(Scopes)=2 (Arguments & Locals)", scopes)
+		}
 		expectScope(t, scopes, 0, "Arguments", 1000)
 		expectScope(t, scopes, 1, "Locals", 1001)
 
@@ -413,12 +416,12 @@ func expectScope(t *testing.T, got *dap.ScopesResponse, i int, name string, varR
 }
 
 // expectChildren is a helper for verifying the number of variables within a VariablesResponse.
-//      parentName - name of the enclosing variable or scope
+//      parentName - pseudoname of the enclosing variable or scope (used for error message only)
 //      numChildren - number of variables/fields/elements of this variable
 func expectChildren(t *testing.T, got *dap.VariablesResponse, parentName string, numChildren int) {
 	t.Helper()
 	if len(got.Body.Variables) != numChildren {
-		t.Errorf("\ngot  len(%s)=%d\nwant %d", parentName, len(got.Body.Variables), numChildren)
+		t.Errorf("\ngot  len(%s)=%d (children=%#v)\nwant len=%d", parentName, len(got.Body.Variables), got.Body.Variables, numChildren)
 	}
 }
 
@@ -432,7 +435,7 @@ func expectChildren(t *testing.T, got *dap.VariablesResponse, parentName string,
 func expectVar(t *testing.T, got *dap.VariablesResponse, i int, name, value string, useExactMatch, hasRef bool) (ref int) {
 	t.Helper()
 	if len(got.Body.Variables) <= i {
-		t.Errorf("\ngot  len=%d\nwant len>%d", len(got.Body.Variables), i)
+		t.Errorf("\ngot  len=%d (children=%#v)\nwant len>%d", len(got.Body.Variables), got.Body.Variables, i)
 		return
 	}
 	if i < 0 {
@@ -556,7 +559,9 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 		runDebugSessionWithBPs(t, client,
 			// Launch
 			func() {
-				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+				client.LaunchRequestWithArgs(map[string]interface{}{
+					"mode": "exec", "program": fixture.Path, "showGlobalVariables": true,
+				})
 			},
 			// Breakpoints are set within the program
 			fixture.Source, []int{},
@@ -580,6 +585,7 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 					scopes := client.ExpectScopesResponse(t)
 					expectScope(t, scopes, 0, "Arguments", 1000)
 					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package main)", 1002)
 
 					// Arguments
 
@@ -596,6 +602,12 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 						expectVarExact(t, bar, 1, "Bur", `"lorem"`, noChildren)
 					}
 
+					// Globals
+
+					client.VariablesRequest(1002)
+					globals := client.ExpectVariablesResponse(t)
+					expectVarExact(t, globals, 0, "p1", "10", noChildren)
+
 					// Locals
 
 					client.VariablesRequest(1001)
@@ -775,6 +787,7 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 					scopes := client.ExpectScopesResponse(t)
 					expectScope(t, scopes, 0, "Arguments", 1000)
 					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package main)", 1002)
 
 					client.ScopesRequest(1111)
 					erres := client.ExpectErrorResponse(t)
@@ -791,6 +804,10 @@ func TestScopesAndVariablesRequests(t *testing.T) {
 					expectChildren(t, locals, "Locals", 1)
 					expectVarExact(t, locals, -1, "a1", `"bur"`, noChildren)
 
+					client.VariablesRequest(1002) // Globals
+					globals := client.ExpectVariablesResponse(t)
+					expectVarExact(t, globals, 0, "p1", "10", noChildren)
+
 					client.VariablesRequest(7777)
 					erres = client.ExpectErrorResponse(t)
 					if erres.Body.Error.Format != "Unable to lookup variable: unknown reference 7777" {
@@ -835,6 +852,9 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 
 					client.ScopesRequest(1000)
 					scopes := client.ExpectScopesResponse(t)
+					if len(scopes.Body.Scopes) > 2 {
+						t.Errorf("\ngot  %#v\nwant len(scopes)=2 (Argumes & Locals)", scopes)
+					}
 					expectScope(t, scopes, 0, "Arguments", 1000)
 					expectScope(t, scopes, 1, "Locals", 1001)
 
@@ -964,18 +984,18 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 						ref = expectVarExact(t, m4, 2, "[key 1]", "<main.astruct>", hasChildren)
 						if ref > 0 {
 							client.VariablesRequest(ref)
-							m4_key1 := client.ExpectVariablesResponse(t)
-							expectChildren(t, m4_key1, "m4_key1", 2)
-							expectVarExact(t, m4_key1, 0, "A", "2", noChildren)
-							expectVarExact(t, m4_key1, 1, "B", "2", noChildren)
+							m4Key1 := client.ExpectVariablesResponse(t)
+							expectChildren(t, m4Key1, "m4Key1", 2)
+							expectVarExact(t, m4Key1, 0, "A", "2", noChildren)
+							expectVarExact(t, m4Key1, 1, "B", "2", noChildren)
 						}
 						ref = expectVarExact(t, m4, 3, "[val 1]", "<main.astruct>", hasChildren)
 						if ref > 0 {
 							client.VariablesRequest(ref)
-							m4_val1 := client.ExpectVariablesResponse(t)
-							expectChildren(t, m4_val1, "m4_val1", 2)
-							expectVarExact(t, m4_val1, 0, "A", "22", noChildren)
-							expectVarExact(t, m4_val1, 1, "B", "22", noChildren)
+							m4Val1 := client.ExpectVariablesResponse(t)
+							expectChildren(t, m4Val1, "m4Val1", 2)
+							expectVarExact(t, m4Val1, 0, "A", "22", noChildren)
+							expectVarExact(t, m4Val1, 1, "B", "22", noChildren)
 						}
 					}
 					expectVarExact(t, locals, -1, "emptymap", "<map[string]string> (length: 0)", noChildren)
@@ -1020,6 +1040,73 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 	})
 }
 
+// TestGlobalScopeAndVariables launches the program with showGlobalVariables
+// arg set, executes to a breakpoint in the main package and tests that global
+// package main variables got loaded. It then steps into a function
+// in another package and tests that globals scope got updated to those vars.
+func TestGlobalScopeAndVariables(t *testing.T) {
+	runTest(t, "consts", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequestWithArgs(map[string]interface{}{
+					"mode": "exec", "program": fixture.Path, "showGlobalVariables": true,
+				})
+			},
+			// Breakpoints are set within the program
+			fixture.Source, []int{},
+			[]onBreakpoint{{
+				// Stop at line 36
+				execute: func() {
+					client.StackTraceRequest(1, 0, 20)
+					stack := client.ExpectStackTraceResponse(t)
+					expectStackFrames(t, stack, 36, 1000, 3, 3)
+
+					client.ScopesRequest(1000)
+					scopes := client.ExpectScopesResponse(t)
+					expectScope(t, scopes, 0, "Arguments", 1000)
+					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package main)", 1002)
+
+					client.VariablesRequest(1002)
+					client.ExpectVariablesResponse(t)
+					// The program has no user-defined globals.
+					// Depending on the Go version, there might
+					// be some runtime globals (e.g. main..inittask)
+					// so testing for the total number is too fragile.
+
+					// Step into pkg.AnotherMethod()
+					client.StepInRequest(1)
+					client.ExpectStepInResponse(t)
+					client.ExpectStoppedEvent(t)
+
+					client.StackTraceRequest(1, 0, 20)
+					stack = client.ExpectStackTraceResponse(t)
+					expectStackFrames(t, stack, 14, 1000, 4, 4)
+
+					client.ScopesRequest(1000)
+					scopes = client.ExpectScopesResponse(t)
+					expectScope(t, scopes, 0, "Arguments", 1000)
+					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package github.com/go-delve/delve/_fixtures/internal/dir0/pkg)", 1002)
+
+					client.VariablesRequest(1002)
+					globals := client.ExpectVariablesResponse(t)
+					expectChildren(t, globals, "Globals", 1)
+					ref := expectVarExact(t, globals, 0, "SomeVar", "<github.com/go-delve/delve/_fixtures/internal/dir0/pkg.SomeType>", hasChildren)
+
+					if ref > 0 {
+						client.VariablesRequest(ref)
+						somevar := client.ExpectVariablesResponse(t)
+						expectChildren(t, somevar, "SomeVar", 1)
+						expectVarExact(t, somevar, 0, "X", "0", noChildren)
+					}
+				},
+				disconnect: false,
+			}})
+	})
+}
+
 // Tests that 'stackTraceDepth' from LaunchRequest is parsed and passed to
 // stacktrace requests handlers.
 func TestLaunchRequestWithStackTraceDepth(t *testing.T) {