From 9d3aec5535adfdeb10a400e92cecc1cc0a5e26a6 Mon Sep 17 00:00:00 2001 From: jimingham Date: Thu, 7 Dec 2023 14:36:27 -0800 Subject: Fix a stall in running `quit` while a live process is running (#74687) 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. --- lldb/include/lldb/Target/Process.h | 10 ++++++- lldb/source/Core/Debugger.cpp | 2 +- .../Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp | 2 +- .../Plugins/Process/elf-core/ProcessElfCore.cpp | 2 +- .../Process/gdb-remote/ProcessGDBRemote.cpp | 2 +- .../Plugins/Process/mach-core/ProcessMachCore.cpp | 2 +- .../Plugins/Process/minidump/ProcessMinidump.cpp | 2 +- .../Plugins/Process/scripted/ScriptedProcess.cpp | 2 +- lldb/source/Target/Process.cpp | 14 +++++++-- lldb/source/Target/ProcessTrace.cpp | 2 +- lldb/source/Target/Target.cpp | 2 +- lldb/test/API/driver/quit_speed/Makefile | 3 ++ .../API/driver/quit_speed/TestQuitWithProcess.py | 34 ++++++++++++++++++++++ lldb/test/API/driver/quit_speed/main.c | 8 +++++ 14 files changed, 74 insertions(+), 13 deletions(-) create mode 100644 lldb/test/API/driver/quit_speed/Makefile create mode 100644 lldb/test/API/driver/quit_speed/TestQuitWithProcess.py create mode 100644 lldb/test/API/driver/quit_speed/main.c diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h index 4646e30..24c599e 100644 --- a/lldb/include/lldb/Target/Process.h +++ b/lldb/include/lldb/Target/Process.h @@ -558,7 +558,10 @@ public: /// /// 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 + /// \b true. 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 @@ protected: /// 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 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 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 21f71e4..398265d 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 /* not destructing */); target_sp->Destroy(); } } diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp index 70bb9aa..de739ac 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 /* destructing */); } 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 aedc43a..a4540de 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 /* destructing */); } 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 b043197..d5e557b 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 /* destructing */); // 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 9830a4b..a2ea193 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 /* destructing */); } 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 0d5ca42..b72307c 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 /* destructing */); } void ProcessMinidump::Initialize() { diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp index 54b3677..66f8613 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 /* destructing */); } void ScriptedProcess::Initialize() { diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 2d77144..aa3b04c 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 061af9e..6e5ef6a 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 /* destructing */); } void ProcessTrace::DidAttach(ArchSpec &process_arch) { diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 2e8d1df..302c2ba 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 /* not destructing */); CleanupProcess(); diff --git a/lldb/test/API/driver/quit_speed/Makefile b/lldb/test/API/driver/quit_speed/Makefile new file mode 100644 index 0000000..1049594 --- /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 0000000..957586d --- /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 0000000..3d6d45c --- /dev/null +++ b/lldb/test/API/driver/quit_speed/main.c @@ -0,0 +1,8 @@ +#include + +int main (int argc, char **argv) { + while(1) + usleep(5); + + return 0; +} -- cgit v1.1