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
