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
