Repository: go-delve/delve
Pull request #2056 (2020-05-17 18:27:37)

service/dap: Add buildFlags error checking and tests in launch requests
Updates #1515

polinasok: Do you want me to fix the anti-pattern that DeepSource detected in the original code?

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f48e1f0f..32512883 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,7 +3,38 @@
 All notable changes to this project will be documented in this file.
 This project adheres to Semantic Versioning.
 
-All changes mention the author, unless contributed by me (@derekparker).
+## [1.4.1] 2020-05-22
+
+### Added
+
+- Support for linux/386 added (@chainhelen)
+- DAP server initial release (@polinasok, @eliben, @hyangah)
+- New command `examinemem` (or `x`) allows users to examine raw memory (@chainhelen)
+- New command `display` allows users to print value of an expression every time the program stops (@aarzilli)
+- New flag `--tty` allows users to supply a TTY for the debugged program to use (@derekparker)
+- Coredump support added for Arm64 (@ossdev07)
+- Ability to print goroutine labels (@aarzilli)
+- Allow printing registers for arbitrary stack frames (@aarzilli)
+- Add `disassemble-flavor` to config to specify assembly syntax (@chainhelen)
+
+### Fixed
+
+- Allow function calls on non-struct types (@derekparker)
+- Dwarf line parsing bug fix (@klemens-morgenstern)
+- Improved error message when building Delve on unsupported systems (@aarzilli)
+- Improved error message when trying to execute a binary in an invalid format for host system (@derekparker)
+- Fix panic in Delve when using `call` command with some invalid input (@chainhelen)
+
+### Changed
+
+- Improved output from `dlv trace` and `trace` REPL commands (@derekparker)
+- Conditional breakpoint performance improvements (@aarzilli)
+- Thread register loading performance improvement on gdbserial backend (@derekparker)
+- Reduce default log level to error (@aarzilli)
+- Linux memory read/write optimization using process_vm_read/write (@cuviper)
+- Terminal output of commands divided into categories (@aarzilli)
+- Use less permissive file settings on history file (@derekparker)
+- Autogenerated interface method calls wrappers now automatically stepped through (@aarzilli)
 
 ## [1.4.0] 2020-02-11
 
