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

[Todd Fiala]

Ok - got it.  Thanks!


On Tue, Mar 25, 2014 at 1:05 PM, Andrew MacPherson <andrew.macp at gmail.com>wrote:

> 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
>>
>
>


-- 
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/617a2044/attachment.html>