[vscode] set default values for terminateDebuggee for the disconnect request

Summary:
Recently I've noticed that VSCode sometimes doesn't send the terminateDebuggee flag within the disconnectRequest,
even though lldb-vscode sets the terminateDebuggee capability correctly.
This has been causing that inferiors don't die after the debug session ends, and many users have reported issues because of this.

An easy way to mitigate this is to set better default values for the terminateDebuggee field in the disconnect request.
I'm assuming that for a launch request, the default will be true, and for attach it'll be false.

Reviewers: clayborg, labath, aadsm

Subscribers: lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D81200

diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
index 790628d..42eb306 100644 (file)
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -239,14 +239,15 @@ class VSCodeTestCaseBase(TestBase):
 
     def attach(self, program=None, pid=None, waitFor=None, trace=None,
                initCommands=None, preRunCommands=None, stopCommands=None,
-               exitCommands=None, attachCommands=None, coreFile=None):
+               exitCommands=None, attachCommands=None, coreFile=None, disconnectAutomatically=True):
         '''Build the default Makefile target, create the VSCode debug adaptor,
            and attach to the process.
         '''
         # Make sure we disconnect and terminate the VSCode debug adaptor even
         # if we throw an exception during the test case.
         def cleanup():
-            self.vscode.request_disconnect(terminateDebuggee=True)
+            if disconnectAutomatically:
+                self.vscode.request_disconnect(terminateDebuggee=True)
             self.vscode.terminate()
 
         # Execute the cleanup function during test case tear down.

diff --git a/lldb/test/API/tools/lldb-vscode/disconnect/Makefile b/lldb/test/API/tools/lldb-vscode/disconnect/Makefile
new file mode 100644 (file)
index 0000000..99998b2
--- /dev/null
+++ b/lldb/test/API/tools/lldb-vscode/disconnect/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