diff --git a/README.md b/README.md
index 55b50558..a8824739 100644
--- a/README.md
+++ b/README.md
@@ -3,7 +3,6 @@
 [![license](http://img.shields.io/badge/license-MIT-blue.svg)](https://raw.githubusercontent.com/go-delve/delve/master/LICENSE)
 [![GoDoc](https://godoc.org/github.com/go-delve/delve?status.svg)](https://godoc.org/github.com/go-delve/delve)
 [![Build Status](https://travis-ci.org/go-delve/delve.svg?branch=master)](https://travis-ci.org/go-delve/delve)
-[![Build status](https://ci.appveyor.com/api/projects/status/0v8r99smxost466s/branch/master?svg=true)](https://ci.appveyor.com/project/derekparker/delve-facy3/branch/master)
 
 The GitHub issue tracker is for **bugs** only. Please use the [developer mailing list](https://groups.google.com/forum/#!forum/delve-dev) for any feature proposals and discussions.
 
diff --git a/pkg/version/version.go b/pkg/version/version.go
index 67422e6f..bda60614 100644
--- a/pkg/version/version.go
+++ b/pkg/version/version.go
@@ -14,7 +14,7 @@ type Version struct {
 var (
 	// DelveVersion is the current version of Delve.
 	DelveVersion = Version{
-		Major: "1", Minor: "4", Patch: "0", Metadata: "",
+		Major: "1", Minor: "4", Patch: "1", Metadata: "",
 		Build: "$Id$",
 	}
 )
diff --git a/service/dap/server.go b/service/dap/server.go
index eb140818..c2fd0f20 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -402,9 +402,16 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 			return
 		}
 
-		buildFlags, ok := request.Arguments["buildFlags"].(string)
-		if !ok {
-			buildFlags = ""
+		buildFlags := ""
+		buildFlagsArg, ok := request.Arguments["buildFlags"]
+		if ok {
+			buildFlags, ok = buildFlagsArg.(string)
+			if !ok {
+				s.sendErrorResponse(request.Request,
+					FailedToContinue, "Failed to launch",
+					fmt.Sprintf("'buildFlags' attribute '%v' in debug configuration is not a string.", buildFlagsArg))
+				return
+			}
 		}
 
 		switch mode {


Test patch:
diff --git a/_fixtures/buildflagtest.go b/_fixtures/buildflagtest.go
new file mode 100644
index 00000000..92ae8c5e
--- /dev/null
+++ b/_fixtures/buildflagtest.go
@@ -0,0 +1,14 @@
+package main
+
+import "fmt"
+
+// To be set via
+//   go build -ldflags '-X main.Hello=World'
+var Hello string
+
+func main() {
+	if len(Hello) < 1 {
+		panic("global main.Hello not set via build flags")
+	}
+	fmt.Printf("Hello %s!\n", Hello)
+}
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index d020f16e..ff86808b 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -345,7 +345,8 @@ func runDebugSession(t *testing.T, client *daptest.Client, launchRequest func())
 
 func TestLaunchDebugRequest(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
-		// We reuse the harness that builds, but ignore the actual binary.
+		// 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, func() {
 			// Use the default output directory.
 			client.LaunchRequestWithArgs(map[string]interface{}{
@@ -357,7 +358,8 @@ func TestLaunchDebugRequest(t *testing.T) {
 func TestLaunchTestRequest(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSession(t, client, func() {
-			// We reuse the harness that builds, but ignore the actual binary.
+			// We reuse the harness that builds, but ignore the built binary,
+			// only relying on the source to be built in response to LaunchRequest.
 			fixtures := protest.FindFixturesDir()
 			testdir, _ := filepath.Abs(filepath.Join(fixtures, "buildtest"))
 			client.LaunchRequestWithArgs(map[string]interface{}{
@@ -366,6 +368,10 @@ func TestLaunchTestRequest(t *testing.T) {
 	})
 }
 
+// Tests that 'args' from LaunchRequest are parsed and passed to the target
+// program. The target program exits without an error on success, and
+// panics on error, causing an unexpected StoppedEvent instead of
+// Terminated Event.
 func TestLaunchRequestWithArgs(t *testing.T) {
 	runTest(t, "testargs", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSession(t, client, func() {
@@ -376,6 +382,22 @@ func TestLaunchRequestWithArgs(t *testing.T) {
 	})
 }
 
+// Tests that 'buildFlags' from LaunchRequest are parsed and passed to the
+// compiler. The target program exits without an error on success, and
+// panics on error, causing an unexpected StoppedEvent instead of
+// TerminatedEvent.
+func TestLaunchRequestWithBuildFlags(t *testing.T) {
+	runTest(t, "buildflagtest", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSession(t, client, func() {
+			// 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,
+				"buildFlags": "-ldflags '-X main.Hello=World'"})
+		})
+	})
+}
+
 func TestUnupportedCommandResponses(t *testing.T) {
 	var got *dap.ErrorResponse
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
@@ -586,6 +608,10 @@ func TestBadLaunchRequests(t *testing.T) {
 		expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t),
 			"Failed to launch: value '1' in 'args' attribute in debug configuration is not a string.")
 
+		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "buildFlags": 123})
+		expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t),
+			"Failed to launch: 'buildFlags' 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))
@@ -596,6 +622,9 @@ func TestBadLaunchRequests(t *testing.T) {
 		client.LaunchRequest("exec", fixture.Source, stopOnEntry)
 		expectFailedToLaunch(client.ExpectErrorResponse(t)) // Not an executable
 
+		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "buildFlags": "123"})
+		expectFailedToLaunch(client.ExpectErrorResponse(t)) // Build error
+
 		// We failed to launch the program. Make sure shutdown still works.
 		client.DisconnectRequest()
 		dresp := client.ExpectDisconnectResponse(t)