Repository: go-delve/delve
Pull request #2186 (2020-09-28 18:04:20)

service/dap: add "panic" and "fatal error" as stopped reasons
The unrecovered panic and fatal throw breakpoints are not set by the
user. We now check for these special breakpoints and send appropriate
stopped reasons to the client.

suzmue: @polinasok

Patch:
diff --git a/service/dap/error_ids.go b/service/dap/error_ids.go
index 84903eb0..c75b4d42 100644
--- a/service/dap/error_ids.go
+++ b/service/dap/error_ids.go
@@ -12,6 +12,7 @@ const (
 	// values below are inspired the original vscode-go debug adaptor.
 	FailedToLaunch            = 3000
 	FailedtoAttach            = 3001
+	UnableToSetBreakpoints    = 2002
 	UnableToDisplayThreads    = 2003
 	UnableToProduceStackTrace = 2004
 	UnableToListLocals        = 2005
diff --git a/service/dap/server.go b/service/dap/server.go
index 62dd14d9..6557a6c1 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -380,6 +380,7 @@ func (s *Server) onInitializeRequest(request *dap.InitializeRequest) {
 	// TODO(polina): Respond with an error if debug session is in progress?
 	response := &dap.InitializeResponse{Response: *newResponse(request.Request)}
 	response.Body.SupportsConfigurationDoneRequest = true
+	response.Body.SupportsConditionalBreakpoints = true
 	// TODO(polina): support this to match vscode-go functionality
 	response.Body.SupportsSetVariable = false
 	// TODO(polina): support these requests in addition to vscode-go feature parity
@@ -535,27 +536,57 @@ func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) {
 }
 
 func (s *Server) onSetBreakpointsRequest(request *dap.SetBreakpointsRequest) {
+	// TODO(polina): handle this while running by halting first.
+
 	if request.Arguments.Source.Path == "" {
-		s.log.Error("ERROR: Unable to set breakpoint for empty file path")
+		s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "empty file path")
+		return
+	}
+
+	// According to the spec we should "set multiple breakpoints for a single source
+	// and clear all previous breakpoints in that source." The simplest way is
+	// to clear all and then set all.
+	//
+	// TODO(polina): should we optimize this as follows?
+	// See https://github.com/golang/vscode-go/issues/163 for details.
+	// If a breakpoint:
+	// -- exists and not in request => ClearBreakpoint
+	// -- exists and in request => AmendBreakpoint
+	// -- doesn't exist and in request => SetBreakpoint
+
+	// Clear all existing breakpoints in the file.
+	existing := s.debugger.Breakpoints()
+	for _, bp := range existing {
+		// Skip special breakpoints such as for panic.
+		if bp.ID < 0 {
+			continue
+		}
+		// Skip other source files.
+		// TODO(polina): should this be normalized because of different OSes?
+		if bp.File != request.Arguments.Source.Path {
+			continue
+		}
+		_, err := s.debugger.ClearBreakpoint(bp)
+		if err != nil {
+			s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error())
+			return
+		}
 	}
+
+	// Set all requested breakpoints.
 	response := &dap.SetBreakpointsResponse{Response: *newResponse(request.Request)}
 	response.Body.Breakpoints = make([]dap.Breakpoint, len(request.Arguments.Breakpoints))
-	// Only verified breakpoints will be set and reported back in the
-	// response. All breakpoints resulting in errors (e.g. duplicates
-	// or lines that do not have statements) will be skipped.
-	i := 0
-	for _, b := range request.Arguments.Breakpoints {
-		bp, err := s.debugger.CreateBreakpoint(
-			&api.Breakpoint{File: request.Arguments.Source.Path, Line: b.Line})
+	for i, want := range request.Arguments.Breakpoints {
+		got, err := s.debugger.CreateBreakpoint(
+			&api.Breakpoint{File: request.Arguments.Source.Path, Line: want.Line, Cond: want.Condition})
+		response.Body.Breakpoints[i].Verified = (err == nil)
 		if err != nil {
-			s.log.Error("ERROR:", err)
-			continue
+			response.Body.Breakpoints[i].Line = want.Line
+			response.Body.Breakpoints[i].Message = err.Error()
+		} else {
+			response.Body.Breakpoints[i].Line = got.Line
 		}
-		response.Body.Breakpoints[i].Verified = true
-		response.Body.Breakpoints[i].Line = bp.Line
-		i++
 	}
-	response.Body.Breakpoints = response.Body.Breakpoints[:i]
 	s.send(response)
 }
 