diff --git a/lldb/test/API/tools/lldb-vscode/disconnect/TestVSCode_disconnect.py b/lldb/test/API/tools/lldb-vscode/disconnect/TestVSCode_disconnect.py
new file mode 100644 (file)
index 0000000..2a4103a
--- /dev/null
+++ b/lldb/test/API/tools/lldb-vscode/disconnect/TestVSCode_disconnect.py
@@ -0,0 +1,82 @@
+"""
+Test lldb-vscode disconnect request
+"""
+
+
+import unittest2
+import vscode
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbvscode_testcase
+import subprocess
+import time
+import os
+
+
+class TestVSCode_launch(lldbvscode_testcase.VSCodeTestCaseBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    source = 'main.cpp'
+
+    def disconnect_and_assert_no_output_printed(self):
+        self.vscode.request_disconnect()
+        # verify we didn't get any input after disconnect
+        time.sleep(2)
+        output = self.get_stdout()
+        self.assertTrue(output is None or len(output) == 0)
+
+    @skipIfWindows
+    @skipIfRemote
+    def test_launch(self):
+        """
+            This test launches a process that would creates a file, but we disconnect
+            before the file is created, which terminates the process and thus the file is not
+            created.
+        """
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+
+        # We set a breakpoint right before the side effect file is created
+        self.set_source_breakpoints(self.source, [line_number(self.source, '// breakpoint')])
+        self.continue_to_next_stop()
+
+        self.vscode.request_disconnect()
+        # verify we didn't produce the side effect file
+        time.sleep(1)
+        self.assertFalse(os.path.exists(program + ".side_effect"))
+
+
+    @skipIfWindows
+    @skipIfRemote
+    def test_attach(self):
+        """
+            This test attaches to a process that creates a file. We attach and disconnect
+            before the file is created, and as the process is not terminated upon disconnection,
+            the file is created anyway.
+        """
+        self.build_and_create_debug_adaptor()
+        program = self.getBuildArtifact("a.out")
+
+        # Use a file as a synchronization point between test and inferior.
+        sync_file_path = lldbutil.append_to_process_working_directory(self,
+            "sync_file_%d" % (int(time.time())))
+        self.addTearDownHook(
+            lambda: self.run_platform_command(
+                "rm %s" %
+                (sync_file_path)))
+
+        self.process = subprocess.Popen([program, sync_file_path])
+        lldbutil.wait_for_file_on_target(self, sync_file_path)
+
+        self.attach(pid=self.process.pid, disconnectAutomatically=False)
+        response = self.vscode.request_evaluate("wait_for_attach = false;")
+        self.assertTrue(response['success'])
+
+        # verify we haven't produced the side effect file yet
+        self.assertFalse(os.path.exists(program + ".side_effect"))
+
+        self.vscode.request_disconnect()
+        time.sleep(2)
+        # verify we produced the side effect file, as the program continued after disconnecting
+        self.assertTrue(os.path.exists(program + ".side_effect"))

diff --git a/lldb/test/API/tools/lldb-vscode/disconnect/main.cpp b/lldb/test/API/tools/lldb-vscode/disconnect/main.cpp
new file mode 100644 (file)
index 0000000..d3d7a4b
--- /dev/null
+++ b/lldb/test/API/tools/lldb-vscode/disconnect/main.cpp
@@ -0,0 +1,33 @@
+#include <chrono>
+#include <cstdio>
+#include <fstream>
+#include <string>
+#include <thread>
+
+volatile bool wait_for_attach = true;
+
+void handle_attach(char *sync_file_path) {
+  lldb_enable_attach();
+
+  {
+    // Create a file to signal that this process has started up.
+    std::ofstream sync_file;
+    sync_file.open(sync_file_path);
+  }
+
+  while (wait_for_attach)
+    std::this_thread::sleep_for(std::chrono::milliseconds(10));
+}
+
+int main(int argc, char **args) {
+  if (argc == 2)
+    handle_attach(args[1]);
+
+  // We let the binary live a little bit to see if it executed after detaching
+  // from // breakpoint
+
+  // Create a file to signal that this process has started up.
+  std::ofstream out_file; // breakpoint
+  out_file.open(std::string(args[0]) + ".side_effect");
+  return 0;
+}

diff --git a/lldb/tools/lldb-vscode/VSCode.cpp b/lldb/tools/lldb-vscode/VSCode.cpp
index 36bc8ec..81a8e10 100644 (file)
--- a/lldb/tools/lldb-vscode/VSCode.cpp
+++ b/lldb/tools/lldb-vscode/VSCode.cpp
@@ -38,7 +38,7 @@ VSCode::VSCode()
            {"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
            {"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),
       focus_tid(LLDB_INVALID_THREAD_ID), sent_terminated_event(false),
-      stop_at_entry(false) {
+      stop_at_entry(false), is_attach(false) {
   const char *log_file_path = getenv("LLDBVSCODE_LOG");
 #if defined(_WIN32)
 // Windows opens stdout and stdin in text mode which converts \n to 13,10

diff --git a/lldb/tools/lldb-vscode/VSCode.h b/lldb/tools/lldb-vscode/VSCode.h
index 5298d75..aceba48 100644 (file)
--- a/lldb/tools/lldb-vscode/VSCode.h
+++ b/lldb/tools/lldb-vscode/VSCode.h
@@ -89,6 +89,7 @@ struct VSCode {
   lldb::tid_t focus_tid;
   bool sent_terminated_event;
   bool stop_at_entry;
+  bool is_attach;
   // Keep track of the last stop thread index IDs as threads won't go away
   // unless we send a "thread" event to indicate the thread exited.
   llvm::DenseSet<lldb::tid_t> thread_ids;

diff --git a/lldb/tools/lldb-vscode/lldb-vscode.cpp b/lldb/tools/lldb-vscode/lldb-vscode.cpp
index f1620d9..3ec4c2c 100644 (file)
--- a/lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ b/lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -514,6 +514,7 @@ void SetSourceMapFromArguments(const llvm::json::Object &arguments) {
 //   }]
 // }
 void request_attach(const llvm::json::Object &request) {
+  g_vsc.is_attach = true;
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);
@@ -769,7 +770,9 @@ void request_disconnect(const llvm::json::Object &request) {
   FillResponse(request, response);
   auto arguments = request.getObject("arguments");
 
-  bool terminateDebuggee = GetBoolean(arguments, "terminateDebuggee", false);
+  bool defaultTerminateDebuggee = g_vsc.is_attach ? false : true;
+  bool terminateDebuggee =
+      GetBoolean(arguments, "terminateDebuggee", defaultTerminateDebuggee);
   lldb::SBProcess process = g_vsc.target.GetProcess();
   auto state = process.GetState();
 
@@ -788,10 +791,9 @@ void request_disconnect(const llvm::json::Object &request) {
   case lldb::eStateStopped:
   case lldb::eStateRunning:
     g_vsc.debugger.SetAsync(false);
-    if (terminateDebuggee)
-      process.Kill();
-    else
-      process.Detach();
+    lldb::SBError error = terminateDebuggee ? process.Kill() : process.Detach();
+    if (!error.Success())
+      response.try_emplace("error", error.GetCString());
     g_vsc.debugger.SetAsync(true);
     break;
   }
@@ -1357,6 +1359,7 @@ void request_initialize(const llvm::json::Object &request) {
 //   }]
 // }
 void request_launch(const llvm::json::Object &request) {
+  g_vsc.is_attach = false;
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);