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

[Andrew MacPherson]

Hey Todd, sorry for the confusion. They're actually two independent issues
so this shouldn't disrupt your testing, this one here is to do with
removing breakpoints on detach and the other one I had pinged (that you're
looking at) is to do with detaching from a running (non-stopped) process
causing an invalid SIGSTOP to be delivered. I will commit this one now but
the other one will still be needed too. Thanks again!


On Tue, Mar 25, 2014 at 9:01 PM, Todd Fiala <tfiala at google.com> wrote:

> Hey Andrew, I was just staging the detach patch from the other thread ---
> does this one supersede the patch from the thread we were on this morning?
>
>
> On Tue, Mar 25, 2014 at 12: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>
>> >
>> >
>>
>> _______________________________________________
>> lldb-dev mailing list
>> lldb-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>>
>
>
>
> --
>  Todd Fiala | Software Engineer |  tfiala at google.com |  650-943-3180
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140325/366df174/attachment.html>