service/dap: support setting breakpoints while running (#2472)

* service/dap: support setting breakpoints while running

* Review comments, faster test

* Fix comments

* Address review comments

* Do not continue automatically

* Add TODO to resume exeuction

* Handle async test messages in either order

Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>

service/dap/server.go

@@ -399,8 +399,7 @@ func (s *Server) handleRequest(request dap.Message) {
 	// We have a couple of options for handling these without blocking
 	// the request loop indefinitely when we are in running state.
 	// --1-- Return a dummy response or an error right away.
-	// --2-- Halt execution, process the request, resume execution.
+	// --2-- Halt execution, process the request, maybe resume execution.
-	// TODO(polina): do this for setting breakpoints
 	// --3-- Handle such requests asynchronously and let them block until
 	// the process stops or terminates (e.g. using a channel and a single
 	// goroutine to preserve the order). This might not be appropriate
@@ -427,6 +426,28 @@ func (s *Server) handleRequest(request dap.Message) {
 				Body:     dap.ThreadsResponseBody{Threads: []dap.Thread{{Id: -1, Name: "Current"}}},
 			}
 			s.send(response)
+		case *dap.SetBreakpointsRequest:
+			s.log.Debug("halting execution to set breakpoints")
+			_, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil)
+			if err != nil {
+				s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error())
+				return
+			}
+			s.onSetBreakpointsRequest(request)
+			// TODO(polina): consider resuming execution here automatically after suppressing
+			// a stop event when an operation in doRunCommand returns. In case that operation
+			// was already stopping for a different reason, we would need to examine the state
+			// that is returned to determine if this halt was the cause of the stop or not.
+			// We should stop with an event and not resume if one of the following is true:
+			// - StopReason is anything but manual
+			// - Any thread has a breakpoint or CallReturn set
+			// - NextInProgress is false and the last command sent by the user was: next,
+			//   step, stepOut, reverseNext, reverseStep or reverseStepOut
+			// Otherwise, we can skip the stop event and resume the temporarily
+			// interrupted process execution with api.DirectionCongruentContinue.
+			// For this to apply in cases other than api.Continue, we would also need to
+			// introduce a new version of halt that skips ClearInternalBreakpoints
+			// in proc.(*Target).Continue, leaving NextInProgress as true.
 		default:
 			r := request.(dap.RequestMessage).GetRequest()
 			s.sendErrorResponse(*r, DebuggeeIsRunning, fmt.Sprintf("Unable to process `%s`", r.Command), "debuggee is running")
@@ -1065,7 +1086,7 @@ func (s *Server) onConfigurationDoneRequest(request *dap.ConfigurationDoneReques
 	}
 	s.send(&dap.ConfigurationDoneResponse{Response: *newResponse(request.Request)})
 	if !s.args.stopOnEntry {
-		s.doCommand(api.Continue, asyncSetupDone)
+		s.doRunCommand(api.Continue, asyncSetupDone)
 	}
 }
 
@@ -1075,7 +1096,7 @@ func (s *Server) onContinueRequest(request *dap.ContinueRequest, asyncSetupDone
 	s.send(&dap.ContinueResponse{
 		Response: *newResponse(request.Request),
 		Body:     dap.ContinueResponseBody{AllThreadsContinued: true}})
-	s.doCommand(api.Continue, asyncSetupDone)
+	s.doRunCommand(api.Continue, asyncSetupDone)
 }
 
 func fnName(loc *proc.Location) string {
@@ -1221,7 +1242,7 @@ func stoppedGoroutineID(state *api.DebuggerState) (id int) {
 	return id
 }
 
-// doStepCommand is a wrapper around doCommand that
+// doStepCommand is a wrapper around doRunCommand that
 // first switches selected goroutine. asyncSetupDone is
 // a channel that will be closed to signal that an
 // asynchornous command has completed setup or was interrupted
@@ -1245,7 +1266,7 @@ func (s *Server) doStepCommand(command string, threadId int, asyncSetupDone chan
 		s.send(stopped)
 		return
 	}
-	s.doCommand(command, asyncSetupDone)
+	s.doRunCommand(command, asyncSetupDone)
 }
 
 // onPauseRequest sends a not-yet-implemented error response.
@@ -1958,14 +1979,14 @@ func (s *Server) resetHandlesForStoppedEvent() {
 	s.variableHandles.reset()
 }
 
