On Mar 25, 2014, at 12:59 PM, Andrew MacPherson <[email protected]> wrote:
> I actually only switched them to be consistent with the way those two were > already being called in Process::Destroy(). :) Thanks! Probably doesn't matter, then ;-) Jim > > > On Tue, Mar 25, 2014 at 8:57 PM, <[email protected]> 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 <[email protected]> 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, <[email protected]> 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 <[email protected]> > > 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, <[email protected]> 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 <[email protected]> > > > 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 <[email protected]> wrote: > > > > On 25 March 2014 06:36, Andrew MacPherson <[email protected]> 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 > > > > [email protected] > > > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev > > > > > > > > > <Process-Destroy.patch> > > > > > > _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
