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

[Andrew MacPherson]

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/95dc324b/attachment.html>