-// doCommand runs a debugger command until it stops on
+// doRunCommand runs a debugger command until it stops on
 // termination, error, breakpoint, etc, when an appropriate
 // event needs to be sent to the client. asyncSetupDone is
 // a channel that will be closed to signal that an
 // asynchornous command has completed setup or was interrupted
 // due to an error, so the server is ready to receive new requests.
-func (s *Server) doCommand(command string, asyncSetupDone chan struct{}) {
+func (s *Server) doRunCommand(command string, asyncSetupDone chan struct{}) {
-	// TODO(polina): it appears that debugger.Command doesn't close
+	// TODO(polina): it appears that debugger.Command doesn't always close
 	// asyncSetupDone (e.g. when having an error next while nexting).
 	// So we should always close it ourselves just in case.
 	defer s.asyncCommandDone(asyncSetupDone)
@@ -1975,6 +1996,9 @@ func (s *Server) doCommand(command string, asyncSetupDone chan struct{}) {
 		return
 	}
 
+	stopReason := s.debugger.StopReason()
+	s.log.Debugf("%q command stopped - reason %q", command, stopReason)
+
 	s.resetHandlesForStoppedEvent()
 	stopped := &dap.StoppedEvent{Event: *newEvent("stopped")}
 	stopped.Body.AllThreadsStopped = true
@@ -1982,9 +2006,7 @@ func (s *Server) doCommand(command string, asyncSetupDone chan struct{}) {
 	if err == nil {
 		stopped.Body.ThreadId = stoppedGoroutineID(state)
 
-		sr := s.debugger.StopReason()
+		switch stopReason {
-		s.log.Debugf("%q command stopped - reason %q", command, sr)
-		switch sr {
 		case proc.StopNextFinished:
 			stopped.Body.Reason = "step"
 		case proc.StopManual: // triggered by halt

service/dap/server_test.go

@@ -1778,6 +1778,32 @@ func TestLaunchRequestWithStackTraceDepth(t *testing.T) {
 	})
 }
 
+type Breakpoint struct {
+	line      int
+	path      string
+	verified  bool
+	msgPrefix string
+}
+
+func expectSetBreakpointsResponse(t *testing.T, client *daptest.Client, bps []Breakpoint) {
+	t.Helper()
+	checkSetBreakpointsResponse(t, client, bps, client.ExpectSetBreakpointsResponse(t))
+}
+
+func checkSetBreakpointsResponse(t *testing.T, client *daptest.Client, bps []Breakpoint, got *dap.SetBreakpointsResponse) {
+	t.Helper()
+	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 || bp.Source.Path != bps[i].path ||
+			!strings.HasPrefix(bp.Message, bps[i].msgPrefix) {
+			t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i])
+		}
+	}
+}
+
 // TestSetBreakpoint executes to a breakpoint and tests different
 // configurations of setBreakpoint requests.
 func TestSetBreakpoint(t *testing.T) {
@@ -1793,34 +1819,13 @@ func TestSetBreakpoint(t *testing.T) {
 				execute: func() {
 					handleStop(t, client, 1, "main.main", 16)
 
-					type Breakpoint struct {
-						line      int
-						path      string
-						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 || bp.Source.Path != bps[i].path ||
-								!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, fixture.Source, true, ""}, {18, fixture.Source, true, ""}})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{17, fixture.Source, true, ""}, {18, fixture.Source, true, ""}})
 
 					// Clear 17, reset 18
 					client.SetBreakpointsRequest(fixture.Source, []int{18})
-					expectSetBreakpointsResponse([]Breakpoint{{18, fixture.Source, true, ""}})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{18, fixture.Source, true, ""}})
 
 					// Skip 17, continue to 18
 					client.ContinueRequest(1)
@@ -1830,7 +1835,7 @@ func TestSetBreakpoint(t *testing.T) {
 
 					// Set another breakpoint inside the loop in loop(), twice to trigger error
 					client.SetBreakpointsRequest(fixture.Source, []int{8, 8})
-					expectSetBreakpointsResponse([]Breakpoint{{8, fixture.Source, true, ""}, {8, "", false, "Breakpoint exists"}})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}, {8, "", false, "Breakpoint exists"}})
 
 					// Continue into the loop
 					client.ContinueRequest(1)
@@ -1843,7 +1848,7 @@ func TestSetBreakpoint(t *testing.T) {
 
 					// Edit the breakpoint to add a condition
 					client.SetConditionalBreakpointsRequest(fixture.Source, []int{8}, map[int]string{8: "i == 3"})
-					expectSetBreakpointsResponse([]Breakpoint{{8, fixture.Source, true, ""}})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}})
 
 					// Continue until condition is hit
 					client.ContinueRequest(1)
