Fix a stall in running `quit` while a live process is running #74687

jimingham · 2023-12-07T01:28:28Z

We need to generate events when finalizing, or we won't know that we succeeded in stopping the process to detach/kill. Instead, we stall and then after our 20 interrupt timeout, we kill the process (even if we were supposed to detach) and exit.

OTOH, we have to not generate events when the Process is being destructed because shared_from_this has already been torn down, and using it will cause crashes.

succeeded in stopping the process to detach/kill. Instead, we stall and then after our 20 interrupt timeout, we kill the process (even if we were supposed to detach) and exit. OTOH, we have to not generate events when the Process is being destructed because shared_from_this has already been torn down, and using it will cause crashes.

llvmbot · 2023-12-07T01:28:58Z

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

We need to generate events when finalizing, or we won't know that we succeeded in stopping the process to detach/kill. Instead, we stall and then after our 20 interrupt timeout, we kill the process (even if we were supposed to detach) and exit.

OTOH, we have to not generate events when the Process is being destructed because shared_from_this has already been torn down, and using it will cause crashes.

Full diff: https://github.com/llvm/llvm-project/pull/74687.diff

14 Files Affected:

(modified) lldb/include/lldb/Target/Process.h (+9-1)
(modified) lldb/source/Core/Debugger.cpp (+1-1)
(modified) lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp (+1-1)
(modified) lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp (+1-1)
(modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+1-1)
(modified) lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp (+1-1)
(modified) lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp (+1-1)
(modified) lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp (+1-1)
(modified) lldb/source/Target/Process.cpp (+11-3)
(modified) lldb/source/Target/ProcessTrace.cpp (+1-1)
(modified) lldb/source/Target/Target.cpp (+1-1)
(added) lldb/test/API/driver/quit_speed/Makefile (+3)
(added) lldb/test/API/driver/quit_speed/TestQuitWithProcess.py (+34)
(added) lldb/test/API/driver/quit_speed/main.c (+10)

diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index 4646e3070cf14..889f553f897f5 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -558,7 +558,10 @@ class Process : public std::enable_shared_from_this<Process>,
   ///
   /// Subclasses that override this method should always call this superclass
   /// method.
-  virtual void Finalize();
+  /// If you are running Finalize in your Process subclass Destructor, pass
+  /// \btrue.  If we are in the destructor, shared_from_this will no longer
+  /// work, so we have to avoid doing anything that might trigger that.
+  virtual void Finalize(bool destructing);
 
   /// Return whether this object is valid (i.e. has not been finalized.)
   ///
@@ -3079,6 +3082,11 @@ void PruneThreadPlans();
   /// This is set at the beginning of Process::Finalize() to stop functions
   /// from looking up or creating things during or after a finalize call.
   std::atomic<bool> m_finalizing;
