service/dap: fix goroutine id selection for hardcoded breakpoints (#2748)

* service/dap: fix goroutine id selection for hardcoded breakpoints

Determining the stopped goroutine id on a breakpoint required
checking for breakpoints since some may be tracepoints. However,
there may be goroutines stopped on hardcoded breakpoints with
no breakpoint. We fix this by checking for runtime.breakpoint or
StopReason=proc.StopHardcodedBreakpoint.

_fixtures/goroutinebreak.go

@@ -0,0 +1,26 @@
+package main
+
+import "runtime"
+
+const N = 10
+
+func agoroutine(started chan<- struct{}, done chan<- struct{}, i int) {
+	started <- struct{}{}
+	done <- struct{}{}
+}
+
+func main() {
+	done := make(chan struct{})
+	started := make(chan struct{})
+	for i := 0; i < N; i++ {
+		runtime.Breakpoint()
+		go agoroutine(started, done, i)
+	}
+	for i := 0; i < N; i++ {
+		<-started
+	}
+	runtime.Gosched()
+	for i := 0; i < N; i++ {
+		<-done
+	}
+}

service/dap/server.go

@@ -1819,26 +1819,25 @@ func stoppedGoroutineID(state *api.DebuggerState) (id int) {
 // stoppedOnBreakpointGoroutineID gets the goroutine id of the first goroutine
 // that is stopped on a real breakpoint, starting with the selected goroutine.
 func (s *Session) stoppedOnBreakpointGoroutineID(state *api.DebuggerState) (int, *api.Breakpoint) {
-	// Check if the selected goroutine is stopped on a real breakpoint
-	// since we would prefer to use that one.
-	goid := stoppedGoroutineID(state)
-	if g, _ := s.debugger.FindGoroutine(goid); g != nil && g.Thread != nil {
-		if bp := g.Thread.Breakpoint(); bp != nil && bp.Breakpoint != nil && !bp.Breakpoint.Tracepoint {
-			return goid, api.ConvertBreakpoint(bp.Breakpoint)
-		}
-	}
-
-	// Some of the breakpoints may be log points, choose the goroutine
-	// that is not stopped on a tracepoint.
-	for _, th := range state.Threads {
-		if bp := th.Breakpoint; bp != nil {
-			if !bp.Tracepoint {
-				return th.GoroutineID, bp
-			}
-		}
-	}
+	// Use the first goroutine that is stopped on a breakpoint.
+	gs := s.stoppedGs(state)
+	if len(gs) == 0 {
 		return 0, nil
 	}
+	goid := gs[0]
+	if goid == 0 {
+		return goid, state.CurrentThread.Breakpoint
+	}
+	g, _ := s.debugger.FindGoroutine(goid)
+	if g == nil || g.Thread == nil {
+		return goid, nil
+	}
+	bp := g.Thread.Breakpoint()
+	if bp == nil || bp.Breakpoint == nil {
+		return goid, nil
+	}
+	return goid, api.ConvertBreakpoint(bp.Breakpoint)
+}
 
 // stepUntilStopAndNotify is a wrapper around runUntilStopAndNotify that
 // first switches selected goroutine. allowNextStateChange is
@@ -3539,12 +3538,13 @@ func (s *Session) resumeOnceAndCheckStop(command string, allowNextStateChange ch
 		return state, err
 	}
 
-	foundRealBreakpoint := s.handleLogPoints(state)
+	s.handleLogPoints(state)
+	gsOnBp := s.stoppedGs(state)
 
 	switch s.debugger.StopReason() {
 	case proc.StopBreakpoint, proc.StopManual:
 		// Make sure a real manual stop was requested or a real breakpoint was hit.
-		if foundRealBreakpoint || s.checkHaltRequested() {
+		if len(gsOnBp) > 0 || s.checkHaltRequested() {
 			s.setRunningCmd(false)
 		}
 	default:
@@ -3559,17 +3559,42 @@ func (s *Session) resumeOnceAndCheckStop(command string, allowNextStateChange ch
 	return state, err
 }
 
-func (s *Session) handleLogPoints(state *api.DebuggerState) bool {
-	foundRealBreakpoint := false
+func (s *Session) handleLogPoints(state *api.DebuggerState) {
 	for _, th := range state.Threads {
 		if bp := th.Breakpoint; bp != nil {
-			logged := s.logBreakpointMessage(bp, th.GoroutineID)
-			if !logged {
-				foundRealBreakpoint = true
+			s.logBreakpointMessage(bp, th.GoroutineID)
 		}
 	}
 }
-	return foundRealBreakpoint
+
+func (s *Session) stoppedGs(state *api.DebuggerState) (gs []int) {
+	// Check the current thread first. There may be no selected goroutine.
+	if state.CurrentThread.Breakpoint != nil && !state.CurrentThread.Breakpoint.Tracepoint {
+		gs = append(gs, state.CurrentThread.GoroutineID)
+	}
+	if s.debugger.StopReason() == proc.StopHardcodedBreakpoint {
+		gs = append(gs, stoppedGoroutineID(state))
+	}
+	for _, th := range state.Threads {
+		// Some threads may be stopped on a hardcoded breakpoint.
+		// TODO(suzmue): This is a workaround for detecting hard coded breakpoints,
+		// though this check is likely not sufficient. It would be better to resolve
+		// this in the debugger layer instead.
+		if th.Function.Name() == "runtime.breakpoint" {
+			gs = append(gs, th.GoroutineID)
+			continue
+		}
+		// We already added the current thread if it had a breakpoint.
+		if th.ID == state.CurrentThread.ID {
+			continue
+		}
+		if bp := th.Breakpoint; bp != nil {
+			if !th.Breakpoint.Tracepoint {
+				gs = append(gs, th.GoroutineID)
+			}
+		}
+	}
+	return gs
 }
 
 func (s *Session) logBreakpointMessage(bp *api.Breakpoint, goid int) bool {

service/dap/server_test.go

@@ -3258,33 +3258,46 @@ func TestHaltPreventsAutoResume(t *testing.T) {
 	})
 }
 
-// TestConcurrentBreakpointsLogPoints executes to a breakpoint and then tests
-// that a breakpoint set in the main goroutine is hit the correct number of times
-// and log points set in the children goroutines produce the correct number of
-// output events.
+// TestConcurrentBreakpointsLogPoints tests that a breakpoint set in the main
+// goroutine is hit the correct number of times and log points set in the
+// children goroutines produce the correct number of output events.
 func TestConcurrentBreakpointsLogPoints(t *testing.T) {
 	if runtime.GOOS == "freebsd" {
 		t.SkipNow()
 	}
-	runTest(t, "goroutinestackprog", func(client *daptest.Client, fixture protest.Fixture) {
-		runDebugSessionWithBPs(t, client, "launch",
-			// Launch
-			func() {
-				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+	tests := []struct {
+		name        string
+		fixture     string
+		start       int
+		breakpoints []int
+	}{
+		{
+			name:        "source breakpoints",
+			fixture:     "goroutinestackprog",
+			breakpoints: []int{23},
 		},
-			// Set breakpoints
-			fixture.Source, []int{20},
-			[]onBreakpoint{{
-				// Stop at line 20
-				execute: func() {
-					checkStop(t, client, 1, "main.main", 20)
-					bps := []int{8, 23}
+		{
+			name:    "hardcoded breakpoint",
+			fixture: "goroutinebreak",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			runTest(t, tt.fixture, func(client *daptest.Client, fixture protest.Fixture) {
+				client.InitializeRequest()
+				client.ExpectInitializeResponseAndCapabilities(t)
+
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+				client.ExpectInitializedEvent(t)
+				client.ExpectLaunchResponse(t)
+
+				bps := append([]int{8}, tt.breakpoints...)
 				logMessages := map[int]string{8: "hello"}
 				client.SetBreakpointsRequestWithArgs(fixture.Source, bps, nil, nil, logMessages)
 				client.ExpectSetBreakpointsResponse(t)
 
-					client.ContinueRequest(1)
-					client.ExpectContinueResponse(t)
+				client.ConfigurationDoneRequest()
+				client.ExpectConfigurationDoneResponse(t)
 
 				// There may be up to 1 breakpoint and any number of log points that are
 				// hit concurrently. We should get a stopped event everytime the breakpoint
@@ -3296,7 +3309,6 @@ func TestConcurrentBreakpointsLogPoints(t *testing.T) {
 						if m.Body.Reason != "breakpoint" || !m.Body.AllThreadsStopped || m.Body.ThreadId != 1 {
 							t.Errorf("\ngot  %#v\nwant Reason='breakpoint' AllThreadsStopped=true ThreadId=1", m)
 						}
-							checkStop(t, client, 1, "main.main", 23)
 						seCount++
 						client.ContinueRequest(1)
 					case *dap.OutputEvent:
@@ -3309,11 +3321,13 @@ func TestConcurrentBreakpointsLogPoints(t *testing.T) {
 						t.Fatalf("Unexpected message type: expect StoppedEvent, OutputEvent, or ContinueResponse, got %#v", m)
 					}
 				}
-					client.ExpectTerminatedEvent(t)
-				},
-				disconnect: false,
-			}})
+				// TODO(suzmue): The dap server may identify some false
+				// positives for hard coded breakpoints, so there may still
+				// be more stopped events.
+				client.DisconnectRequestWithKillOption(true)
 			})
+		})
+	}
 }
 
 func TestSetBreakpointWhileRunning(t *testing.T) {
@@ -4272,6 +4286,30 @@ func TestNextAndStep(t *testing.T) {
 	})
 }
 
+func TestHardCodedBreakpoints(t *testing.T) {
+	runTest(t, "consts", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			fixture.Source, []int{28},
+			[]onBreakpoint{{ // Stop at line 28
+				execute: func() {
+					checkStop(t, client, 1, "main.main", 28)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					se := client.ExpectStoppedEvent(t)
+					if se.Body.ThreadId != 1 || se.Body.Reason != "breakpoint" {
+						t.Errorf("\ngot  %#v\nwant ThreadId=1 Reason=\"breakpoint\"", se)
+					}
+				},
+				disconnect: false,
+			}})
+	})
+}
+
 // TestStepInstruction executes to a breakpoint and tests stepping
 // a single instruction
 func TestStepInstruction(t *testing.T) {