@@ -1856,7 +1861,7 @@ func TestSetBreakpoint(t *testing.T) {
 
 					// Edit the breakpoint to remove a condition
 					client.SetConditionalBreakpointsRequest(fixture.Source, []int{8}, map[int]string{8: ""})
-					expectSetBreakpointsResponse([]Breakpoint{{8, fixture.Source, true, ""}})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}})
 
 					// Continue for one more loop iteration
 					client.ContinueRequest(1)
@@ -1869,7 +1874,7 @@ func TestSetBreakpoint(t *testing.T) {
 
 					// 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
+					expectSetBreakpointsResponse(t, client, []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,
@@ -1877,6 +1882,78 @@ func TestSetBreakpoint(t *testing.T) {
 	})
 }
 
+func expectSetBreakpointsResponseAndStoppedEvent(t *testing.T, client *daptest.Client) (se *dap.StoppedEvent, br *dap.SetBreakpointsResponse) {
+	for i := 0; i < 2; i++ {
+		switch m := client.ExpectMessage(t).(type) {
+		case *dap.StoppedEvent:
+			se = m
+		case *dap.SetBreakpointsResponse:
+			br = m
+		default:
+			t.Fatalf("Unexpected message type: expect StoppedEvent or SetBreakpointsResponse, got %#v", m)
+		}
+	}
+	if se == nil || br == nil {
+		t.Fatal("Expected StoppedEvent and SetBreakpointsResponse")
+	}
+	return se, br
+}
+
+func TestSetBreakpointWhileRunning(t *testing.T) {
+	runTest(t, "integrationprog", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{16},
+			[]onBreakpoint{{
+				execute: func() {
+					// The program loops 3 times over lines 14-15-8-9-10-16
+					handleStop(t, client, 1, "main.main", 16) // Line that sleeps for 1 second
+
+					// We can set breakpoints while nexting
+					client.NextRequest(1)
+					client.ExpectNextResponse(t)
+					client.SetBreakpointsRequest(fixture.Source, []int{15}) // [16,] => [15,]
+					se, br := expectSetBreakpointsResponseAndStoppedEvent(t, client)
+					if se.Body.Reason != "pause" || !se.Body.AllThreadsStopped || se.Body.ThreadId != 0 && se.Body.ThreadId != 1 {
+						t.Errorf("\ngot  %#v\nwant Reason='pause' AllThreadsStopped=true ThreadId=0/1", se)
+					}
+					checkSetBreakpointsResponse(t, client, []Breakpoint{{15, fixture.Source, true, ""}}, br)
+					// Halt cancelled next, so if we continue we will not stop at line 14.
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					se = client.ExpectStoppedEvent(t)
+					if se.Body.Reason != "breakpoint" || !se.Body.AllThreadsStopped || se.Body.ThreadId != 1 {
+						t.Errorf("\ngot  %#v\nwant Reason='breakpoint' AllThreadsStopped=true ThreadId=1", se)
+					}
+					handleStop(t, client, 1, "main.main", 15)
+
+					// We can set breakpoints while continuing
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.SetBreakpointsRequest(fixture.Source, []int{9}) // [15,] => [9,]
+					se, br = expectSetBreakpointsResponseAndStoppedEvent(t, client)
+					if se.Body.Reason != "pause" || !se.Body.AllThreadsStopped || se.Body.ThreadId != 0 && se.Body.ThreadId != 1 {
+						t.Errorf("\ngot  %#v\nwant Reason='pause' AllThreadsStopped=true ThreadId=0/1", se)
+					}
+					checkSetBreakpointsResponse(t, client, []Breakpoint{{9, fixture.Source, true, ""}}, br)
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					se = client.ExpectStoppedEvent(t)
+					if se.Body.Reason != "breakpoint" || !se.Body.AllThreadsStopped || se.Body.ThreadId != 1 {
+						t.Errorf("\ngot  %#v\nwant Reason='breakpoint' AllThreadsStopped=true ThreadId=1", se)
+					}
+					handleStop(t, client, 1, "main.sayhi", 9)
+
+				},
+				disconnect: true,
+			}})
+	})
+}
+
 // TestLaunchSubstitutePath sets a breakpoint using a path
 // that does not exist and expects the substitutePath attribute
 // in the launch configuration to take care of the mapping.