Page MenuHomePhabricator

Delay replacing the process till after we've finalized it
ClosedPublic

Authored by jingham on Dec 12 2018, 6:38 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

jingham created this revision.Dec 12 2018, 6:38 PM
clayborg added inline comments.Dec 13 2018, 9:41 AM
source/Target/Target.cpp
2867–2880 ↗(On Diff #177984)

All this weak pointer code is no longer needed now? This code:

// Get a weak pointer to the previous process if we have one
ProcessWP process_wp;
if (m_process_sp)
  process_wp = m_process_sp;



// 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
// much as it can without the object being destroyed. We try to lock the
// shared pointer and if that works, then someone else still has a strong
// reference to the process.

ProcessSP old_process_sp(process_wp.lock());
if (old_process_sp)
  old_process_sp->Finalize();

Will just become:

if (m_process_sp)
  m_process_sp->Finalize();

The original thinking was that Process::Finalize() was just an optional call that could be made in case a process does not go away to reduce the memory that it owned in case it sticks around forever and also to mark the process in a way that would stop it from doing any future work (don't try to fetch thread lists, etc). Has Finalize changed to something that is required now? Are we doing work that must be done in order to properly shut down a process? That wasn't the original intention IIRC. "svn blame" was no help as all this code was there prior the great reformatting of code.

Yeah, I'll simplify the logic after this change.

I've had mysterious crashes that I've never been able to reproduce for quite some time now. Things like we go to tear down a process, the thread is deleting its thread plans, a thread plan goes to remove one of it's breakpoints and crashes because Thread::GetTarget returns a bogus target. Then a little while ago I got one test in the test suite to crash this way (on my machine for about a day and only when I debugged it lightly...) I figured out that the problem was that the Thread relies on the ProcessWP -> ProcessSP to get the target. I am pretty sure this is an infrequent and racy crash because generally somebody else is holding onto a shared pointer to the process, so m_process_sp is not the last reference. But if it is, the ThreadPlans can no longer get to the target.

I thought a bit about having the thread plans handle GetTarget being fallible, but they really do have to have a way to get to the Target when being deleted or they will leave stray random breakpoints in the target that will be inserted on rerun. That would be no good.

This change patches over the problem for now. It will make sure that Finalize gets called before destruction.

But longer term we either need to require Finalize before destruction, or we need to make a more robust way for Threads to get back to their Targets.

jingham updated this revision to Diff 178306.Dec 14 2018, 3:24 PM

This is better. Instead of just finalizing, I call DeleteCurrentProcess. All the plugins that implement DebugProcess actually call DeleteCurrentProcess, so this just makes sure it happens before we drop our (potentially last) reference to the process sp.

clayborg accepted this revision.Dec 17 2018, 9:15 AM
This revision is now accepted and ready to land.Dec 17 2018, 9:15 AM
This revision was automatically updated to reflect the committed changes.