service/dap: switch goroutines when stepping (#2403)

* service/dap: switch goroutine before stepping

The correct goroutine needs to be selected when stepping in order
for the step to execute to the correct location.

* handle next in progress while stepping

* Add tests for steps when switching goroutine

* remove nextInProgress handling

* add new step out test and review debug state check

* update text of stopped event and set goroutine id

service/dap/server.go

@@ -863,28 +863,44 @@ func (s *Server) onAttachRequest(request *dap.AttachRequest) {
 // onNextRequest handles 'next' request.
 // This is a mandatory request to support.
 func (s *Server) onNextRequest(request *dap.NextRequest) {
-	// This ignores 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)
+	s.doStepCommand(api.Next, request.Arguments.ThreadId)
 }
 
 // onStepInRequest handles 'stepIn' request
 // This is a mandatory request to support.
 func (s *Server) onStepInRequest(request *dap.StepInRequest) {
-	// This ignores 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)
+	s.doStepCommand(api.Step, request.Arguments.ThreadId)
 }
 
 // onStepOutRequest handles 'stepOut' request
 // This is a mandatory request to support.
 func (s *Server) onStepOutRequest(request *dap.StepOutRequest) {
-	// This ignores 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)
+	s.doStepCommand(api.StepOut, request.Arguments.ThreadId)
+}
+
+func (s *Server) doStepCommand(command string, threadId int) {
+	// Use SwitchGoroutine to change the current goroutine.
+	state, err := s.debugger.Command(&api.DebuggerCommand{Name: api.SwitchGoroutine, GoroutineID: threadId}, nil)
+	if err != nil {
+		s.log.Errorf("Error switching goroutines while stepping: %e", err)
+		// If we encounter an error, we will have to send a stopped event
+		// since we already sent the step response.
+		stopped := &dap.StoppedEvent{Event: *newEvent("stopped")}
+		stopped.Body.AllThreadsStopped = true
+		if state.SelectedGoroutine != nil {
+			stopped.Body.ThreadId = state.SelectedGoroutine.ID
+		} else if state.CurrentThread != nil {
+			stopped.Body.ThreadId = state.CurrentThread.GoroutineID
+		}
+		stopped.Body.Reason = "error"
+		stopped.Body.Text = err.Error()
+		s.send(stopped)
+		return
+	}
+	s.doCommand(command)
 }
 
 // onPauseRequest sends a not-yet-implemented error response.

service/dap/server_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"math/rand"
 	"net"
 	"os"
 	"os/exec"
@@ -2068,6 +2069,181 @@ func TestNextAndStep(t *testing.T) {
 	})
 }
 
