[lldb] Simplify the is_finalized logic in process and make it thread safe.

[JDevlieghere]

Summary

This is a speculative fix when looking at the finalization code in Process. It tackles the following issues:

• Adds synchronization to prevent races between threads.
• Marks the process as finalized/invalid as soon as Finalize is called rather than at the end.
• Simplifies the code by using only a single instance variable to track finalization.

[jingham]

LGTM. It looks like we never made use of the distinction between "started to finalize" and "done finalizing", so just marking it at the start of finalization seems fine.

I quibble a bit with "m_finalized" because when it gets set to true we really are starting to finalize, we aren't done finalizing. And it's a little weird to have tests for "am I shutting down", if so don't do X. You might wonder, if we're really all the way done, who would ever ask me this question? I think m_finalizing is a more accurate name. But I don't feel strongly about this.

lldb/source/Target/Process.cpp

1568–1569

I don't think that comment refers to anything still in the code... Might as well ax it here.

[shafik]

lldb/include/lldb/Target/Process.h

2837

std::atomic<bool> m_finalizing{false};

honestly the whole thing should be refactored, the mem-initializer list is kind of ridiculous.

[JDevlieghere]

Thanks Jim, I addressed both issues in the commit.

[aharries-upmem]

Hi all,

Apologies for being the bearer of bad news, but I believe that this patch breaks our[1] (downstream) lldb, by introducing a deadlock when a process is killed by a parent debugging process. Specifically, I believe that this patch causes a process to fail to exit, which causes a later deadlock when a listener waits for the process to exit.

I'm in the process of trying to produce some convincing output from our backend so that I can properly file a bug, but in the meantime I wanted to raise this here in case anyone has any comments or thoughts on this. Although I managed to narrow the issue down to this commit through a git bisect, I am not confident in this patch being the *cause* of the deadlock, as it may be the case that we are relying on incorrect behaviour in lldb that this patch fixes.

[1]: https://github.com/upmem/llvm-project/tree/upmem_release_120

[jingham]

I wonder if instead of doing:

// Use our target to get a shared pointer to ourselves...
if (m_finalize_called && !PrivateStateThreadIsValid())
  BroadcastEvent(event_sp);
else
  m_private_state_broadcaster.BroadcastEvent(event_sp);

->

m_private_state_broadcaster.BroadcastEvent(event_sp);

we should have just replaced m_finalize_called with m_finalizing? If you tried to sent the exited event to the private event broadcaster after it was shut down, that event would never get to the public process event queue.

That's the only part of the patch that seems a little suspect to me.

Can you try making that change and see if things go better?

Jim

[aharries-upmem]

In D93479#2839076, @jingham wrote:

I wonder if instead of doing:

// Use our target to get a shared pointer to ourselves...
if (m_finalize_called && !PrivateStateThreadIsValid())
  BroadcastEvent(event_sp);
else
  m_private_state_broadcaster.BroadcastEvent(event_sp);

->

m_private_state_broadcaster.BroadcastEvent(event_sp);

we should have just replaced m_finalize_called with m_finalizing? If you tried to sent the exited event to the private event broadcaster after it was shut down, that event would never get to the public process event queue.

Hi Jim, I've tried reverting this check and replacing m_finalize_called with m_finalizing, but sadly the code still seems to deadlock.

I'm also a little suspicious of (in Process::Finalize):

if (m_finalizing.exchange(true))
  return;

rather than

m_finalize_called = true;

As it would seem (to me) to introduce the possibility of an early exit where there wasn't one before. I don't know if that was intended (to avoid things being done twice?), but it's the only other place I can see a clear change in control flow.

[aharries-upmem]

In D93479#2840497, @aharries-upmem wrote:

I'm also a little suspicious of (in Process::Finalize):

if (m_finalizing.exchange(true))
  return;

rather than

m_finalize_called = true;

I've also tried replacing this with m_finalizing.exchange(true);, which also (sadly) doesn't seem to stop the deadlock.

[jingham]

Huh... I fear you are going to have to debug this further on your end. I can't see anything else suspect in this patch.

[aharries-upmem]

