service/dap: support next, stepIn and stepOut requests (#2143)

* Issue stopped event on runtime error when continuing

* Support next, stepIn and stepOut

* Refactor stop logic

* Explicitely set AllThreadsContinued

* Make DeepSource happy

* Respond to review comments
Co-authored-by: NPolina Sokolova <polinasok@users.noreply.github.com>

service/dap/daptest/client.go

@@ -93,6 +93,21 @@ func (c *Client) ExpectContinueResponse(t *testing.T) *dap.ContinueResponse {
 	return c.expectReadProtocolMessage(t).(*dap.ContinueResponse)
 }
 
+func (c *Client) ExpectNextResponse(t *testing.T) *dap.NextResponse {
+	t.Helper()
+	return c.expectReadProtocolMessage(t).(*dap.NextResponse)
+}
+
+func (c *Client) ExpectStepInResponse(t *testing.T) *dap.StepInResponse {
+	t.Helper()
+	return c.expectReadProtocolMessage(t).(*dap.StepInResponse)
+}
+
+func (c *Client) ExpectStepOutResponse(t *testing.T) *dap.StepOutResponse {
+	t.Helper()
+	return c.expectReadProtocolMessage(t).(*dap.StepOutResponse)
+}
+
 func (c *Client) ExpectTerminatedEvent(t *testing.T) *dap.TerminatedEvent {
 	t.Helper()
 	return c.expectReadProtocolMessage(t).(*dap.TerminatedEvent)
@@ -132,6 +147,11 @@ func (c *Client) ExpectStoppedEvent(t *testing.T) *dap.StoppedEvent {
 	return c.expectReadProtocolMessage(t).(*dap.StoppedEvent)
 }
 
+func (c *Client) ExpectOutputEvent(t *testing.T) *dap.OutputEvent {
+	t.Helper()
+	return c.expectReadProtocolMessage(t).(*dap.OutputEvent)
+}
+
 func (c *Client) ExpectConfigurationDoneResponse(t *testing.T) *dap.ConfigurationDoneResponse {
 	t.Helper()
 	return c.expectReadProtocolMessage(t).(*dap.ConfigurationDoneResponse)
@@ -344,23 +364,23 @@ func (c *Client) ContinueRequest(thread int) {
 }
 
 // NextRequest sends a 'next' request.
-func (c *Client) NextRequest() {
+func (c *Client) NextRequest(thread int) {
 	request := &dap.NextRequest{Request: *c.newRequest("next")}
-	// TODO(polina): arguments
+	request.Arguments.ThreadId = thread
 	c.send(request)
 }
 
 // StepInRequest sends a 'stepIn' request.
-func (c *Client) StepInRequest() {
+func (c *Client) StepInRequest(thread int) {
 	request := &dap.NextRequest{Request: *c.newRequest("stepIn")}
-	// TODO(polina): arguments
+	request.Arguments.ThreadId = thread
 	c.send(request)
 }
 
 // StepOutRequest sends a 'stepOut' request.
-func (c *Client) StepOutRequest() {
+func (c *Client) StepOutRequest(thread int) {
 	request := &dap.NextRequest{Request: *c.newRequest("stepOut")}
-	// TODO(polina): arguments
+	request.Arguments.ThreadId = thread
 	c.send(request)
 }
 

service/dap/server.go

@@ -257,15 +257,12 @@ func (s *Server) handleRequest(request dap.Message) {
 		s.onContinueRequest(request)
 	case *dap.NextRequest:
 		// Required
-		// TODO: implement this request in V0
 		s.onNextRequest(request)
 	case *dap.StepInRequest:
 		// Required
-		// TODO: implement this request in V0
 		s.onStepInRequest(request)
 	case *dap.StepOutRequest:
 		// Required
-		// TODO: implement this request in V0
 		s.onStepOutRequest(request)
 	case *dap.StepBackRequest:
 		// Optional (capability ‘supportsStepBack’)
@@ -567,13 +564,15 @@ func (s *Server) onConfigurationDoneRequest(request *dap.ConfigurationDoneReques
 	}
 	s.send(&dap.ConfigurationDoneResponse{Response: *newResponse(request.Request)})
 	if !s.args.stopOnEntry {
-		s.doContinue()
+		s.doCommand(api.Continue)
 	}
 }
 
 func (s *Server) onContinueRequest(request *dap.ContinueRequest) {
-	s.send(&dap.ContinueResponse{Response: *newResponse(request.Request)})
-	s.doContinue()
+	s.send(&dap.ContinueResponse{
+		Response: *newResponse(request.Request),
+		Body:     dap.ContinueResponseBody{AllThreadsContinued: true}})
+	s.doCommand(api.Continue)
 }
 
 func (s *Server) onThreadsRequest(request *dap.ThreadsRequest) {
@@ -625,22 +624,31 @@ func (s *Server) onAttachRequest(request *dap.AttachRequest) { // TODO V0
 	s.sendNotYetImplementedErrorResponse(request.Request)
 }
 
-// onNextRequest sends a not-yet-implemented error response.
+// onNextRequest handles 'next' request.
 // This is a mandatory request to support.
-func (s *Server) onNextRequest(request *dap.NextRequest) { // TODO V0
-	s.sendNotYetImplementedErrorResponse(request.Request)
+func (s *Server) onNextRequest(request *dap.NextRequest) {
+	// This ingores threadId argument to match the original vscode-go implementation.
+	// TODO(polina): use SwitchGoroutine to change the current goroutine.
+	s.send(&dap.NextResponse{Response: *newResponse(request.Request)})
+	s.doCommand(api.Next)
 }
 
-// onStepInRequest sends a not-yet-implemented error response.
+// onStepInRequest handles 'stepIn' request
 // This is a mandatory request to support.
-func (s *Server) onStepInRequest(request *dap.StepInRequest) { // TODO V0
-	s.sendNotYetImplementedErrorResponse(request.Request)
+func (s *Server) onStepInRequest(request *dap.StepInRequest) {
+	// This ingores threadId argument to match the original vscode-go implementation.
+	// TODO(polina): use SwitchGoroutine to change the current goroutine.
+	s.send(&dap.StepInResponse{Response: *newResponse(request.Request)})
+	s.doCommand(api.Step)
 }
 
-// onStepOutRequest sends a not-yet-implemented error response.
+// onStepOutRequest handles 'stepOut' request
 // This is a mandatory request to support.
-func (s *Server) onStepOutRequest(request *dap.StepOutRequest) { // TODO V0
-	s.sendNotYetImplementedErrorResponse(request.Request)
+func (s *Server) onStepOutRequest(request *dap.StepOutRequest) {
+	// This ingores threadId argument to match the original vscode-go implementation.
+	// TODO(polina): use SwitchGoroutine to change the current goroutine.
+	s.send(&dap.StepOutResponse{Response: *newResponse(request.Request)})
+	s.doCommand(api.StepOut)
 }
 
 // onPauseRequest sends a not-yet-implemented error response.
@@ -1031,32 +1039,65 @@ func newEvent(event string) *dap.Event {
 	}
 }
 
-func (s *Server) doContinue() {
+const BetterBadAccessError = `invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation]
+Unable to propogate EXC_BAD_ACCESS signal to target process and panic (see https://github.com/go-delve/delve/issues/852)`
+
+// doCommand runs a debugger command until it stops on
+// termination, error, breakpoint, etc, when an appropriate
+// event needs to be sent to the client.
+func (s *Server) doCommand(command string) {
 	if s.debugger == nil {
 		return
 	}
-	state, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Continue})
-	if err != nil {
-		s.log.Error(err)
-		switch err.(type) {
-		case proc.ErrProcessExited:
-			e := &dap.TerminatedEvent{Event: *newEvent("terminated")}
-			s.send(e)
-		default:
-		}
+
+	state, err := s.debugger.Command(&api.DebuggerCommand{Name: command})
+	if _, isexited := err.(proc.ErrProcessExited); isexited || err == nil && state.Exited {
+		e := &dap.TerminatedEvent{Event: *newEvent("terminated")}
+		s.send(e)
 		return
 	}
+
 	s.stackFrameHandles.reset()
 	s.variableHandles.reset()
-	if state.Exited {
-		e := &dap.TerminatedEvent{Event: *newEvent("terminated")}
-		s.send(e)
+
+	stopped := &dap.StoppedEvent{Event: *newEvent("stopped")}
+	stopped.Body.AllThreadsStopped = true
+
+	if err == nil {
+		stopped.Body.ThreadId = state.SelectedGoroutine.ID
+		switch command {
+		case api.Next, api.Step, api.StepOut:
+			stopped.Body.Reason = "step"
+		default:
+			stopped.Body.Reason = "breakpoint"
+		}
+		s.send(stopped)
 	} else {
-		e := &dap.StoppedEvent{Event: *newEvent("stopped")}
-		// TODO(polina): differentiate between breakpoint and pause on halt.
-		e.Body.Reason = "breakpoint"
-		e.Body.AllThreadsStopped = true
-		e.Body.ThreadId = state.SelectedGoroutine.ID
-		s.send(e)
+		s.log.Error("runtime error: ", err)
+		stopped.Body.Reason = "runtime error"
+		stopped.Body.Text = err.Error()
+		// Special case in the spirit of https://github.com/microsoft/vscode-go/issues/1903
+		if stopped.Body.Text == "bad access" {
+			stopped.Body.Text = BetterBadAccessError
+		}
+		state, err := s.debugger.State( /*nowait*/ true)
+		if err == nil {
+			stopped.Body.ThreadId = state.CurrentThread.GoroutineID
+		}
+		s.send(stopped)
+
+		// TODO(polina): according to the spec, the extra 'text' is supposed to show up in the UI (e.g. on hover),
+		// but so far I am unable to get this to work in vscode - see https://github.com/microsoft/vscode/issues/104475.
+		// Options to explore:
+		//   - supporting ExceptionInfo request
+		//   - virtual variable scope for Exception that shows the message (details here: https://github.com/microsoft/vscode/issues/3101)
+		// In the meantime, provide the extra details by outputing an error message.
+		// {"body":{"category":"stdout","output":"API server listening at: 127.0.0.1:11973\n"}}
+		s.send(&dap.OutputEvent{
+			Event: *newEvent("output"),
+			Body: dap.OutputEventBody{
+				Output:   fmt.Sprintf("ERROR: %s\n", stopped.Body.Text),
+				Category: "stderr",
+			}})
 	}
 }

service/dap/server_test.go

@@ -27,11 +27,14 @@ const stopOnEntry bool = true
 const hasChildren bool = true
 const noChildren bool = false
 
+var testBackend string
+
 func TestMain(m *testing.M) {
 	var logOutput string
 	flag.StringVar(&logOutput, "log-output", "", "configures log output")
 	flag.Parse()
 	logflags.Setup(logOutput != "", logOutput, "")
+	protest.DefaultTestBackend(&testBackend)
 	os.Exit(protest.RunTestsWithFixtures(m))
 }
 
@@ -187,8 +190,8 @@ func TestStopOnEntry(t *testing.T) {
 		// 10 >> continue, << continue, << terminated
 		client.ContinueRequest(1)
 		contResp := client.ExpectContinueResponse(t)
-		if contResp.Seq != 0 || contResp.RequestSeq != 10 {
-			t.Errorf("\ngot %#v\nwant Seq=0, RequestSeq=10", contResp)
+		if contResp.Seq != 0 || contResp.RequestSeq != 10 || !contResp.Body.AllThreadsContinued {
+			t.Errorf("\ngot %#v\nwant Seq=0, RequestSeq=10 Body.AllThreadsContinued=true", contResp)
 		}
 		termEvent := client.ExpectTerminatedEvent(t)
 		if termEvent.Seq != 0 {
@@ -302,7 +305,6 @@ func TestSetBreakpoint(t *testing.T) {
 		if len(tResp.Body.Threads) < 2 { // 1 main + runtime
 			t.Errorf("\ngot  %#v\nwant len(Threads)>1", tResp.Body.Threads)
 		}
-		// TODO(polina): can we reliably test for these values?
 		wantMain := dap.Thread{Id: 1, Name: "main.Increment"}
 		wantRuntime := dap.Thread{Id: 2, Name: "runtime.gopark"}
 		for _, got := range tResp.Body.Threads {
@@ -353,7 +355,10 @@ func TestSetBreakpoint(t *testing.T) {
 		expectChildren(t, locals, "Locals", 0)
 
 		client.ContinueRequest(1)
-		client.ExpectContinueResponse(t)
+		ctResp := client.ExpectContinueResponse(t)
+		if !ctResp.Body.AllThreadsContinued {
+			t.Errorf("\ngot  %#v\nwant AllThreadsContinued=true", ctResp.Body)
+		}
 		// "Continue" is triggered after the response is sent
 
 		client.ExpectTerminatedEvent(t)
@@ -1040,6 +1045,126 @@ func TestLaunchRequestWithStackTraceDepth(t *testing.T) {
 	})
 }
 
+func TestNextAndStep(t *testing.T) {
+	runTest(t, "testinline", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{11},
+			[]onBreakpoint{{ // Stop at line 11
+				execute: func() {
+					handleStop(t, client, 1, 11)
+
+					expectStop := func(line int) {
+						t.Helper()
+						se := client.ExpectStoppedEvent(t)
+						if se.Body.Reason != "step" || se.Body.ThreadId != 1 || !se.Body.AllThreadsStopped {
+							t.Errorf("got %#v, want Reason=\"step\", ThreadId=1, AllThreadsStopped=true", se)
+						}
+						handleStop(t, client, 1, line)
+					}
+
+					client.StepOutRequest(1)
+					client.ExpectStepOutResponse(t)
+					expectStop(18)
+
+					client.NextRequest(1)
+					client.ExpectNextResponse(t)
+					expectStop(19)
+
+					client.StepInRequest(1)
+					client.ExpectStepInResponse(t)
+					expectStop(5)
+
+					client.NextRequest(-10000 /*this is ignored*/)
+					client.ExpectNextResponse(t)
+					expectStop(6)
+				},
+				disconnect: false,
+			}})
+	})
+}
+
+func TestBadAccess(t *testing.T) {
+	if runtime.GOOS != "darwin" || testBackend != "lldb" {
+		t.Skip("not applicable")
+	}
+	runTest(t, "issue2078", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{4},
+			[]onBreakpoint{{ // Stop at line 4
+				execute: func() {
+					handleStop(t, client, 1, 4)
+
+					expectStoppedOnError := func(errorPrefix string) {
+						t.Helper()
+						se := client.ExpectStoppedEvent(t)
+						if se.Body.ThreadId != 1 || se.Body.Reason != "runtime error" || !strings.HasPrefix(se.Body.Text, errorPrefix) {
+							t.Errorf("\ngot  %#v\nwant ThreadId=1 Reason=\"runtime error\" Text=\"%s\"", se, errorPrefix)
+						}
+						oe := client.ExpectOutputEvent(t)
+						if oe.Body.Category != "stderr" || !strings.HasPrefix(oe.Body.Output, "ERROR: "+errorPrefix) {
+							t.Errorf("\ngot  %#v\nwant Category=\"stderr\" Output=\"%s ...\"", oe, errorPrefix)
+						}
+					}
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					expectStoppedOnError("invalid memory address or nil pointer dereference")
+
+					client.NextRequest(1)
+					client.ExpectNextResponse(t)
+					expectStoppedOnError("invalid memory address or nil pointer dereference")
+
+					client.NextRequest(1)
+					client.ExpectNextResponse(t)
+					expectStoppedOnError("next while nexting")
+
+					client.StepInRequest(1)
+					client.ExpectStepInResponse(t)
+					expectStoppedOnError("next while nexting")
+
+					client.StepOutRequest(1)
+					client.ExpectStepOutResponse(t)
+					expectStoppedOnError("next while nexting")
+				},
+				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,
+// so this is just a sanity check.
+func handleStop(t *testing.T, client *daptest.Client, thread, line int) {
+	t.Helper()
+	client.ThreadsRequest()
+	client.ExpectThreadsResponse(t)
+
+	client.StackTraceRequest(thread, 0, 20)
+	st := client.ExpectStackTraceResponse(t)
+	if len(st.Body.StackFrames) < 1 || st.Body.StackFrames[0].Line != line {
+		t.Errorf("\ngot  %#v\nwant Line=%d", st, line)
+	}
+
+	client.ScopesRequest(1000)
+	client.ExpectScopesResponse(t)
+
+	client.VariablesRequest(1000) // Arguments
+	client.ExpectVariablesResponse(t)
+	client.VariablesRequest(1001) // Locals
+	client.ExpectVariablesResponse(t)
+}
+
 // onBreakpoint specifies what the test harness should simulate at
 // a stopped breakpoint. First execute() is to be called to test
 // specified editor-driven or user-driven requests. Then if
@@ -1228,15 +1353,6 @@ func TestRequiredNotYetImplementedResponses(t *testing.T) {
 		client.AttachRequest()
 		expectNotYetImplemented("attach")
 
-		client.NextRequest()
-		expectNotYetImplemented("next")
-
-		client.StepInRequest()
-		expectNotYetImplemented("stepIn")
-
-		client.StepOutRequest()
-		expectNotYetImplemented("stepOut")
-
 		client.PauseRequest()
 		expectNotYetImplemented("pause")