+func TestNextParked(t *testing.T) {
+	if runtime.GOOS == "freebsd" {
+		t.SkipNow()
+	}
+	runTest(t, "parallel_next", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{15},
+			[]onBreakpoint{{ // Stop at line 15
+				execute: func() {
+					goroutineId := testStepParkedHelper(t, client, fixture)
+
+					client.NextRequest(goroutineId)
+					client.ExpectNextResponse(t)
+
+					se := client.ExpectStoppedEvent(t)
+					if se.Body.ThreadId != goroutineId {
+						t.Fatalf("Next did not continue on the selected goroutine, expected %d got %d", goroutineId, se.Body.ThreadId)
+					}
+				},
+				disconnect: false,
+			}})
+	})
+}
+
+func TestStepInParked(t *testing.T) {
+	if runtime.GOOS == "freebsd" {
+		t.SkipNow()
+	}
+	runTest(t, "parallel_next", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{15},
+			[]onBreakpoint{{ // Stop at line 15
+				execute: func() {
+					goroutineId := testStepParkedHelper(t, client, fixture)
+
+					client.StepInRequest(goroutineId)
+					client.ExpectStepInResponse(t)
+
+					se := client.ExpectStoppedEvent(t)
+					if se.Body.ThreadId != goroutineId {
+						t.Fatalf("StepIn did not continue on the selected goroutine, expected %d got %d", goroutineId, se.Body.ThreadId)
+					}
+				},
+				disconnect: false,
+			}})
+	})
+}
+
+func testStepParkedHelper(t *testing.T, client *daptest.Client, fixture protest.Fixture) int {
+	t.Helper()
+	// Set a breakpoint at main.sayHi
+	client.SetBreakpointsRequest(fixture.Source, []int{8})
+	client.ExpectSetBreakpointsResponse(t)
+
+	var goroutineId = -1
+	for goroutineId < 0 {
+		client.ContinueRequest(1)
+		client.ExpectContinueResponse(t)
+
+		se := client.ExpectStoppedEvent(t)
+
+		client.ThreadsRequest()
+		threads := client.ExpectThreadsResponse(t)
+
+		// Search for a parked goroutine that we know for sure will have to be
+		// resumed before the program can exit. This is a parked goroutine that:
+		// 1. is executing main.sayhi
+		// 2. hasn't called wg.Done yet
+		// 3. is not the currently selected goroutine
+		for _, g := range threads.Body.Threads {
+			// We do not need to check the thread that the program
+			// is currently stopped on.
+			if g.Id == se.Body.ThreadId {
+				continue
+			}
+			client.StackTraceRequest(g.Id, 0, 5)
+			frames := client.ExpectStackTraceResponse(t)
+			for _, frame := range frames.Body.StackFrames {
+				// line 11 is the line where wg.Done is called
+				if frame.Name == "main.sayhi" && frame.Line < 11 {
+					goroutineId = g.Id
+					break
+				}
+			}
+			if goroutineId >= 0 {
+				break
+			}
+		}
+	}
+
+	// Clear all breakpoints.
+	client.SetBreakpointsRequest(fixture.Source, []int{})
+	client.ExpectSetBreakpointsResponse(t)
+
+	return goroutineId
+}
+
+func TestStepOutPreservesGoroutine(t *testing.T) {
+	// Checks that StepOut preserves the currently selected goroutine.
+	if runtime.GOOS == "freebsd" {
+		t.SkipNow()
+	}
+	rand.Seed(time.Now().Unix())
+	runTest(t, "issue2113", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{25},
+			[]onBreakpoint{{ // Stop at line 25
+				execute: func() {
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+
+					// The program contains runtime.Breakpoint()
+					se := client.ExpectStoppedEvent(t)
+
+					client.ThreadsRequest()
+					gs := client.ExpectThreadsResponse(t)
+
+					candg := []int{}
+					bestg := []int{}
+					for _, g := range gs.Body.Threads {
+						// We do not need to check the thread that the program
+						// is currently stopped on.
+						if g.Id == se.Body.ThreadId {
+							continue
+						}
+
+						client.StackTraceRequest(g.Id, 0, 20)
+						frames := client.ExpectStackTraceResponse(t)
+						for _, frame := range frames.Body.StackFrames {
+							if frame.Name == "main.coroutine" {
+								candg = append(candg, g.Id)
+								if strings.HasPrefix(frames.Body.StackFrames[0].Name, "runtime.") {
+									bestg = append(bestg, g.Id)
+								}
+								break
+							}
+						}
+					}
+					var goroutineId int
+					if len(bestg) > 0 {
+						goroutineId = bestg[rand.Intn(len(bestg))]
+						t.Logf("selected goroutine %d (best)\n", goroutineId)
+					} else {
+						goroutineId = candg[rand.Intn(len(candg))]
+						t.Logf("selected goroutine %d\n", goroutineId)
+
+					}
+					client.StepOutRequest(goroutineId)
+					client.ExpectStepOutResponse(t)
+
+					se = client.ExpectStoppedEvent(t)
+					if se.Body.ThreadId != goroutineId {
+						t.Fatalf("StepIn did not continue on the selected goroutine, expected %d got %d", goroutineId, se.Body.ThreadId)
+					}
+				},
+				disconnect: false,
+			}})
+	})
+}
+
 func TestBadAccess(t *testing.T) {
 	if runtime.GOOS != "darwin" || testBackend != "lldb" {
 		t.Skip("not applicable")