In D93479#2841049, @jingham wrote:

Huh... I fear you are going to have to debug this further on your end. I can't see anything else suspect in this patch.

Agreed. Sadly I need to prioritise another task in the short term, but I think I'm going to try slowly re-implementing the changes in the patch to see I can find a minimal change that triggers it when I have time.

[jingham]

In D93479#2850104, @aharries-upmem wrote:

In D93479#2841049, @jingham wrote:

Huh... I fear you are going to have to debug this further on your end. I can't see anything else suspect in this patch.

Agreed. Sadly I need to prioritise another task in the short term, but I think I'm going to try slowly re-implementing the changes in the patch to see I can find a minimal change that triggers it when I have time.

Don't hesitate to reach out for questions, explanations, "what the heck"-s once you get back to this.

[aharries-upmem]

! In D93479#2850445, @jingham wrote:
Don't hesitate to reach out for questions, explanations, "what the heck"-s once you get back to this.

Thanks Jim, that's very kind of you!

lldb/include/lldb/Target/Process.h

/// UnregisterNotificationCallbacks (const Notifications&)
   /// method.
   virtual void Finalize();
 
   /// Return whether this object is valid (i.e. has not been finalized.)
   ///
   /// \return
   ///     Returns \b true if this Process has not been finalized
   ///     and \b false otherwise.
-  bool IsValid() const { return !m_finalize_called; }
+  bool IsValid() const { return !m_finalizing; }
 
   /// Return a multi-word command object that can be used to expose plug-in
   /// specific commands.
   ///
   /// This object will be used to resolve plug-in commands and can be
   /// triggered by a call to:
   ///
   ///     (lldb) process command <args>
protected:
   std::vector<PreResumeCallbackAndBaton> m_pre_resume_actions;
   ProcessRunLock m_public_run_lock;
   ProcessRunLock m_private_run_lock;
   bool m_currently_handling_do_on_removals;
   bool m_resume_requested; // If m_currently_handling_event or
                            // m_currently_handling_do_on_removals are true,
                            // Resume will only request a resume, using this
                            // flag to check.
-  bool m_finalizing; // This is set at the beginning of Process::Finalize() to
-                     // stop functions from looking up or creating things
-                     // during a finalize call
-  bool m_finalize_called; // This is set at the end of Process::Finalize()
+
+  /// 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;
+
   bool m_clear_thread_plans_on_stop;
   bool m_force_next_event_delivery;
   lldb::StateType m_last_broadcast_state; /// This helps with the Public event
                                           /// coalescing in
                                           /// ShouldBroadcastEvent.
   std::map<lldb::addr_t, lldb::addr_t> m_resolved_indirect_addresses;
   bool m_destroy_in_process;
   bool m_can_interpret_function_calls;  // Some targets, e.g the OSX kernel,
protected:
 
   Status StopForDestroyOrDetach(lldb::EventSP &exit_event_sp);
 
   virtual Status UpdateAutomaticSignalFiltering();
 
   void LoadOperatingSystemPlugin(bool flush);
 
 private:
+  Status DestroyImpl(bool force_kill);
+
   /// This is the part of the event handling that for a process event. It
   /// decides what to do with the event and returns true if the event needs to
   /// be propagated to the user, and false otherwise. If the event is not
   /// propagated, this call will most likely set the target to executing
   /// again. There is only one place where this call should be called,
   /// HandlePrivateEvent. Don't call it from anywhere else...
   ///
   /// \param[in] event_ptr

lldb/source/Target/Process.cpp

: ProcessProperties(this),
       m_notifications(), m_image_tokens(), m_listener_sp(listener_sp),
       m_breakpoint_site_list(), m_dynamic_checkers_up(),
       m_unix_signals_sp(unix_signals_sp), m_abi_sp(), m_process_input_reader(),
       m_stdio_communication("process.stdio"), m_stdio_communication_mutex(),
       m_stdin_forward(false), m_stdout_data(), m_stderr_data(),
       m_profile_data_comm_mutex(), m_profile_data(), m_iohandler_sync(0),
       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_finalizing(false), m_finalize_called(false),
