From 11cf6e689f2bf36192c02c04ab9e8852143b722c Mon Sep 17 00:00:00 2001 From: polinasok <51177946+polinasok@users.noreply.github.com> Date: Mon, 17 May 2021 09:17:00 -0700 Subject: [PATCH] service/dap: support setting breakpoints while running (#2472) * service/dap: support setting breakpoints while running * Review comments, faster test * Fix comments * Address review comments * Do not continue automatically * Add TODO to resume exeuction * Handle async test messages in either order Co-authored-by: Polina Sokolova --- service/dap/server.go | 46 +++++++++---- service/dap/server_test.go | 131 +++++++++++++++++++++++++++++-------- 2 files changed, 138 insertions(+), 39 deletions(-) diff --git a/service/dap/server.go b/service/dap/server.go index d091a36a..68c983ab 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -399,8 +399,7 @@ func (s *Server) handleRequest(request dap.Message) { // We have a couple of options for handling these without blocking // the request loop indefinitely when we are in running state. // --1-- Return a dummy response or an error right away. - // --2-- Halt execution, process the request, resume execution. - // TODO(polina): do this for setting breakpoints + // --2-- Halt execution, process the request, maybe resume execution. // --3-- Handle such requests asynchronously and let them block until // the process stops or terminates (e.g. using a channel and a single // goroutine to preserve the order). This might not be appropriate @@ -427,6 +426,28 @@ func (s *Server) handleRequest(request dap.Message) { Body: dap.ThreadsResponseBody{Threads: []dap.Thread{{Id: -1, Name: "Current"}}}, } s.send(response) + case *dap.SetBreakpointsRequest: + s.log.Debug("halting execution to set breakpoints") + _, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil) + if err != nil { + s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error()) + return + } + s.onSetBreakpointsRequest(request) + // TODO(polina): consider resuming execution here automatically after suppressing + // a stop event when an operation in doRunCommand returns. In case that operation + // was already stopping for a different reason, we would need to examine the state + // that is returned to determine if this halt was the cause of the stop or not. + // We should stop with an event and not resume if one of the following is true: + // - StopReason is anything but manual + // - Any thread has a breakpoint or CallReturn set + // - NextInProgress is false and the last command sent by the user was: next, + // step, stepOut, reverseNext, reverseStep or reverseStepOut + // Otherwise, we can skip the stop event and resume the temporarily + // interrupted process execution with api.DirectionCongruentContinue. + // For this to apply in cases other than api.Continue, we would also need to + // introduce a new version of halt that skips ClearInternalBreakpoints + // in proc.(*Target).Continue, leaving NextInProgress as true. default: r := request.(dap.RequestMessage).GetRequest() s.sendErrorResponse(*r, DebuggeeIsRunning, fmt.Sprintf("Unable to process `%s`", r.Command), "debuggee is running") @@ -1065,7 +1086,7 @@ func (s *Server) onConfigurationDoneRequest(request *dap.ConfigurationDoneReques } s.send(&dap.ConfigurationDoneResponse{Response: *newResponse(request.Request)}) if !s.args.stopOnEntry { - s.doCommand(api.Continue, asyncSetupDone) + s.doRunCommand(api.Continue, asyncSetupDone) } } @@ -1075,7 +1096,7 @@ func (s *Server) onContinueRequest(request *dap.ContinueRequest, asyncSetupDone s.send(&dap.ContinueResponse{ Response: *newResponse(request.Request), Body: dap.ContinueResponseBody{AllThreadsContinued: true}}) - s.doCommand(api.Continue, asyncSetupDone) + s.doRunCommand(api.Continue, asyncSetupDone) } func fnName(loc *proc.Location) string { @@ -1221,7 +1242,7 @@ func stoppedGoroutineID(state *api.DebuggerState) (id int) { return id } -// doStepCommand is a wrapper around doCommand that +// doStepCommand is a wrapper around doRunCommand that // first switches selected goroutine. asyncSetupDone is // a channel that will be closed to signal that an // asynchornous command has completed setup or was interrupted @@ -1245,7 +1266,7 @@ func (s *Server) doStepCommand(command string, threadId int, asyncSetupDone chan s.send(stopped) return } - s.doCommand(command, asyncSetupDone) + s.doRunCommand(command, asyncSetupDone) } // onPauseRequest sends a not-yet-implemented error response. @@ -1958,14 +1979,14 @@ func (s *Server) resetHandlesForStoppedEvent() { s.variableHandles.reset() } -// doCommand runs a debugger command until it stops on +// doRunCommand runs a debugger command until it stops on // termination, error, breakpoint, etc, when an appropriate // event needs to be sent to the client. asyncSetupDone is // a channel that will be closed to signal that an // asynchornous command has completed setup or was interrupted // due to an error, so the server is ready to receive new requests. -func (s *Server) doCommand(command string, asyncSetupDone chan struct{}) { - // TODO(polina): it appears that debugger.Command doesn't close +func (s *Server) doRunCommand(command string, asyncSetupDone chan struct{}) { + // TODO(polina): it appears that debugger.Command doesn't always close // asyncSetupDone (e.g. when having an error next while nexting). // So we should always close it ourselves just in case. defer s.asyncCommandDone(asyncSetupDone) @@ -1975,6 +1996,9 @@ func (s *Server) doCommand(command string, asyncSetupDone chan struct{}) { return } + stopReason := s.debugger.StopReason() + s.log.Debugf("%q command stopped - reason %q", command, stopReason) + s.resetHandlesForStoppedEvent() stopped := &dap.StoppedEvent{Event: *newEvent("stopped")} stopped.Body.AllThreadsStopped = true @@ -1982,9 +2006,7 @@ func (s *Server) doCommand(command string, asyncSetupDone chan struct{}) { if err == nil { stopped.Body.ThreadId = stoppedGoroutineID(state) - sr := s.debugger.StopReason() - s.log.Debugf("%q command stopped - reason %q", command, sr) - switch sr { + switch stopReason { case proc.StopNextFinished: stopped.Body.Reason = "step" case proc.StopManual: // triggered by halt diff --git a/service/dap/server_test.go b/service/dap/server_test.go index 18e05d67..5dcf11c9 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -1778,6 +1778,32 @@ func TestLaunchRequestWithStackTraceDepth(t *testing.T) { }) } +type Breakpoint struct { + line int + path string + verified bool + msgPrefix string +} + +func expectSetBreakpointsResponse(t *testing.T, client *daptest.Client, bps []Breakpoint) { + t.Helper() + checkSetBreakpointsResponse(t, client, bps, client.ExpectSetBreakpointsResponse(t)) +} + +func checkSetBreakpointsResponse(t *testing.T, client *daptest.Client, bps []Breakpoint, got *dap.SetBreakpointsResponse) { + t.Helper() + if len(got.Body.Breakpoints) != len(bps) { + t.Errorf("got %#v,\nwant len(Breakpoints)=%d", got, len(bps)) + return + } + for i, bp := range got.Body.Breakpoints { + if bp.Line != bps[i].line || bp.Verified != bps[i].verified || bp.Source.Path != bps[i].path || + !strings.HasPrefix(bp.Message, bps[i].msgPrefix) { + t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i]) + } + } +} + // TestSetBreakpoint executes to a breakpoint and tests different // configurations of setBreakpoint requests. func TestSetBreakpoint(t *testing.T) { @@ -1793,34 +1819,13 @@ func TestSetBreakpoint(t *testing.T) { execute: func() { handleStop(t, client, 1, "main.main", 16) - type Breakpoint struct { - line int - path string - verified bool - msgPrefix string - } - expectSetBreakpointsResponse := func(bps []Breakpoint) { - t.Helper() - got := client.ExpectSetBreakpointsResponse(t) - if len(got.Body.Breakpoints) != len(bps) { - t.Errorf("got %#v,\nwant len(Breakpoints)=%d", got, len(bps)) - return - } - for i, bp := range got.Body.Breakpoints { - if bp.Line != bps[i].line || bp.Verified != bps[i].verified || bp.Source.Path != bps[i].path || - !strings.HasPrefix(bp.Message, bps[i].msgPrefix) { - t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i]) - } - } - } - // Set two breakpoints at the next two lines in main client.SetBreakpointsRequest(fixture.Source, []int{17, 18}) - expectSetBreakpointsResponse([]Breakpoint{{17, fixture.Source, true, ""}, {18, fixture.Source, true, ""}}) + expectSetBreakpointsResponse(t, client, []Breakpoint{{17, fixture.Source, true, ""}, {18, fixture.Source, true, ""}}) // Clear 17, reset 18 client.SetBreakpointsRequest(fixture.Source, []int{18}) - expectSetBreakpointsResponse([]Breakpoint{{18, fixture.Source, true, ""}}) + expectSetBreakpointsResponse(t, client, []Breakpoint{{18, fixture.Source, true, ""}}) // Skip 17, continue to 18 client.ContinueRequest(1) @@ -1830,7 +1835,7 @@ func TestSetBreakpoint(t *testing.T) { // Set another breakpoint inside the loop in loop(), twice to trigger error client.SetBreakpointsRequest(fixture.Source, []int{8, 8}) - expectSetBreakpointsResponse([]Breakpoint{{8, fixture.Source, true, ""}, {8, "", false, "Breakpoint exists"}}) + expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}, {8, "", false, "Breakpoint exists"}}) // Continue into the loop client.ContinueRequest(1) @@ -1843,7 +1848,7 @@ func TestSetBreakpoint(t *testing.T) { // Edit the breakpoint to add a condition client.SetConditionalBreakpointsRequest(fixture.Source, []int{8}, map[int]string{8: "i == 3"}) - expectSetBreakpointsResponse([]Breakpoint{{8, fixture.Source, true, ""}}) + expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}}) // Continue until condition is hit client.ContinueRequest(1) @@ -1856,7 +1861,7 @@ func TestSetBreakpoint(t *testing.T) { // Edit the breakpoint to remove a condition client.SetConditionalBreakpointsRequest(fixture.Source, []int{8}, map[int]string{8: ""}) - expectSetBreakpointsResponse([]Breakpoint{{8, fixture.Source, true, ""}}) + expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}}) // Continue for one more loop iteration client.ContinueRequest(1) @@ -1869,7 +1874,7 @@ func TestSetBreakpoint(t *testing.T) { // Set at a line without a statement client.SetBreakpointsRequest(fixture.Source, []int{1000}) - expectSetBreakpointsResponse([]Breakpoint{{1000, "", false, "could not find statement"}}) // all cleared, none set + expectSetBreakpointsResponse(t, client, []Breakpoint{{1000, "", false, "could not find statement"}}) // all cleared, none set }, // The program has an infinite loop, so we must kill it by disconnecting. disconnect: true, @@ -1877,6 +1882,78 @@ func TestSetBreakpoint(t *testing.T) { }) } +func expectSetBreakpointsResponseAndStoppedEvent(t *testing.T, client *daptest.Client) (se *dap.StoppedEvent, br *dap.SetBreakpointsResponse) { + for i := 0; i < 2; i++ { + switch m := client.ExpectMessage(t).(type) { + case *dap.StoppedEvent: + se = m + case *dap.SetBreakpointsResponse: + br = m + default: + t.Fatalf("Unexpected message type: expect StoppedEvent or SetBreakpointsResponse, got %#v", m) + } + } + if se == nil || br == nil { + t.Fatal("Expected StoppedEvent and SetBreakpointsResponse") + } + return se, br +} + +func TestSetBreakpointWhileRunning(t *testing.T) { + runTest(t, "integrationprog", func(client *daptest.Client, fixture protest.Fixture) { + runDebugSessionWithBPs(t, client, "launch", + // Launch + func() { + client.LaunchRequest("exec", fixture.Path, !stopOnEntry) + }, + // Set breakpoints + fixture.Source, []int{16}, + []onBreakpoint{{ + execute: func() { + // The program loops 3 times over lines 14-15-8-9-10-16 + handleStop(t, client, 1, "main.main", 16) // Line that sleeps for 1 second + + // We can set breakpoints while nexting + client.NextRequest(1) + client.ExpectNextResponse(t) + client.SetBreakpointsRequest(fixture.Source, []int{15}) // [16,] => [15,] + se, br := expectSetBreakpointsResponseAndStoppedEvent(t, client) + if se.Body.Reason != "pause" || !se.Body.AllThreadsStopped || se.Body.ThreadId != 0 && se.Body.ThreadId != 1 { + t.Errorf("\ngot %#v\nwant Reason='pause' AllThreadsStopped=true ThreadId=0/1", se) + } + checkSetBreakpointsResponse(t, client, []Breakpoint{{15, fixture.Source, true, ""}}, br) + // Halt cancelled next, so if we continue we will not stop at line 14. + client.ContinueRequest(1) + client.ExpectContinueResponse(t) + se = client.ExpectStoppedEvent(t) + if se.Body.Reason != "breakpoint" || !se.Body.AllThreadsStopped || se.Body.ThreadId != 1 { + t.Errorf("\ngot %#v\nwant Reason='breakpoint' AllThreadsStopped=true ThreadId=1", se) + } + handleStop(t, client, 1, "main.main", 15) + + // We can set breakpoints while continuing + client.ContinueRequest(1) + client.ExpectContinueResponse(t) + client.SetBreakpointsRequest(fixture.Source, []int{9}) // [15,] => [9,] + se, br = expectSetBreakpointsResponseAndStoppedEvent(t, client) + if se.Body.Reason != "pause" || !se.Body.AllThreadsStopped || se.Body.ThreadId != 0 && se.Body.ThreadId != 1 { + t.Errorf("\ngot %#v\nwant Reason='pause' AllThreadsStopped=true ThreadId=0/1", se) + } + checkSetBreakpointsResponse(t, client, []Breakpoint{{9, fixture.Source, true, ""}}, br) + client.ContinueRequest(1) + client.ExpectContinueResponse(t) + se = client.ExpectStoppedEvent(t) + if se.Body.Reason != "breakpoint" || !se.Body.AllThreadsStopped || se.Body.ThreadId != 1 { + t.Errorf("\ngot %#v\nwant Reason='breakpoint' AllThreadsStopped=true ThreadId=1", se) + } + handleStop(t, client, 1, "main.sayhi", 9) + + }, + disconnect: true, + }}) + }) +} + // TestLaunchSubstitutePath sets a breakpoint using a path // that does not exist and expects the substitutePath attribute // in the launch configuration to take care of the mapping.