<div dir="ltr">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?<br></div><div class="gmail_extra">

<br><br><div class="gmail_quote">On Tue, Mar 25, 2014 at 8:42 PM,  <span dir="ltr"><<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

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

<br>
Jim<br>
<div><div class="h5"><br>
On Mar 25, 2014, at 12:40 PM, Andrew MacPherson <<a href="mailto:andrew.macp@gmail.com">andrew.macp@gmail.com</a>> wrote:<br>
<br>
> 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().<br>


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


><br>
> Thanks!<br>
><br>
><br>
> On Tue, Mar 25, 2014 at 6:06 PM, <<a href="mailto:jingham@apple.com">jingham@apple.com</a>> wrote:<br>
> 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.<br>


><br>
> 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.<br>
><br>
> Jim<br>
><br>
> On Mar 25, 2014, at 6:25 AM, Andrew MacPherson <<a href="mailto:andrew.macp@gmail.com">andrew.macp@gmail.com</a>> wrote:<br>
><br>
> > 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?<br>


> ><br>
> ><br>
> > On Tue, Mar 25, 2014 at 1:53 PM, Ed Maste <<a href="mailto:emaste@freebsd.org">emaste@freebsd.org</a>> wrote:<br>
> > On 25 March 2014 06:36, Andrew MacPherson <<a href="mailto:andrew.macp@gmail.com">andrew.macp@gmail.com</a>> wrote:<br>
> > > When detaching from a debugged process any breakpoint sites need to be<br>
> > > cleared before detaching so that they don't generate uncaught SIGTRAPs.<br>
> > > Target::CleanupProcess() seems to do the necessary cleanup so call this from<br>
> > > the ProcessLinux::WillDetach() method.<br>
> > ><br>
> > > If this is the right fix and if it applies to other OSes as well maybe the<br>
> > > cleanup call should be moved into an earlier Process class in the hierarchy.<br>
> ><br>
> > I fixed a similar issue on FreeBSD in r201724 by calling<br>
> > DisableAllBreakpointSites() in ProcessFreeBSD::DoDetach, based on<br>
> > ProcessGDBRemote::DoDetach.  I think you're right that this should be<br>
> > moved earlier, probably not in individual Process classes at all.<br>
> ><br>
> > -Ed<br>
> ><br>
> > _______________________________________________<br>
> > lldb-dev mailing list<br>
> > <a href="mailto:lldb-dev@cs.uiuc.edu">lldb-dev@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev</a><br>
><br>
><br>
</div></div>> <Process-Destroy.patch><br>
<br>
</blockquote></div><br></div>