+      m_private_run_lock(), m_finalizing(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_warnings_issued(),
       m_run_thread_plan_lock(), m_can_jit(eCanJITDontKnow) {
   CheckInWithManager();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
   LLDB_LOGF(log, "%p Process::Process()", static_cast<void *>(this));
const ProcessPropertiesSP &Process::GetGlobalProperties() {
   // NOTE: intentional leak so we don't crash if global destructor chain gets
   // called as other threads still use the result of this function
   static ProcessPropertiesSP *g_settings_sp_ptr =
       new ProcessPropertiesSP(new ProcessProperties(nullptr));
   return *g_settings_sp_ptr;
 }
 
 void Process::Finalize() {
-  m_finalizing = true;
+  if (m_finalizing.exchange(true))
+    return;
 
   // Destroy this process if needed
   switch (GetPrivateState()) {
   case eStateConnected:
   case eStateAttaching:
   case eStateLaunching:
   case eStateStopped:
   case eStateRunning:
   case eStateStepping:
   case eStateCrashed:
   case eStateSuspended:
-    Destroy(false);
+    DestroyImpl(false);
     break;
 
   case eStateInvalid:
   case eStateUnloaded:
   case eStateDetached:
   case eStateExited:
     break;
   }
void Process::Finalize() {
   // contain events that have ProcessSP values in them which can keep this
   // process around forever. These events need to be cleared out.
   m_private_state_listener_sp->Clear();
   m_public_run_lock.TrySetRunning(); // This will do nothing if already locked
   m_public_run_lock.SetStopped();
   m_private_run_lock.TrySetRunning(); // This will do nothing if already locked
   m_private_run_lock.SetStopped();
   m_structured_data_plugin_map.clear();
-  m_finalize_called = true;
 }
 
 void Process::RegisterNotificationCallbacks(const Notifications &callbacks) {
   m_notifications.push_back(callbacks);
   if (callbacks.initialize != nullptr)
     callbacks.initialize(callbacks.baton, this);
 }
 
if (hijacking_name &&
       return true;
   }
   return false;
 }
 
 StateType Process::GetPrivateState() { return m_private_state.GetValue(); }
 
 void Process::SetPrivateState(StateType new_state) {
-  if (m_finalize_called)
+  if (m_finalizing)
     return;
 
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STATE |
                                                   LIBLLDB_LOG_PROCESS));
   bool state_changed = false;
 
   LLDB_LOGF(log, "Process::SetPrivateState (%s)", StateAsCString(new_state));
 
if (StateIsStoppedState(new_state, false)) {
 
       m_mod_id.BumpStopID();
       if (!m_mod_id.IsLastResumeForUserExpression())
         m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
       m_memory_cache.Clear();
       LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
                 StateAsCString(new_state), m_mod_id.GetStopID());
     }
 
-    // Use our target to get a shared pointer to ourselves...
-    if (m_finalize_called && !PrivateStateThreadIsValid())
-      BroadcastEvent(event_sp);
-    else
-      m_private_state_broadcaster.BroadcastEvent(event_sp);
   } else {
     LLDB_LOGF(log,
               "Process::SetPrivateState (%s) state didn't change. Ignoring...",
               StateAsCString(new_state));
   }
 }
 
 void Process::SetRunningUserExpression(bool on) {
Status Process::Detach(bool keep_stopped) {
 
   m_public_run_lock.SetStopped();
   return error;
 }
 
 Status Process::Destroy(bool force_kill) {
   // If we've already called Process::Finalize then there's nothing useful to
   // be done here.  Finalize has actually called Destroy already.
-  if (m_finalize_called)
+  if (m_finalizing)
     return {};
+  return DestroyImpl(force_kill);
+}
 
+Status Process::DestroyImpl(bool force_kill) {
   // Tell ourselves we are in the process of destroying the process, so that we
   // don't do any unnecessary work that might hinder the destruction.  Remember
   // to set this back to false when we are done.  That way if the attempt
   // failed and the process stays around for some reason it won't be in a
   // confused state.
 
   if (force_kill)
     m_should_detach = false;