Repository: go-delve/delve
Pull request #2399 (2021-03-22 16:35:01)

service/dap: truncate long compound map keys and use unique address suffix for uniqueness
When keys are truncated, there is risk of duplication in remaining strings. Using address keys as suffixes fixes that.
Removes suffixes in all other cases because after change #2383 keys that have not been truncated are unique inlined values rather than identical key type placeholders.

Updates https://github.com/golang/vscode-go/issues/1267


Patch:
diff --git a/cmd/dlv/cmds/commands.go b/cmd/dlv/cmds/commands.go
index 649276b6..2e94261f 100644
--- a/cmd/dlv/cmds/commands.go
+++ b/cmd/dlv/cmds/commands.go
@@ -422,7 +422,7 @@ func dapCmd(cmd *cobra.Command, args []string) {
 			fmt.Fprintf(os.Stderr, "Warning: build flags ignored with dap; specify via launch/attach request instead\n")
 		}
 		if workingDir != "" {
-			fmt.Fprintf(os.Stderr, "Warning: working directory ignored with dap; launch requests must specify full program path\n")
+			fmt.Fprintf(os.Stderr, "Warning: working directory ignored with dap: specify via launch request instead\n")
 		}
 		dlvArgs, targetArgs := splitArgs(cmd, args)
 		if len(dlvArgs) > 0 {
diff --git a/service/dap/server.go b/service/dap/server.go
index e9d301d0..e74621a8 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -16,10 +16,13 @@ import (
 	"io"
 	"net"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"reflect"
 	"regexp"
+	"runtime"
 	"strings"
+	"sync"
 
 	"github.com/go-delve/delve/pkg/gobuild"
 	"github.com/go-delve/delve/pkg/logflags"
@@ -67,6 +70,11 @@ type Server struct {
 	variableHandles *variablesHandlesMap
 	// args tracks special settings for handling debug session requests.
 	args launchAttachArgs
+
+	mu sync.Mutex
+
+	// noDebugProcess is set for the noDebug launch process.
+	noDebugProcess *exec.Cmd
 }
 
 // launchAttachArgs captures arguments from launch/attach request that
@@ -428,8 +436,16 @@ func (s *Server) onInitializeRequest(request *dap.InitializeRequest) {
 	s.send(response)
 }
 
-// Output path for the compiled binary in debug or test modes.
-const debugBinary string = "./__debug_bin"
+// Default output file pathname for the compiled binary in debug or test modes,
+// relative to the current working directory of the server.
+const defaultDebugBinary string = "./__debug_bin"
+
+func cleanExeName(name string) string {
+	if runtime.GOOS == "windows" && filepath.Ext(name) != ".exe" {
+		return name + ".exe"
+	}
+	return name
+}
 
 func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 	// Validate launch request mode
@@ -456,9 +472,9 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 	if mode == "debug" || mode == "test" {
 		output, ok := request.Arguments["output"].(string)
 		if !ok || output == "" {
-			output = debugBinary
+			output = cleanExeName(defaultDebugBinary)
 		}
-		debugname, err := filepath.Abs(output)
+		debugbinary, err := filepath.Abs(output)
 		if err != nil {
 			s.sendInternalErrorResponse(request.Seq, err.Error())
 			return
@@ -476,11 +492,12 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 			}
 		}
 
+		s.log.Debugf("building binary at %s", debugbinary)
 		switch mode {
 		case "debug":
-			err = gobuild.GoBuild(debugname, []string{program}, buildFlags)
+			err = gobuild.GoBuild(debugbinary, []string{program}, buildFlags)
 		case "test":
-			err = gobuild.GoTestBuild(debugname, []string{program}, buildFlags)
+			err = gobuild.GoTestBuild(debugbinary, []string{program}, buildFlags)
 		}
 		if err != nil {
 			s.sendErrorResponse(request.Request,
@@ -488,8 +505,8 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 				fmt.Sprintf("Build error: %s", err.Error()))
 			return
 		}
-		program = debugname
-		s.binaryToRemove = debugname
+		program = debugbinary
+		s.binaryToRemove = debugbinary
 	}
 
 	s.setLaunchAttachArgs(request)
@@ -519,6 +536,30 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 	s.config.ProcessArgs = append([]string{program}, targetArgs...)
 	s.config.Debugger.WorkingDir = filepath.Dir(program)
 
+	// Set the WorkingDir for this program to the one specified in the request arguments.
+	wd, ok := request.Arguments["wd"]
+	if ok {
+		wdParsed, ok := wd.(string)
+		if !ok {
+			s.sendErrorResponse(request.Request,
+				FailedToLaunch, "Failed to launch",
+				fmt.Sprintf("'wd' attribute '%v' in debug configuration is not a string.", wd))
+			return
+		}
+		s.config.Debugger.WorkingDir = wdParsed
+	}
+
+	noDebug, ok := request.Arguments["noDebug"]
+	if ok {
+		if v, ok := noDebug.(bool); ok && v { // noDebug == true
+			if err := s.runWithoutDebug(program, targetArgs, s.config.Debugger.WorkingDir); err != nil {
+				s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error())
+			} else { // program terminated.
+				s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")})
+			}
+			return
+		}
+	}
 	var err error
 	if s.debugger, err = debugger.New(&s.config.Debugger, s.config.ProcessArgs); err != nil {
 		s.sendErrorResponse(request.Request,
@@ -533,6 +574,52 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 	s.send(&dap.LaunchResponse{Response: *newResponse(request.Request)})
 }
 
+func (s *Server) runWithoutDebug(program string, targetArgs []string, wd string) error {
+	s.log.Println("Running without debug: ", program)
+
+	cmd := exec.Command(program, targetArgs...)
+	// TODO: send stdin/out/err as OutputEvent messages
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	cmd.Stdin = os.Stdin
+	cmd.Dir = s.config.Debugger.WorkingDir
+
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	if s.noDebugProcess != nil {
+		return fmt.Errorf("previous process (pid=%v) is still active", s.noDebugProcess.Process.Pid)
+	}
+	if err := cmd.Start(); err != nil {
+		return err
+	}
+	s.noDebugProcess = cmd
+	s.mu.Unlock() // allow disconnect or restart requests to call stopNoDebugProcess.
+
+	// block until the process terminates.
+	if err := cmd.Wait(); err != nil {
+		s.log.Errorf("process exited with %v", err)
+	}
+
+	s.mu.Lock()
+	if s.noDebugProcess == cmd {
+		s.noDebugProcess = nil
+	}
+	return nil // Program ran and terminated.
+}
+
+func (s *Server) stopNoDebugProcess() {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	if s.noDebugProcess == nil {
+		return
+	}
+
+	// TODO(hyangah): gracefully terminate the process and its children processes.
+	if err := s.noDebugProcess.Process.Kill(); err != nil {
+		s.log.Errorf("killing process (pid=%v) failed: %v", s.noDebugProcess.Process.Pid, err)
+	}
+}
+
 // TODO(polina): support "remote" mode
 func isValidLaunchMode(launchMode interface{}) bool {
 	switch launchMode {
@@ -565,6 +652,8 @@ func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) {
 		if err != nil {
 			s.log.Error(err)
 		}
+	} else {
+		s.stopNoDebugProcess()
 	}
 	// TODO(polina): make thread-safe when handlers become asynchronous.
 	s.signalDisconnect()
@@ -973,7 +1062,10 @@ func (s *Server) onVariablesRequest(request *dap.VariablesRequest) {
 					Value:        val,
 				}
 				if keyref != 0 { // key is a type to be expanded
-					kvvar.Name = fmt.Sprintf("%s(%#x)", kvvar.Name, keyv.Addr) // Make the name unique
+					if len(key) > DefaultLoadConfig.MaxStringLen {
+						// Truncate and make unique
+						kvvar.Name = fmt.Sprintf("%s... @ %#x", key[0:DefaultLoadConfig.MaxStringLen], keyv.Addr)
+					}
 					kvvar.VariablesReference = keyref
 				} else if valref != 0 { // val is a type to be expanded
 					kvvar.VariablesReference = valref


Test patch:
diff --git a/_fixtures/testvariables2.go b/_fixtures/testvariables2.go
index a3a6deb7..7ffeaec0 100644
--- a/_fixtures/testvariables2.go
+++ b/_fixtures/testvariables2.go
@@ -326,6 +326,7 @@ func main() {
 	runearray := [4]rune{116, 232, 115, 116}
 
 	longstr := "very long string 0123456789a0123456789b0123456789c0123456789d0123456789e0123456789f0123456789g012345678h90123456789i0123456789j0123456789"
+	m5 := map[C]int{{longstr}: 1}
 
 	var nilstruct *astruct = nil
 
@@ -361,5 +362,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, 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, errtypednil, emptyslice, emptymap, byteslice, runeslice, bytearray, runearray, longstr, nilstruct, as2, as2.NonPointerRecieverMethod, s4, iface2map, issue1578, ll, unread, w2, w3, w4, w5, longarr, longslice)
 }
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index 071feaf6..93da346c 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -3,6 +3,7 @@ package dap
 import (
 	"bufio"
 	"flag"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net"
@@ -1190,7 +1191,7 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 						client.VariablesRequest(ref)
 						m3 := client.ExpectVariablesResponse(t)
 						expectChildren(t, m3, "m3", 2) // each key-value represented by a single child
-						ref = expectVarRegex(t, m3, 0, `main\.astruct {A: 1, B: 1}\(0x[0-9a-f]+\)`, `m3\[\(\*\(\*"main.astruct"\)\(0x[0-9a-f]+\)\)\]`, "42", hasChildren)
+						ref = expectVarRegex(t, m3, 0, `main\.astruct {A: 1, B: 1}`, `m3\[\(\*\(\*"main.astruct"\)\(0x[0-9a-f]+\)\)\]`, "42", hasChildren)
 						if ref > 0 {
 							client.VariablesRequest(ref)
 							m3kv0 := client.ExpectVariablesResponse(t)
@@ -1198,7 +1199,7 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 							expectVarRegex(t, m3kv0, 0, "A", `\(*\(*"main\.astruct"\)\(0x[0-9a-f]+\)\)\.A`, "1", noChildren)
 							validateEvaluateName(t, client, m3kv0, 0)
 						}
-						ref = expectVarRegex(t, m3, 1, `main\.astruct {A: 2, B: 2}\(0x[0-9a-f]+\)`, `m3\[\(\*\(\*"main.astruct"\)\(0x[0-9a-f]+\)\)\]`, "43", hasChildren)
+						ref = expectVarRegex(t, m3, 1, `main\.astruct {A: 2, B: 2}`, `m3\[\(\*\(\*"main.astruct"\)\(0x[0-9a-f]+\)\)\]`, "43", hasChildren)
 						if ref > 0 { // inspect another key from another key-value child
 							client.VariablesRequest(ref)
 							m3kv1 := client.ExpectVariablesResponse(t)
@@ -1207,6 +1208,14 @@ func TestScopesAndVariablesRequests2(t *testing.T) {
 							validateEvaluateName(t, client, m3kv1, 1)
 						}
 					}
+					// key - compound + truncated, value - scalar
+					ref = expectVarExact(t, locals, -1, "m5", "m5", `map[main.C]int [{s: "very long string 0123456789a0123456789b0123456789c0123456789d012...+73 more"}: 1, ]`, hasChildren)
+					if ref > 0 {
+						client.VariablesRequest(ref)
+						m5 := client.ExpectVariablesResponse(t)
+						expectChildren(t, m5, "m5", 1)
+						expectVarRegex(t, m5, 0, `main\.C {s: "very long string 0123456789a0123456789b0123456789c01\.\.\. @ 0x[0-9a-f]+`, `m5\[\(\*\(\*"main\.C"\)\(0x[0-9a-f]+\)\)\]`, "1", hasChildren)
+					}
 					// key - compound, value - compound
 					ref = expectVarExact(t, locals, -1, "m4", "m4", "map[main.astruct]main.astruct [{A: 1, B: 1}: {A: 11, B: 11}, {A: 2, B: 2}: {A: 22, B: 22}, ]", hasChildren)
 					if ref > 0 {
@@ -1676,6 +1685,42 @@ func TestSetBreakpoint(t *testing.T) {
 	})
 }
 
+// TestWorkingDir executes to a breakpoint and tests that the specified
+// working directory is the one used to run the program.
+func TestWorkingDir(t *testing.T) {
+	runTest(t, "workdir", func(client *daptest.Client, fixture protest.Fixture) {
+		wd := os.TempDir()
+		// For Darwin `os.TempDir()` returns `/tmp` which is symlink to `/private/tmp`.
+		if runtime.GOOS == "darwin" {
+			wd = "/private/tmp"
+		}
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequestWithArgs(map[string]interface{}{
+					"mode":        "exec",
+					"program":     fixture.Path,
+					"stopOnEntry": false,
+					"wd":          wd,
+				})
+			},
+			// Set breakpoints
+			fixture.Source, []int{10}, // b main.main
+			[]onBreakpoint{{
+				execute: func() {
+					handleStop(t, client, 1, "main.main", 10)
+					client.VariablesRequest(1001) // Locals
+					locals := client.ExpectVariablesResponse(t)
+					expectChildren(t, locals, "Locals", 2)
+					expectVarExact(t, locals, 0, "pwd", "pwd", fmt.Sprintf("%q", wd), noChildren)
+					expectVarExact(t, locals, 1, "err", "err", "error nil", noChildren)
+
+				},
+				disconnect: false,
+			}})
+	})
+}
+
 // expectEval is a helper for verifying the values within an EvaluateResponse.
 //     value - the value of the evaluated expression
 //     hasRef - true if the evaluated expression should have children and therefore a non-0 variable reference
