From f8deab85224b61b2dab6a0cec4ea2ac9234761dc Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Fri, 29 Oct 2021 22:40:16 -0400 Subject: [PATCH] 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 | 26 +++++++ service/dap/server.go | 75 +++++++++++++------- service/dap/server_test.go | 132 +++++++++++++++++++++++------------- 3 files changed, 161 insertions(+), 72 deletions(-) create mode 100644 _fixtures/goroutinebreak.go diff --git a/_fixtures/goroutinebreak.go b/_fixtures/goroutinebreak.go new file mode 100644 index 00000000..438d1ed8 --- /dev/null +++ b/_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 + } +} diff --git a/service/dap/server.go b/service/dap/server.go index 2b8d8ced..3bf160fa 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -1819,25 +1819,24 @@ 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) - } + // Use the first goroutine that is stopped on a breakpoint. + gs := s.stoppedGs(state) + if len(gs) == 0 { + return 0, nil } - - // 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 - } - } + goid := gs[0] + if goid == 0 { + return goid, state.CurrentThread.Breakpoint } - return 0, nil + 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 @@ -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) + } + } +} + +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 foundRealBreakpoint + return gs } func (s *Session) logBreakpointMessage(bp *api.Breakpoint, goid int) bool { diff --git a/service/dap/server_test.go b/service/dap/server_test.go index b8b20fd5..d1d8e508 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -3258,62 +3258,76 @@ 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() { + tests := []struct { + name string + fixture string + start int + breakpoints []int + }{ + { + name: "source breakpoints", + fixture: "goroutinestackprog", + breakpoints: []int{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) - }, - // Set breakpoints - fixture.Source, []int{20}, - []onBreakpoint{{ - // Stop at line 20 - execute: func() { - checkStop(t, client, 1, "main.main", 20) - bps := []int{8, 23} - logMessages := map[int]string{8: "hello"} - client.SetBreakpointsRequestWithArgs(fixture.Source, bps, nil, nil, logMessages) - client.ExpectSetBreakpointsResponse(t) + client.ExpectInitializedEvent(t) + client.ExpectLaunchResponse(t) - client.ContinueRequest(1) - client.ExpectContinueResponse(t) + bps := append([]int{8}, tt.breakpoints...) + logMessages := map[int]string{8: "hello"} + client.SetBreakpointsRequestWithArgs(fixture.Source, bps, nil, nil, logMessages) + client.ExpectSetBreakpointsResponse(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 - // is hit and an output event for each log point hit. - var oeCount, seCount int - for oeCount < 10 || seCount < 10 { - switch m := client.ExpectMessage(t).(type) { - case *dap.StoppedEvent: - 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: - checkLogMessage(t, m, -1, "hello", fixture.Source, 8) - oeCount++ - case *dap.ContinueResponse: - case *dap.TerminatedEvent: - t.Fatalf("\nexpected 10 output events and 10 stopped events, got %d output events and %d stopped events", oeCount, seCount) - default: - t.Fatalf("Unexpected message type: expect StoppedEvent, OutputEvent, or ContinueResponse, got %#v", m) + 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 + // is hit and an output event for each log point hit. + var oeCount, seCount int + for oeCount < 10 || seCount < 10 { + switch m := client.ExpectMessage(t).(type) { + case *dap.StoppedEvent: + if m.Body.Reason != "breakpoint" || !m.Body.AllThreadsStopped || m.Body.ThreadId != 1 { + t.Errorf("\ngot %#v\nwant Reason='breakpoint' AllThreadsStopped=true ThreadId=1", m) } + seCount++ + client.ContinueRequest(1) + case *dap.OutputEvent: + checkLogMessage(t, m, -1, "hello", fixture.Source, 8) + oeCount++ + case *dap.ContinueResponse: + case *dap.TerminatedEvent: + t.Fatalf("\nexpected 10 output events and 10 stopped events, got %d output events and %d stopped events", oeCount, seCount) + default: + 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) {