[lldb-dev] [PATCH] Cleanup Linux process on detach

[jingham at apple.com]

Yes, that looks fine.  You switched the order of discarding the thread plans and disabling the breakpoint sites.  I can't think of any reason that would matter, however.

Jim

On Mar 25, 2014, at 12:52 PM, Andrew MacPherson <andrew.macp at gmail.com> wrote:

> There's already a check for m_comm.IsRunning() in ProcessKDP::DisableBreakpointSite() (though it's not as clear as it was in DoDestroy()). Does the patch look ok for commit in that case?
> 
> 
> On Tue, Mar 25, 2014 at 8:42 PM, <jingham at apple.com> wrote:
> If there's a problem it should be fixed by moving the "m_comm.IsRunning()" test into the ProcessKDP::DisableBreakpointSite, I think.  It is silly for everybody to have to do this necessary housekeeping.
> 
> Jim
> 
> On Mar 25, 2014, at 12:40 PM, Andrew MacPherson <andrew.macp at gmail.com> wrote:
> 
> > I've attached a patch here that moves DisableAllBreakpointSites() and m_thread_list.DiscardThreadPlans() into Process::Detach(). This works on Linux however is dependent on what you say about ProcessKDP::DisableBreakpointSite() behaving correctly when called in all circumstances. Before this change the call to DisableAllBreakpointSites() in ProcessKDP was dependent on !m_comm.IsRunning().
> >
> > If you think it's not safe to make this assumption about ProcessKDP I will simply copy those two calls into ProcessLinux::DoDetach() and leave everything else as-is.
> >
> > I was mistaken about DisableBreakpointSite() being a problem, I was seeing that it returns an error from the base Process if unimplemented in a subclass however these errors are ignored by DisableAllBreakpointSites() so there would be no spurious error reported.
> >
> > Thanks!
> >
> >
> > On Tue, Mar 25, 2014 at 6:06 PM, <jingham at apple.com> wrote:
> > The one issue with moving this higher up is that some targets are not interruptible while running (e.g. the Mac OS X Kernel debugging target (KDP).)  So calling DisableAllBreakpointSites can't do its job.  The kernel stub will take care of this when the debugger connection closes, but you need to make sure that you don't block trying to disable breakpoints, which you can't do.  However, as long as DisableBreakpointSite for the KDP side of things does the right thing, it should be fine to call DisableAllBreakpointSites in the Process class Detach before calling DoDetach.  Probably also fine to move clearing the thread plans there as well, that's the other bit of cleanup everybody does.
> >
> > Not sure what you mean about not being able to use DisableAllBreakpointSites, however.  It will call the virtual DisableBreakpointSite, which does do the right thing.
> >
> > Jim
> >
> > On Mar 25, 2014, at 6:25 AM, Andrew MacPherson <andrew.macp at gmail.com> wrote:
> >
> > > Thanks Ed, maybe it should be moved into Process:Detach() in fact? I would think that everyone would want to clear all breakpoint sites before detaching. Though I guess we couldn't use DisableAllBreakpointSites() there because DisableBreakpointSite() in the base Process class just errors out. We could use Target::CleanupProcess() or else just get the BreakpointLists from the Target and call ClearAllBreakpointSites() on them though. What do you think?
> > >
> > >
> > > On Tue, Mar 25, 2014 at 1:53 PM, Ed Maste <emaste at freebsd.org> wrote:
> > > On 25 March 2014 06:36, Andrew MacPherson <andrew.macp at gmail.com> wrote:
> > > > When detaching from a debugged process any breakpoint sites need to be
> > > > cleared before detaching so that they don't generate uncaught SIGTRAPs.
> > > > Target::CleanupProcess() seems to do the necessary cleanup so call this from
> > > > the ProcessLinux::WillDetach() method.
> > > >
> > > > If this is the right fix and if it applies to other OSes as well maybe the
> > > > cleanup call should be moved into an earlier Process class in the hierarchy.
> > >
> > > I fixed a similar issue on FreeBSD in r201724 by calling
> > > DisableAllBreakpointSites() in ProcessFreeBSD::DoDetach, based on
> > > ProcessGDBRemote::DoDetach.  I think you're right that this should be
> > > moved earlier, probably not in individual Process classes at all.
> > >
> > > -Ed
> > >
> > > _______________________________________________
> > > lldb-dev mailing list
> > > lldb-dev at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
> >
> >
> > <Process-Destroy.patch>
> 
>