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