@@ -1164,13 +1195,24 @@ func (s *Server) doCommand(command string) {
 	stopped.Body.AllThreadsStopped = true
 
 	if err == nil {
-		stopped.Body.ThreadId = state.SelectedGoroutine.ID
-		switch command {
-		case api.Next, api.Step, api.StepOut:
+		if state.SelectedGoroutine != nil {
+			stopped.Body.ThreadId = state.SelectedGoroutine.ID
+		}
+
+		switch s.debugger.StopReason() {
+		case proc.StopNextFinished:
 			stopped.Body.Reason = "step"
 		default:
 			stopped.Body.Reason = "breakpoint"
 		}
+		if state.CurrentThread.Breakpoint != nil {
+			switch state.CurrentThread.Breakpoint.Name {
+			case proc.FatalThrow:
+				stopped.Body.Reason = "fatal error"
+			case proc.UnrecoveredPanic:
+				stopped.Body.Reason = "panic"
+			}
+		}
 		s.send(stopped)
 	} else {
 		s.log.Error("runtime error: ", err)
diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go
index 28123a79..c58ae2bf 100644
--- a/service/debugger/debugger.go
+++ b/service/debugger/debugger.go
@@ -1602,6 +1602,15 @@ func (d *Debugger) StopRecording() error {
 	return d.stopRecording()
 }
 
+// StopReason returns the reason the reason why the target process is stopped.
+// A process could be stopped for multiple simultaneous reasons, in which
+// case only one will be reported.
+func (d *Debugger) StopReason() proc.StopReason {
+	d.targetMutex.Lock()
+	defer d.targetMutex.Unlock()
+	return d.target.StopReason
+}
+
 // LockTarget acquires the target mutex.
 func (d *Debugger) LockTarget() {
 	d.targetMutex.Lock()


Test patch:
diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go
index e06cddbb..b35f888f 100644
--- a/service/dap/daptest/client.go
+++ b/service/dap/daptest/client.go
@@ -329,6 +329,11 @@ func (c *Client) DisconnectRequest() {
 
 // SetBreakpointsRequest sends a 'setBreakpoints' request.
 func (c *Client) SetBreakpointsRequest(file string, lines []int) {
+	c.SetConditionalBreakpointsRequest(file, lines, nil)
+}
+
+// SetBreakpointsRequest sends a 'setBreakpoints' request with conditions.
+func (c *Client) SetConditionalBreakpointsRequest(file string, lines []int, conditions map[int]string) {
 	request := &dap.SetBreakpointsRequest{Request: *c.newRequest("setBreakpoints")}
 	request.Arguments = dap.SetBreakpointsArguments{
 		Source: dap.Source{
@@ -340,6 +345,10 @@ func (c *Client) SetBreakpointsRequest(file string, lines []int) {
 	}
 	for i, l := range lines {
 		request.Arguments.Breakpoints[i].Line = l
+		cond, ok := conditions[l]
+		if ok {
+			request.Arguments.Breakpoints[i].Condition = cond
+		}
 	}
 	c.send(request)
 }
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index b2a82d04..6ccd14e7 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -261,9 +261,9 @@ func TestContinueOnEntry(t *testing.T) {
 	})
 }
 
-// TestSetBreakpoint corresponds to a debug session that is configured to
+// TestPreSetBreakpoint corresponds to a debug session that is configured to
 // continue on entry with a pre-set breakpoint.
-func TestSetBreakpoint(t *testing.T) {
+func TestPreSetBreakpoint(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
 		client.InitializeRequest()
 		client.ExpectInitializeResponse(t)
@@ -272,7 +272,7 @@ func TestSetBreakpoint(t *testing.T) {
 		client.ExpectInitializedEvent(t)
 		client.ExpectLaunchResponse(t)
 
-		client.SetBreakpointsRequest(fixture.Source, []int{8, 100})
+		client.SetBreakpointsRequest(fixture.Source, []int{8})
 		sResp := client.ExpectSetBreakpointsResponse(t)
 		if len(sResp.Body.Breakpoints) != 1 {
 			t.Errorf("got %#v, want len(Breakpoints)=1", sResp)
@@ -479,8 +479,7 @@ func expectVarRegex(t *testing.T, got *dap.VariablesResponse, i int, name, value
 	return expectVar(t, got, i, name, value, false, hasRef)
 }
 
-// TestStackTraceRequest executes to a breakpoint (similarly to TestSetBreakpoint
-// that includes more thorough checking of that sequence) and tests different
+// TestStackTraceRequest executes to a breakpoint and tests different
 // good and bad configurations of 'stackTrace' requests.
 func TestStackTraceRequest(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
@@ -1134,6 +1133,104 @@ func TestLaunchRequestWithStackTraceDepth(t *testing.T) {
 	})
 }
 
+// TestSetBreakpoint executes to a breakpoint and tests different
+// configurations of setBreakpoint requests.
+func TestSetBreakpoint(t *testing.T) {
+	runTest(t, "loopprog", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{16}, // b main.main
+			[]onBreakpoint{{
+				execute: func() {
+					handleStop(t, client, 1, 16)
+
+					type Breakpoint struct {
+						line      int
+						verified  bool
+						msgPrefix string
+					}
+					expectSetBreakpointsResponse := func(bps []Breakpoint) {
+						t.Helper()
+						got := client.ExpectSetBreakpointsResponse(t)
+						if len(got.Body.Breakpoints) != len(bps) {
+							t.Errorf("got %#v,\nwant len(Breakpoints)=%d", got, len(bps))
+							return
+						}
+						for i, bp := range got.Body.Breakpoints {
+							if bp.Line != bps[i].line || bp.Verified != bps[i].verified ||
+								!strings.HasPrefix(bp.Message, bps[i].msgPrefix) {
+								t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i])
+							}
+						}
+					}
+
+					// Set two breakpoints at the next two lines in main
+					client.SetBreakpointsRequest(fixture.Source, []int{17, 18})
+					expectSetBreakpointsResponse([]Breakpoint{{17, true, ""}, {18, true, ""}})
+
+					// Clear 17, reset 18
+					client.SetBreakpointsRequest(fixture.Source, []int{18})
+					expectSetBreakpointsResponse([]Breakpoint{{18, true, ""}})
+
+					// Skip 17, continue to 18
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					handleStop(t, client, 1, 18)
+
+					// Set another breakpoint inside the loop in loop(), twice to trigger error
+					client.SetBreakpointsRequest(fixture.Source, []int{8, 8})
+					expectSetBreakpointsResponse([]Breakpoint{{8, true, ""}, {8, false, "Breakpoint exists"}})
+
+					// Continue into the loop
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					handleStop(t, client, 1, 8)
+					client.VariablesRequest(1001) // Locals
+					locals := client.ExpectVariablesResponse(t)
+					expectVarExact(t, locals, 0, "i", "0", noChildren) // i == 0
+
+					// Edit the breakpoint to add a condition
+					client.SetConditionalBreakpointsRequest(fixture.Source, []int{8}, map[int]string{8: "i == 3"})
+					expectSetBreakpointsResponse([]Breakpoint{{8, true, ""}})
+
+					// Continue until condition is hit
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					handleStop(t, client, 1, 8)
+					client.VariablesRequest(1001) // Locals
+					locals = client.ExpectVariablesResponse(t)
+					expectVarExact(t, locals, 0, "i", "3", noChildren) // i == 3
+
+					// Edit the breakpoint to remove a condition
+					client.SetConditionalBreakpointsRequest(fixture.Source, []int{8}, map[int]string{8: ""})
+					expectSetBreakpointsResponse([]Breakpoint{{8, true, ""}})
+
+					// Continue for one more loop iteration
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					handleStop(t, client, 1, 8)
+					client.VariablesRequest(1001) // Locals
+					locals = client.ExpectVariablesResponse(t)
+					expectVarExact(t, locals, 0, "i", "4", noChildren) // i == 4
+
+					// Set at a line without a statement
+					client.SetBreakpointsRequest(fixture.Source, []int{1000})
+					expectSetBreakpointsResponse([]Breakpoint{{1000, false, "could not find statement"}}) // all cleared, none set
+				},
+				// The program has an infinite loop, so we must kill it by disconnecting.
+				disconnect: true,
+			}})
+	})
+}
+
 func TestNextAndStep(t *testing.T) {
 	runTest(t, "testinline", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSessionWithBPs(t, client,
@@ -1230,6 +1327,91 @@ func TestBadAccess(t *testing.T) {
 	})
 }
 
+func TestPanicBreakpointOnContinue(t *testing.T) {
+	runTest(t, "panic", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{5},
+			[]onBreakpoint{{
+				execute: func() {
+					handleStop(t, client, 1, 5)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+
+					se := client.ExpectStoppedEvent(t)
+					if se.Body.ThreadId != 1 || se.Body.Reason != "panic" {
+						t.Errorf("\ngot  %#v\nwant ThreadId=1 Reason=\"panic\"", se)
+					}
+				},
+				disconnect: true,
+			}})
+	})
+}
+
+func TestPanicBreakpointOnNext(t *testing.T) {
+	if !goversion.VersionAfterOrEqual(runtime.Version(), 1, 14) {
+		// In Go 1.13, 'next' will step into the defer in the runtime
+		// main function, instead of the next line in the main program.
+		t.SkipNow()
+	}
+
+	runTest(t, "panic", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{5},
+			[]onBreakpoint{{
+				execute: func() {
+					handleStop(t, client, 1, 5)
+
+					client.NextRequest(1)
+					client.ExpectNextResponse(t)
+
+					se := client.ExpectStoppedEvent(t)
+
+					if se.Body.ThreadId != 1 || se.Body.Reason != "panic" {
+						t.Errorf("\ngot  %#v\nexpected ThreadId=1 Reason=\"panic\"", se)
+					}
+				},
+				disconnect: true,
+			}})
+	})
+}
+
+func TestFatalThrowBreakpoint(t *testing.T) {
+	runTest(t, "testdeadlock", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{3},
+			[]onBreakpoint{{
+				execute: func() {
+					handleStop(t, client, 1, 3)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+
+					se := client.ExpectStoppedEvent(t)
+					if se.Body.Reason != "fatal error" {
+						t.Errorf("\ngot  %#v\nwant Reason=\"fatal error\"", se)
+					}
+				},
+				disconnect: true,
+			}})
+	})
+}
+
 // handleStop covers the standard sequence of reqeusts issued by
 // a client at a breakpoint or another non-terminal stop event.
 // The details have been tested by other tests,