+  // When we are "Finalizing" we need to do some cleanup.  But if the Finalize
+  // call is coming in the Destructor, we can't do any actual work in the
+  // process because that is likely to call "shared_from_this" which crashes
+  // if run while destructing.  We use this flag to determine that.   
+  std::atomic<bool> m_destructing;
 
   /// Mask for code an data addresses. The default value (0) means no mask is
   /// set.  The bits set to 1 indicate bits that are NOT significant for
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 21f71e449ca5e..d0b362045e801 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -930,7 +930,7 @@ void Debugger::Clear() {
     for (TargetSP target_sp : m_target_list.Targets()) {
       if (target_sp) {
         if (ProcessSP process_sp = target_sp->GetProcessSP())
-          process_sp->Finalize();
+          process_sp->Finalize(false);
         target_sp->Destroy();
       }
     }
diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
index 70bb9aa7a833c..4ab6c8cf1959d 100644
--- a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
+++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
@@ -164,7 +164,7 @@ ProcessKDP::~ProcessKDP() {
   // make sure all of the broadcaster cleanup goes as planned. If we destruct
   // this class, then Process::~Process() might have problems trying to fully
   // destroy the broadcaster.
-  Finalize();
+  Finalize(true);
 }
 
 Status ProcessKDP::DoWillLaunch(Module *module) {
diff --git a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
index aedc43a015ff1..24d5fdebd4bd5 100644
--- a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -108,7 +108,7 @@ ProcessElfCore::~ProcessElfCore() {
   // make sure all of the broadcaster cleanup goes as planned. If we destruct
   // this class, then Process::~Process() might have problems trying to fully
   // destroy the broadcaster.
-  Finalize();
+  Finalize(true);
 }
 
 lldb::addr_t ProcessElfCore::AddAddressRangeFromLoadSegment(
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index b04319703b946..b9ce4198471a0 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -303,7 +303,7 @@ ProcessGDBRemote::~ProcessGDBRemote() {
   // make sure all of the broadcaster cleanup goes as planned. If we destruct
   // this class, then Process::~Process() might have problems trying to fully
   // destroy the broadcaster.
-  Finalize();
+  Finalize(true);
 
   // The general Finalize is going to try to destroy the process and that
   // SHOULD shut down the async thread.  However, if we don't kill it it will
diff --git a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
index 9830a4b8599df..4ca475ee3425f 100644
--- a/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ b/lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -123,7 +123,7 @@ ProcessMachCore::~ProcessMachCore() {
   // make sure all of the broadcaster cleanup goes as planned. If we destruct
   // this class, then Process::~Process() might have problems trying to fully
   // destroy the broadcaster.
-  Finalize();
+  Finalize(true);
 }
 
 bool ProcessMachCore::CheckAddressForDyldOrKernel(lldb::addr_t addr,
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
index 0d5ca42691d3d..2e751e8624ef9 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -166,7 +166,7 @@ ProcessMinidump::~ProcessMinidump() {
   // make sure all of the broadcaster cleanup goes as planned. If we destruct
   // this class, then Process::~Process() might have problems trying to fully
   // destroy the broadcaster.
-  Finalize();
+  Finalize(true);
 }
 
 void ProcessMinidump::Initialize() {
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
index 54b367727913c..ed2bfb57f9918 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
@@ -140,7 +140,7 @@ ScriptedProcess::~ScriptedProcess() {
   // make sure all of the broadcaster cleanup goes as planned. If we destruct
   // this class, then Process::~Process() might have problems trying to fully
   // destroy the broadcaster.
-  Finalize();
+  Finalize(true);
 }
 
 void ScriptedProcess::Initialize() {
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 2d77144a9b28d..23f72645e8fcd 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -445,7 +445,7 @@ Process::Process(lldb::TargetSP target_sp, ListenerSP listener_sp,
       m_memory_cache(*this), m_allocated_memory_cache(*this),
       m_should_detach(false), m_next_event_action_up(), m_public_run_lock(),
       m_private_run_lock(), m_currently_handling_do_on_removals(false),
-      m_resume_requested(false), m_finalizing(false),
+      m_resume_requested(false), m_finalizing(false), m_destructing(false),
       m_clear_thread_plans_on_stop(false), m_force_next_event_delivery(false),
       m_last_broadcast_state(eStateInvalid), m_destroy_in_process(false),
       m_can_interpret_function_calls(false), m_run_thread_plan_lock(),
@@ -518,9 +518,11 @@ ProcessProperties &Process::GetGlobalProperties() {
   return *g_settings_ptr;
 }
 
-void Process::Finalize() {
+void Process::Finalize(bool destructing) {
   if (m_finalizing.exchange(true))
     return;
+  if (destructing)
+    m_destructing.exchange(true);
 
   // Destroy the process. This will call the virtual function DoDestroy under
   // the hood, giving our derived class a chance to do the ncessary tear down.
@@ -1415,7 +1417,13 @@ bool Process::StateChangedIsHijackedForSynchronousResume() {
 StateType Process::GetPrivateState() { return m_private_state.GetValue(); }
 
 void Process::SetPrivateState(StateType new_state) {
-  if (m_finalizing)
+  // Use m_destructing not m_finalizing here.  If we are finalizing a process
+  // that we haven't started tearing down, we'd like to be able to nicely 
+  // detach if asked, but that requires the event system be live.  That will
+  // not be true for an in-the-middle-of-being-destructed Process, since the
+  // event system relies on Process::shared_from_this, which may have already
+  // been destroyed.
+  if (m_destructing)
     return;
 
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process | LLDBLog::Unwind));
diff --git a/lldb/source/Target/ProcessTrace.cpp b/lldb/source/Target/ProcessTrace.cpp
index 061af9e0e520f..376ad7e38c1f9 100644
--- a/lldb/source/Target/ProcessTrace.cpp
+++ b/lldb/source/Target/ProcessTrace.cpp
@@ -50,7 +50,7 @@ ProcessTrace::~ProcessTrace() {
   // make sure all of the broadcaster cleanup goes as planned. If we destruct
   // this class, then Process::~Process() might have problems trying to fully
   // destroy the broadcaster.
-  Finalize();
+  Finalize(true);
 }
 
 void ProcessTrace::DidAttach(ArchSpec &process_arch) {
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 2e8d1dfdaa176..d8a5370664986 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -197,7 +197,7 @@ void Target::DeleteCurrentProcess() {
     if (m_process_sp->IsAlive())
       m_process_sp->Destroy(false);
 
-    m_process_sp->Finalize();
+    m_process_sp->Finalize(false);
 
     CleanupProcess();
 
diff --git a/lldb/test/API/driver/quit_speed/Makefile b/lldb/test/API/driver/quit_speed/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/driver/quit_speed/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/driver/quit_speed/TestQuitWithProcess.py b/lldb/test/API/driver/quit_speed/TestQuitWithProcess.py
new file mode 100644
index 0000000000000..957586d41f6b4
--- /dev/null
+++ b/lldb/test/API/driver/quit_speed/TestQuitWithProcess.py
@@ -0,0 +1,34 @@
+"""
+Test that killing the target while quitting doesn't stall
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import pexpect
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+
+class DriverQuitSpeedTest(PExpectTest):
+    source = "main.c"
+
+    def test_run_quit(self):
+        """Test that the lldb driver's batch mode works correctly."""
+        self.build()
+
+        exe = self.getBuildArtifact("a.out")
+
+        # Turn on auto-confirm removes the wait for the prompt.
+        self.launch(executable=exe, extra_args=["-O", "settings set auto-confirm 1"])
+        child = self.child
+
+        # Launch the process without a TTY so we don't have to interrupt:
+        child.sendline("process launch -n")
+        print("launched process")
+        child.expect("Process ([\d]*) launched:")
+        print("Got launch message")
+        child.sendline("quit")
+        print("sent quit")
+        child.expect(pexpect.EOF, timeout=15)
diff --git a/lldb/test/API/driver/quit_speed/main.c b/lldb/test/API/driver/quit_speed/main.c
new file mode 100644
index 0000000000000..fd041a4fe9d6b
--- /dev/null
+++ b/lldb/test/API/driver/quit_speed/main.c
@@ -0,0 +1,10 @@
+#include <unistd.h>
+
+int 
+main (int argc, char **argv)
+{
+  while(1)
+    usleep(5);
+
+  return 0;
+}

github-actions · 2023-12-07T01:30:56Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff c6dc9cd1fbfcb47aa193f16cb02b97876643e1fe 2f8c9baad5beac622111ed3c81de619002bf877d -- lldb/test/API/driver/quit_speed/main.c lldb/include/lldb/Target/Process.h lldb/source/Core/Debugger.cpp lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp lldb/source/Target/Process.cpp lldb/source/Target/ProcessTrace.cpp lldb/source/Target/Target.cpp

View the diff from clang-format here.

diff --git a/lldb/test/API/driver/quit_speed/main.c b/lldb/test/API/driver/quit_speed/main.c
index 3d6d45ce5e..7daea6fe52 100644
--- a/lldb/test/API/driver/quit_speed/main.c
+++ b/lldb/test/API/driver/quit_speed/main.c
@@ -1,7 +1,7 @@
 #include <unistd.h>
 
-int main (int argc, char **argv) {
-  while(1)
+int main(int argc, char **argv) {
+  while (1)
     usleep(5);
 
   return 0;

medismailben

LGTM!

bulbazord

LGTM, small typo

bulbazord · 2023-12-07T18:22:55Z

lldb/include/lldb/Target/Process.h

@@ -558,7 +558,10 @@ class Process : public std::enable_shared_from_this<Process>,
  ///
  /// Subclasses that override this method should always call this superclass
  /// method.
-  virtual void Finalize();
+  /// If you are running Finalize in your Process subclass Destructor, pass
+  /// \btrue.  If we are in the destructor, shared_from_this will no longer


\btrue -> \b true

clayborg

LGTM

jasonmolenda

lgtm

JDevlieghere

The change itself LGTM, but I'd like to either have an inline comment (e.g. Finalize(/*destructing=*/false)) or use an enum value to convey the meaning of those values.

…ess::Finalize".

jimingham requested a review from JDevlieghere as a code owner December 7, 2023 01:28

llvmbot added the lldb label Dec 7, 2023

jimingham requested review from bulbazord, clayborg and jasonmolenda December 7, 2023 01:29

medismailben approved these changes Dec 7, 2023

View reviewed changes

bulbazord approved these changes Dec 7, 2023

View reviewed changes

Address review & nanny comments.

05f7f2e

clayborg approved these changes Dec 7, 2023

View reviewed changes

jasonmolenda approved these changes Dec 7, 2023

View reviewed changes

JDevlieghere reviewed Dec 7, 2023

View reviewed changes

Add a comment tag for the destructing variable in the uses of "Proc…

2f8c9ba

…ess::Finalize".

jimingham merged commit 9d3aec5 into llvm:main Dec 7, 2023

jimingham deleted the quit-doesnt-stall branch December 7, 2023 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix a stall in running `quit` while a live process is running #74687

Fix a stall in running `quit` while a live process is running #74687

Uh oh!

jimingham commented Dec 7, 2023

Uh oh!

llvmbot commented Dec 7, 2023

Uh oh!

github-actions bot commented Dec 7, 2023 •

edited

Loading

Uh oh!

medismailben left a comment

Uh oh!

bulbazord left a comment

Uh oh!

bulbazord Dec 7, 2023

Uh oh!

clayborg left a comment

Uh oh!

jasonmolenda left a comment

Uh oh!

JDevlieghere left a comment

Uh oh!

Uh oh!

Fix a stall in running quit while a live process is running #74687

Fix a stall in running quit while a live process is running #74687

Uh oh!

Conversation

jimingham commented Dec 7, 2023

Uh oh!

llvmbot commented Dec 7, 2023

Uh oh!

github-actions bot commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medismailben left a comment

Choose a reason for hiding this comment

Uh oh!

bulbazord left a comment

Choose a reason for hiding this comment

Uh oh!

bulbazord Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmolenda left a comment

Choose a reason for hiding this comment

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix a stall in running `quit` while a live process is running #74687

Fix a stall in running `quit` while a live process is running #74687

github-actions bot commented Dec 7, 2023 •

edited

Loading