dap: remote attach must not use terminateDebuggee in single-client launch mode (#2995)

cmd/dlv/dlv_test.go

@@ -754,10 +754,12 @@ func TestDAPCmdWithNoDebugBinary(t *testing.T) {
 	cmd.Wait()
 }
 
-func newDAPRemoteClient(t *testing.T, addr string) *daptest.Client {
+func newDAPRemoteClient(t *testing.T, addr string, isDlvAttach bool, isMulti bool) *daptest.Client {
 	c := daptest.NewClient(addr)
 	c.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
-	c.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
+	if isDlvAttach || isMulti {
+		c.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
+	}
 	c.ExpectInitializedEvent(t)
 	c.ExpectAttachResponse(t)
 	c.ConfigurationDoneRequest()
@@ -794,7 +796,7 @@ func TestRemoteDAPClient(t *testing.T) {
 		}
 	}()
 
-	client := newDAPRemoteClient(t, listenAddr)
+	client := newDAPRemoteClient(t, listenAddr, false, false)
 	client.ContinueRequest(1)
 	client.ExpectContinueResponse(t)
 	client.ExpectTerminatedEvent(t)
@@ -854,7 +856,7 @@ func TestRemoteDAPClientMulti(t *testing.T) {
 	dapclient0.ExpectErrorResponse(t)
 
 	// Client 1 connects and continues to main.main
-	dapclient := newDAPRemoteClient(t, listenAddr)
+	dapclient := newDAPRemoteClient(t, listenAddr, false, true)
 	dapclient.SetFunctionBreakpointsRequest([]godap.FunctionBreakpoint{{Name: "main.main"}})
 	dapclient.ExpectSetFunctionBreakpointsResponse(t)
 	dapclient.ContinueRequest(1)
@@ -864,7 +866,7 @@ func TestRemoteDAPClientMulti(t *testing.T) {
 	closeDAPRemoteMultiClient(t, dapclient, "halted")
 
 	// Client 2 reconnects at main.main and continues to process exit
-	dapclient2 := newDAPRemoteClient(t, listenAddr)
+	dapclient2 := newDAPRemoteClient(t, listenAddr, false, true)
 	dapclient2.CheckStopLocation(t, 1, "main.main", 5)
 	dapclient2.ContinueRequest(1)
 	dapclient2.ExpectContinueResponse(t)
@@ -924,7 +926,7 @@ func TestRemoteDAPClientAfterContinue(t *testing.T) {
 		}
 	}()
 
-	c := newDAPRemoteClient(t, listenAddr)
+	c := newDAPRemoteClient(t, listenAddr, false, true)
 	c.ContinueRequest(1)
 	c.ExpectContinueResponse(t)
 	c.DisconnectRequest()
@@ -933,7 +935,7 @@ func TestRemoteDAPClientAfterContinue(t *testing.T) {
 	c.ExpectTerminatedEvent(t)
 	c.Close()
 
-	c = newDAPRemoteClient(t, listenAddr)
+	c = newDAPRemoteClient(t, listenAddr, false, true)
 	c.DisconnectRequestWithKillOption(true)
 	c.ExpectOutputEventDetachingKill(t)
 	c.ExpectDisconnectResponse(t)

service/dap/server.go

@@ -393,7 +393,7 @@ func (s *Session) Close() {
 	defer s.mu.Unlock()
 
 	if s.debugger != nil {
-		killProcess := s.config.Debugger.AttachPid == 0
+		killProcess := s.debugger.AttachPid() == 0
 		s.stopDebugSession(killProcess)
 	} else if s.noDebugProcess != nil {
 		s.stopNoDebugProcess()
@@ -1165,7 +1165,7 @@ func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) {
 		// In case of attach, we leave the program
 		// running by default, which can be
 		// overridden by an explicit request to terminate.
-		killProcess := s.config.Debugger.AttachPid == 0 || request.Arguments.TerminateDebuggee
+		killProcess := s.debugger.AttachPid() == 0 || request.Arguments.TerminateDebuggee
 		err = s.stopDebugSession(killProcess)
 	} else if s.noDebugProcess != nil {
 		s.stopNoDebugProcess()
@@ -1226,7 +1226,7 @@ func (s *Session) stopDebugSession(killProcess bool) error {
 	} else if killProcess {
 		s.logToConsole("Detaching and terminating target process")
 	} else {
-		s.logToConsole("Detaching without terminating target processs")
+		s.logToConsole("Detaching without terminating target process")
 	}
 	err = s.debugger.Detach(killProcess)
 	if err != nil {
@@ -1770,9 +1770,15 @@ func (s *Session) onAttachRequest(request *dap.AttachRequest) {
 		if s.config.Debugger.Backend == "rr" {
 			s.send(&dap.CapabilitiesEvent{Event: *newEvent("capabilities"), Body: dap.CapabilitiesEventBody{Capabilities: dap.Capabilities{SupportsStepBack: true}}})
 		}
-		// Give the user an option to terminate this server when client disconnects (default is to leave it)
-		s.send(&dap.CapabilitiesEvent{Event: *newEvent("capabilities"), Body: dap.CapabilitiesEventBody{Capabilities: dap.Capabilities{SupportTerminateDebuggee: true}}})
-		// TODO(polina); also use SupportSuspendDebuggee when available
+		// Customize termination options for debugger and debuggee
+		if s.config.AcceptMulti {
+			// User can stop debugger with process or leave it running
+			s.send(&dap.CapabilitiesEvent{Event: *newEvent("capabilities"), Body: dap.CapabilitiesEventBody{Capabilities: dap.Capabilities{SupportTerminateDebuggee: true}}})
+			// TODO(polina): support SupportSuspendDebuggee when available
+		} else if s.config.Debugger.AttachPid > 0 {
+			// User can stop debugger with process or leave the processs running
+			s.send(&dap.CapabilitiesEvent{Event: *newEvent("capabilities"), Body: dap.CapabilitiesEventBody{Capabilities: dap.Capabilities{SupportTerminateDebuggee: true}}})
+		} // else program was launched and the only option will be to stop both
 	default:
 		s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach",
 			fmt.Sprintf("invalid debug configuration - unsupported 'mode' attribute %q", args.Mode))

service/dap/server_test.go

@@ -77,6 +77,9 @@ func startDAPServerWithClient(t *testing.T, serverStopped chan struct{}) *daptes
 	return client
 }
 
+// Starts an empty server and a stripped down config just to establish a client connection.
+// To mock a server created by dap.NewServer(config) or serving dap.NewSession(conn, config, debugger)
+// set those arg fields manually after the server creation.
 func startDAPServer(t *testing.T, serverStopped chan struct{}) (server *Server, forceStop chan struct{}) {
 	// Start the DAP server.
 	listener, err := net.Listen("tcp", ":0")
@@ -6516,6 +6519,18 @@ func launchDebuggerWithTargetHalted(t *testing.T, fixture string) (*protest.Fixt
 	return &fixbin, dbg
 }
 
+func attachDebuggerWithTargetHalted(t *testing.T, fixture string) (*exec.Cmd, *debugger.Debugger) {
+	t.Helper()
+	fixbin := protest.BuildFixture(fixture, protest.AllNonOptimized)
+	cmd := execFixture(t, fixbin)
+	cfg := service.Config{Debugger: debugger.Config{Backend: "default", AttachPid: cmd.Process.Pid}}
+	dbg, err := debugger.New(&cfg.Debugger, nil) // debugger halts process on entry
+	if err != nil {
+		t.Fatal("failed to start debugger:", err)
+	}
+	return cmd, dbg
+}
+
 // runTestWithDebugger starts the server and sets its debugger, initializes a debug session,
 // runs test, then disconnects. Expects no running async handler at the end of test() (either
 // process is halted or debug session never launched.)
@@ -6528,6 +6543,10 @@ func runTestWithDebugger(t *testing.T, dbg *debugger.Debugger, test func(c *dapt
 	if server.session == nil {
 		t.Fatal("DAP session is not ready")
 	}
+	// Mock dap.NewSession arguments, so
+	// this dap.Server can be used as a proxy for
+	// rpccommon.Server running dap.Session.
+	server.session.config.Debugger.AttachPid = dbg.AttachPid()
 	server.session.debugger = dbg
 	server.sessionMu.Unlock()
 	defer client.Close()
@@ -6537,7 +6556,7 @@ func runTestWithDebugger(t *testing.T, dbg *debugger.Debugger, test func(c *dapt
 	test(client)
 
 	client.DisconnectRequest()
-	if server.config.Debugger.AttachPid == 0 { // launched target
+	if dbg.AttachPid() == 0 { // launched target
 		client.ExpectOutputEventDetachingKill(t)
 	} else { // attached to target
 		client.ExpectOutputEventDetachingNoKill(t)
@@ -6548,9 +6567,24 @@ func runTestWithDebugger(t *testing.T, dbg *debugger.Debugger, test func(c *dapt
 	<-serverStopped
 }
 
-func TestAttachRemoteToHaltedTargetStopOnEntry(t *testing.T) {
+func TestAttachRemoteToDlvLaunchHaltedStopOnEntry(t *testing.T) {
 	// Halted + stop on entry
 	_, dbg := launchDebuggerWithTargetHalted(t, "increment")
+	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
+		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
+		client.ExpectInitializedEvent(t)
+		client.ExpectAttachResponse(t)
+		client.ConfigurationDoneRequest()
+		client.ExpectStoppedEvent(t)
+		client.ExpectConfigurationDoneResponse(t)
+	})
+}
+
+func TestAttachRemoteToDlvAttachHaltedStopOnEntry(t *testing.T) {
+	if runtime.GOOS == "freebsd" || runtime.GOOS == "windows" {
+		t.SkipNow()
+	}
+	cmd, dbg := attachDebuggerWithTargetHalted(t, "http_server")
 	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
 		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
 		client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
@@ -6560,6 +6594,7 @@ func TestAttachRemoteToHaltedTargetStopOnEntry(t *testing.T) {
 		client.ExpectStoppedEvent(t)
 		client.ExpectConfigurationDoneResponse(t)
 	})
+	cmd.Process.Kill()
 }
 
 func TestAttachRemoteToHaltedTargetContinueOnEntry(t *testing.T) {
@@ -6567,7 +6602,6 @@ func TestAttachRemoteToHaltedTargetContinueOnEntry(t *testing.T) {
 	_, dbg := launchDebuggerWithTargetHalted(t, "http_server")
 	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
 		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": false})
-		client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
 		client.ExpectInitializedEvent(t)
 		client.ExpectAttachResponse(t)
 		client.ConfigurationDoneRequest()
@@ -6584,7 +6618,6 @@ func TestAttachRemoteToRunningTargetStopOnEntry(t *testing.T) {
 	fixture, dbg := launchDebuggerWithTargetRunning(t, "loopprog")
 	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
 		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
-		client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
 		client.ExpectInitializedEvent(t)
 		client.ExpectAttachResponse(t)
 		// Target is halted here
@@ -6604,7 +6637,6 @@ func TestAttachRemoteToRunningTargetContinueOnEntry(t *testing.T) {
 	fixture, dbg := launchDebuggerWithTargetRunning(t, "loopprog")
 	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
 		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": false})
-		client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
 		client.ExpectInitializedEvent(t)
 		client.ExpectAttachResponse(t)
 		// Target is halted here
@@ -6643,8 +6675,8 @@ func TestAttachRemoteMultiClientDisconnect(t *testing.T) {
 			if server.session == nil {
 				t.Fatal("dap session is not ready")
 			}
-			// DAP server doesn't support accept-multiclient, but we can use this
-			// hack to test the inner connection logic that can be used by a server that does.
+			// A dap.Server doesn't support accept-multiclient, but we can use this
+			// hack to test the inner connection logic that is used by a server that does.
 			server.session.config.AcceptMulti = true
 			_, server.session.debugger = launchDebuggerWithTargetHalted(t, "increment")
 			server.sessionMu.Unlock()
@@ -6672,7 +6704,7 @@ func TestAttachRemoteMultiClientDisconnect(t *testing.T) {
 			if tc.expect == closingClientSessionOnly {
 				// At this point a multi-client server is still running.
 				verifySessionStopped(t, server.session)
-				// Since it is a dap server, it cannot accept another client, so the only
+				// Since it is a dap.Server, it cannot accept another client, so the only
 				// way to take down the server is to force-kill it.
 				close(forceStop)
 			}

service/debugger/debugger.go

@@ -2205,6 +2205,10 @@ func (d *Debugger) BuildID() string {
 	return d.target.BinInfo().BuildID
 }
 
+func (d *Debugger) AttachPid() int {
+	return d.config.AttachPid
+}
+
 func (d *Debugger) GetBufferedTracepoints() []api.TracepointResult {
 	traces := d.target.GetBufferedTracepoints()
 	if traces == nil {