@@ -1313,11 +1495,11 @@ func runDebugSessionWithBPs(t *testing.T, client *daptest.Client, launchRequest
 	client.ExpectDisconnectResponse(t)
 }
 
-// runDebugSesion is a helper for executing the standard init and shutdown
+// runDebugSession is a helper for executing the standard init and shutdown
 // sequences for a program that does not stop on entry
 // while specifying unique launch criteria via parameters.
-func runDebugSession(t *testing.T, client *daptest.Client, launchRequest func()) {
-	runDebugSessionWithBPs(t, client, launchRequest, "", nil, nil)
+func runDebugSession(t *testing.T, client *daptest.Client, launchRequest func(), source string) {
+	runDebugSessionWithBPs(t, client, launchRequest, source, nil, nil)
 }
 
 func TestLaunchDebugRequest(t *testing.T) {
@@ -1328,7 +1510,7 @@ func TestLaunchDebugRequest(t *testing.T) {
 			// Use the default output directory.
 			client.LaunchRequestWithArgs(map[string]interface{}{
 				"mode": "debug", "program": fixture.Source})
-		})
+		}, fixture.Source)
 	})
 }
 
@@ -1341,7 +1523,7 @@ func TestLaunchTestRequest(t *testing.T) {
 			testdir, _ := filepath.Abs(filepath.Join(fixtures, "buildtest"))
 			client.LaunchRequestWithArgs(map[string]interface{}{
 				"mode": "test", "program": testdir, "output": "__mytestdir"})
-		})
+		}, fixture.Source)
 	})
 }
 
@@ -1355,7 +1537,7 @@ func TestLaunchRequestWithArgs(t *testing.T) {
 			client.LaunchRequestWithArgs(map[string]interface{}{
 				"mode": "exec", "program": fixture.Path,
 				"args": []string{"test", "pass flag"}})
-		})
+		}, fixture.Source)
 	})
 }
 
@@ -1371,7 +1553,7 @@ func TestLaunchRequestWithBuildFlags(t *testing.T) {
 			client.LaunchRequestWithArgs(map[string]interface{}{
 				"mode": "debug", "program": fixture.Source,
 				"buildFlags": "-ldflags '-X main.Hello=World'"})
-		})
+		}, fixture.Source)
 	})
 }