pkg/proc: configure target to not clear stepping breakpoints (#2635)

In order for DAP to support halting the program (either manually or on a breakpoint) performing some action and then resuming execution, there needs to be a way to stop the program without clearing the internal breakpoints. This is necessary for log points and stopping the program to set breakpoints.

The debugging UI makes it seem like a user should be able to set or clear a breakpoint at any time. Adding this ability to complete synchronous requests while the program is running is thus important to create a seamless user experience.

This change just adds a configuration to determine whether the target should clear the stepping breakpoints, and changes the server to use this new mode. Using the new mode means that the DAP server must determine when it expect the next to be canceled and do this manually.

pkg/proc/proc_test.go

@@ -2579,6 +2579,32 @@ func TestNextBreakpoint(t *testing.T) {
 	})
 }
 
+func TestNextBreakpointKeepsSteppingBreakpoints(t *testing.T) {
+	protest.AllowRecording(t)
+	withTestProcess("testnextprog", t, func(p *proc.Target, fixture protest.Fixture) {
+		p.KeepSteppingBreakpoints = proc.TracepointKeepsSteppingBreakpoints
+		bp := setFileBreakpoint(p, t, fixture.Source, 34)
+		assertNoError(p.Continue(), t, "Continue()")
+		p.ClearBreakpoint(bp.Addr)
+
+		// Next should be interrupted by a tracepoint on the same goroutine.
+		bp = setFileBreakpoint(p, t, fixture.Source, 14)
+		bp.Tracepoint = true
+		assertNoError(p.Next(), t, "Next()")
+		assertLineNumber(p, t, 14, "wrong line number")
+		if !p.Breakpoints().HasSteppingBreakpoints() {
+			t.Fatal("does not have internal breakpoints after hitting tracepoint on same goroutine")
+		}
+
+		// Continue to complete next.
+		assertNoError(p.Continue(), t, "Continue()")
+		assertLineNumber(p, t, 35, "wrong line number")
+		if p.Breakpoints().HasSteppingBreakpoints() {
+			t.Fatal("has internal breakpoints after completing next")
+		}
+	})
+}
+
 func TestStepOutDefer(t *testing.T) {
 	protest.AllowRecording(t)
 	withTestProcess("testnextdefer", t, func(p *proc.Target, fixture protest.Fixture) {
@@ -3648,6 +3674,26 @@ func TestIssue1145(t *testing.T) {
 	})
 }
 
+func TestHaltKeepsSteppingBreakpoints(t *testing.T) {
+	withTestProcess("sleep", t, func(p *proc.Target, fixture protest.Fixture) {
+		p.KeepSteppingBreakpoints = proc.HaltKeepsSteppingBreakpoints
+		setFileBreakpoint(p, t, fixture.Source, 18)
+		assertNoError(p.Continue(), t, "Continue()")
+		resumeChan := make(chan struct{}, 1)
+		p.ResumeNotify(resumeChan)
+		go func() {
+			<-resumeChan
+			time.Sleep(100 * time.Millisecond)
+			p.RequestManualStop()
+		}()
+
+		assertNoError(p.Next(), t, "Next()")
+		if !p.Breakpoints().HasSteppingBreakpoints() {
+			t.Fatal("does not have internal breakpoints after manual stop request")
+		}
+	})
+}
+
 func TestDisassembleGlobalVars(t *testing.T) {
 	skipOn(t, "broken - global variable symbolication", "arm64") // On ARM64 symLookup can't look up variables due to how they are loaded, see issue #1778
 	// On 386 linux when pie, the genered code use __x86.get_pc_thunk to ensure position-independent.

pkg/proc/target.go

@@ -46,6 +46,10 @@ type Target struct {
 	// CanDump is true if core dumping is supported.
 	CanDump bool
 
+	// KeepSteppingBreakpoints determines whether certain stop reasons (e.g. manual halts)
+	// will keep the stepping breakpoints instead of clearing them.
+	KeepSteppingBreakpoints KeepSteppingBreakpoints
+
 	// currentThread is the thread that will be used by next/step/stepout and to evaluate variables if no goroutine is selected.
 	currentThread Thread
 
@@ -76,6 +80,13 @@ type Target struct {
 	fakeMemoryRegistryMap map[string]*compositeMemory
 }
 
+type KeepSteppingBreakpoints uint8
+
+const (
+	HaltKeepsSteppingBreakpoints KeepSteppingBreakpoints = 1 << iota
+	TracepointKeepsSteppingBreakpoints
+)
+
 // ErrProcessExited indicates that the process has exited and contains both
 // process id and exit status.
 type ErrProcessExited struct {

pkg/proc/target_exec.go

@@ -61,13 +61,17 @@ func (dbp *Target) Continue() error {
 		// manual stop request and hit a breakpoint.
 		if dbp.CheckAndClearManualStopRequest() {
 			dbp.StopReason = StopManual
-			dbp.ClearSteppingBreakpoints()
+			if dbp.KeepSteppingBreakpoints&HaltKeepsSteppingBreakpoints == 0 {
+				dbp.ClearSteppingBreakpoints()
+			}
 		}
 	}()
 	for {
 		if dbp.CheckAndClearManualStopRequest() {
 			dbp.StopReason = StopManual
-			dbp.ClearSteppingBreakpoints()
+			if dbp.KeepSteppingBreakpoints&HaltKeepsSteppingBreakpoints == 0 {
+				dbp.ClearSteppingBreakpoints()
+			}
 			return nil
 		}
 		dbp.ClearCaches()
@@ -201,7 +205,8 @@ func (dbp *Target) Continue() error {
 			if err != nil {
 				return err
 			}
-			if onNextGoroutine {
+			if onNextGoroutine &&
+				((!curbp.Tracepoint && !curbp.TraceReturn) || dbp.KeepSteppingBreakpoints&TracepointKeepsSteppingBreakpoints == 0) {
 				err := dbp.ClearSteppingBreakpoints()
 				if err != nil {
 					return err

service/dap/server.go

@@ -1413,6 +1413,8 @@ func (s *Server) onConfigurationDoneRequest(request *dap.ConfigurationDoneReques
 		}
 		s.send(e)
 	}
+	s.debugger.Target().KeepSteppingBreakpoints = proc.HaltKeepsSteppingBreakpoints | proc.TracepointKeepsSteppingBreakpoints
+
 	s.send(&dap.ConfigurationDoneResponse{Response: *newResponse(request.Request)})
 	if !s.args.stopOnEntry {
 		s.doRunCommand(api.Continue, asyncSetupDone)
@@ -2893,6 +2895,13 @@ func (s *Server) doRunCommand(command string, asyncSetupDone chan struct{}) {
 	stopped.Body.AllThreadsStopped = true
 
 	if err == nil {
+		if stopReason == proc.StopManual {
+			if err := s.debugger.CancelNext(); err != nil {
+				s.log.Error(err)
+			} else {
+				state.NextInProgress = false
+			}
+		}
 		// TODO(suzmue): If stopped.Body.ThreadId is not a valid goroutine
 		// then the stopped reason does not show up anywhere in the
 		// vscode ui.