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

[Andrew MacPherson]

I actually only switched them to be consistent with the way those two were
already being called in Process::Destroy(). :) Thanks!


On Tue, Mar 25, 2014 at 8:57 PM, <jingham at apple.com> wrote:

> 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>
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140325/ad097822/attachment.html>