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

[Andrew MacPherson]

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140325/f3975e6c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Process-Destroy.patch
Type: text/x-diff
Size: 2085 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140325/f3975e6c/attachment.patch>