@@ -2278,8 +2323,9 @@ func TestLaunchDebugRequest(t *testing.T) {
 		// We reuse the harness that builds, but ignore the built binary,
 		// only relying on the source to be built in response to LaunchRequest.
 		runDebugSession(t, client, "launch", func() {
+			wd, _ := os.Getwd()
 			client.LaunchRequestWithArgs(map[string]interface{}{
-				"mode": "debug", "program": fixture.Source, "output": "__mydir"})
+				"mode": "debug", "program": fixture.Source, "output": filepath.Join(wd, "__mybin")})
 		}, fixture.Source)
 	})
 	// Wait for the test to finish to capture all stderr
@@ -2309,14 +2355,14 @@ func TestLaunchRequestDefaults(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSession(t, client, "launch", func() {
 			client.LaunchRequestWithArgs(map[string]interface{}{
-				"mode": "" /*"debug" by default*/, "program": fixture.Source, "output": "__mydir"})
+				"mode": "" /*"debug" by default*/, "program": fixture.Source, "output": "__mybin"})
 		}, fixture.Source)
 	})
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSession(t, client, "launch", func() {
 			// Use the default output directory.
 			client.LaunchRequestWithArgs(map[string]interface{}{
-				/*"mode":"debug" by default*/ "program": fixture.Source, "output": "__mydir"})
+				/*"mode":"debug" by default*/ "program": fixture.Source, "output": "__mybin"})
 		}, fixture.Source)
 	})
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
@@ -2329,6 +2375,48 @@ func TestLaunchRequestDefaults(t *testing.T) {
 	})
 }
 
