Repository: go-delve/delve
Pull request #2143 (2020-08-21 05:17:26)

service/dap: support next, stepIn and stepOut requests
Updates #1515 


Patch:
diff --git a/pkg/proc/bininfo.go b/pkg/proc/bininfo.go
index 62981abc..1acae9c7 100644
--- a/pkg/proc/bininfo.go
+++ b/pkg/proc/bininfo.go
@@ -108,7 +108,7 @@ type BinaryInfo struct {
 
 var (
 	// ErrCouldNotDetermineRelocation is an error returned when Delve could not determine the base address of a
-	// position independant executable.
+	// position independent executable.
 	ErrCouldNotDetermineRelocation = errors.New("could not determine the base address of a PIE")
 
 	// ErrNoDebugInfoFound is returned when Delve cannot open the debug_info
diff --git a/pkg/proc/fbsdutil/regs.go b/pkg/proc/fbsdutil/regs.go
index cf65a122..94c13030 100644
--- a/pkg/proc/fbsdutil/regs.go
+++ b/pkg/proc/fbsdutil/regs.go
@@ -312,7 +312,7 @@ func (r *AMD64Registers) Get(n int) (uint64, error) {
 	return 0, proc.ErrUnknownRegister
 }
 
-// Copy returns a copy of these registers that is guarenteed not to change.
+// Copy returns a copy of these registers that is guaranteed not to change.
 func (r *AMD64Registers) Copy() (proc.Registers, error) {
 	if r.loadFpRegs != nil {
 		err := r.loadFpRegs(r)
diff --git a/pkg/proc/gdbserial/gdbserver.go b/pkg/proc/gdbserial/gdbserver.go
index 2c7a5be9..acfa2f62 100644
--- a/pkg/proc/gdbserial/gdbserver.go
+++ b/pkg/proc/gdbserial/gdbserver.go
@@ -840,7 +840,7 @@ func (p *gdbProcess) handleThreadSignals(trapthread *gdbThread) (trapthreadOut *
 }
 
 // RequestManualStop will attempt to stop the process
-// without a breakpoint or signal having been recieved.
+// without a breakpoint or signal having been received.
 func (p *gdbProcess) RequestManualStop() error {
 	p.conn.manualStopMutex.Lock()
 	p.manualStopRequested = true
diff --git a/pkg/proc/linutil/regs_amd64_arch.go b/pkg/proc/linutil/regs_amd64_arch.go
index 911a9f44..7083c9cc 100644
--- a/pkg/proc/linutil/regs_amd64_arch.go
+++ b/pkg/proc/linutil/regs_amd64_arch.go
@@ -288,7 +288,7 @@ func (r *AMD64Registers) Get(n int) (uint64, error) {
 	return 0, proc.ErrUnknownRegister
 }
 
-// Copy returns a copy of these registers that is guarenteed not to change.
+// Copy returns a copy of these registers that is guaranteed not to change.
 func (r *AMD64Registers) Copy() (proc.Registers, error) {
 	if r.loadFpRegs != nil {
 		err := r.loadFpRegs(r)
diff --git a/pkg/proc/linutil/regs_arm64_arch.go b/pkg/proc/linutil/regs_arm64_arch.go
index 51ea3df3..a82b0b7d 100644
--- a/pkg/proc/linutil/regs_arm64_arch.go
+++ b/pkg/proc/linutil/regs_arm64_arch.go
@@ -122,7 +122,7 @@ func (r *ARM64Registers) Get(n int) (uint64, error) {
 	return 0, proc.ErrUnknownRegister
 }
 
-// Copy returns a copy of these registers that is guarenteed not to change.
+// Copy returns a copy of these registers that is guaranteed not to change.
 func (r *ARM64Registers) Copy() (proc.Registers, error) {
 	if r.loadFpRegs != nil {
 		err := r.loadFpRegs(r)
diff --git a/pkg/proc/linutil/regs_i386_arch.go b/pkg/proc/linutil/regs_i386_arch.go
index eb8e179a..ed7d6bbe 100644
--- a/pkg/proc/linutil/regs_i386_arch.go
+++ b/pkg/proc/linutil/regs_i386_arch.go
@@ -188,7 +188,7 @@ func (r *I386Registers) Get(n int) (uint64, error) {
 	return 0, proc.ErrUnknownRegister
 }
 
-// Copy returns a copy of these registers that is guarenteed not to change.
+// Copy returns a copy of these registers that is guaranteed not to change.
 func (r *I386Registers) Copy() (proc.Registers, error) {
 	if r.loadFpRegs != nil {
 		err := r.loadFpRegs(r)
diff --git a/pkg/proc/variables.go b/pkg/proc/variables.go
index f98c13cc..85ede40f 100644
--- a/pkg/proc/variables.go
+++ b/pkg/proc/variables.go
@@ -377,7 +377,7 @@ func FindGoroutine(dbp *Target, gid int) (*G, error) {
 		// 3. if the caller asks for gid == 0 and the selected goroutine is
 		//    either 0 or nil.
 		//    Goroutine 0 is special, it either means we have no current goroutine
-		//    (for example, running C code), or that we are running on a speical
+		//    (for example, running C code), or that we are running on a special
 		//    stack (system stack, signal handling stack) and we didn't properly
 		//    detect it.
 		//    Since there could be multiple goroutines '0' running simultaneously
diff --git a/pkg/proc/winutil/regs.go b/pkg/proc/winutil/regs.go
index 3a5acabd..3822e415 100644
--- a/pkg/proc/winutil/regs.go
+++ b/pkg/proc/winutil/regs.go
@@ -322,7 +322,7 @@ func (r *AMD64Registers) Get(n int) (uint64, error) {
 	return 0, proc.ErrUnknownRegister
 }
 
-// Copy returns a copy of these registers that is guarenteed not to change.
+// Copy returns a copy of these registers that is guaranteed not to change.
 func (r *AMD64Registers) Copy() (proc.Registers, error) {
 	var rr AMD64Registers
 	rr = *r
diff --git a/service/dap/server.go b/service/dap/server.go
index 75b38e34..c87e0946 100644
--- a/service/dap/server.go
+++ b/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",
+			}})
 	}
 }


Test patch:
diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go
index a156334c..c49e8f8b 100644
--- a/pkg/proc/proc_test.go
+++ b/pkg/proc/proc_test.go
@@ -2667,7 +2667,7 @@ func TestIssue664(t *testing.T) {
 	})
 }
 
-// Benchmarks (*Processs).Continue + (*Scope).FunctionArguments
+// Benchmarks (*Process).Continue + (*Scope).FunctionArguments
 func BenchmarkTrace(b *testing.B) {
 	withTestProcess("traceperf", b, func(p *proc.Target, fixture protest.Fixture) {
 		setFunctionBreakpoint(p, b, "main.PerfCheck")
diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go
index 31f512f2..e06cddbb 100644
--- a/service/dap/daptest/client.go
+++ b/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)
 }
 
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index c171ab70..55ad100f 100644
--- a/service/dap/server_test.go
+++ b/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")