[Lldb-commits] [PATCH] D55631: Delay replacing the process till after we've finalized it

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added a reviewer: clayborg.
Herald added subscribers: lldb-commits, abidh.

Because of the way we use weak pointers in Process, it is not safe to just destroy a fully live Process object.  For instance, because the Thread holds a ProcessWP, if the Process gets destroyed as a result of its SharedPointer count going to 0, the thread can't get back to it's Process anymore (the WP->SP conversion fails), and since it gets to the Target from the Process, it can't get its target either.

This is structurally weak, but we've worked around it so far by making sure we call Finalize on the Process before we allow it to be destroyed.  Finalize clears out all of the objects Process owns, and then the eventual destruction can be just reclamation of memory.

However, we shot ourselves in the foot in Target::Launch by storing away a ProcessWP for the previous process owned by the target, then setting our m_process_sp to the new process THEN reconstituting the ProcessWP and calling Finalize on it.  But if Target was the last holder of the old ProcessSP, then when we set m_process_sp to the new process, that would trigger the destruction of the old Process BEFORE finalizing it.  Tearing down the Process before finalizing it doesn't always crash, it depends on what work the process was doing at the time.  But sometimes it does crash.

This patch avoids this problem by first finalizing the old process, THEN resetting the shared pointer.  That way we know Finalize will happen before destruction.

This is a NFC commit, it only fixes a fairly obscure crash.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55631

Files:
  source/Target/Target.cpp


Index: source/Target/Target.cpp
===================================================================
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -2868,8 +2868,6 @@
     ProcessWP process_wp;
     if (m_process_sp)
       process_wp = m_process_sp;
-    m_process_sp =
-        GetPlatform()->DebugProcess(launch_info, debugger, this, error);
 
     // Cleanup the old process since someone might still have a strong
     // reference to this process and we would like to allow it to cleanup as
@@ -2880,6 +2878,10 @@
     ProcessSP old_process_sp(process_wp.lock());
     if (old_process_sp)
       old_process_sp->Finalize();
+
+    m_process_sp =
+        GetPlatform()->DebugProcess(launch_info, debugger, this, error);
+
   } else {
     if (log)
       log->Printf("Target::%s the platform doesn't know how to debug a "


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55631.177984.patch
Type: text/x-patch
Size: 841 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20181213/de9d1533/attachment.bin>