+func TestLaunchRequestDefaultsNoDebug(t *testing.T) {
+	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
+		runNoDebugDebugSession(t, client, "launch", func() {
+			client.LaunchRequestWithArgs(map[string]interface{}{
+				"noDebug": true,
+				"mode":    "", /*"debug" by default*/
+				"program": fixture.Source,
+				"output":  cleanExeName("__mybin")})
+		}, fixture.Source)
+	})
+	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
+		runNoDebugDebugSession(t, client, "launch", func() {
+			// Use the default output directory.
+			client.LaunchRequestWithArgs(map[string]interface{}{
+				"noDebug": true,
+				/*"mode":"debug" by default*/
+				"program": fixture.Source,
+				"output":  cleanExeName("__mybin")})
+		}, fixture.Source)
+	})
+	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
+		runNoDebugDebugSession(t, client, "launch", func() {
+			// Use the default output directory.
+			client.LaunchRequestWithArgs(map[string]interface{}{
+				"noDebug": true,
+				"mode":    "debug",
+				"program": fixture.Source})
+			// writes to default output dir __debug_bin
+		}, fixture.Source)
+	})
+}
+
+func runNoDebugDebugSession(t *testing.T, client *daptest.Client, cmd string, cmdRequest func(), source string) {
+	client.InitializeRequest()
+	client.ExpectInitializeResponse(t)
+
+	cmdRequest()
+	// ! client.InitializedEvent.
+	// ! client.ExpectLaunchResponse
+	client.ExpectTerminatedEvent(t)
+}
+
 func TestLaunchTestRequest(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSession(t, client, "launch", func() {
@@ -2366,7 +2454,7 @@ func TestLaunchRequestWithBuildFlags(t *testing.T) {
 			// We reuse the harness that builds, but ignore the built binary,
 			// only relying on the source to be built in response to LaunchRequest.
 			client.LaunchRequestWithArgs(map[string]interface{}{
-				"mode": "debug", "program": fixture.Source, "output": "__mydir",
+				"mode": "debug", "program": fixture.Source, "output": "__mybin",
 				"buildFlags": "-ldflags '-X main.Hello=World'"})
 		}, fixture.Source)
 	})
@@ -2613,6 +2701,10 @@ func TestBadLaunchRequests(t *testing.T) {
 		expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t),
 			"Failed to launch: 'buildFlags' attribute '123' in debug configuration is not a string.")
 
+		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "wd": 123})
+		expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t),
+			"Failed to launch: 'wd' attribute '123' in debug configuration is not a string.")
+
 		// Skip detailed message checks for potentially different OS-specific errors.
 		client.LaunchRequest("exec", fixture.Path+"_does_not_exist", stopOnEntry)
 		expectFailedToLaunch(client.ExpectErrorResponse(t)) // No such file or directory