[PATCH] Cleanup Linux process on detach

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.

Thanks,

Andrew

Linux.cleanup-on-detach.patch (953 Bytes)

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

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?

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

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!

Process-Destroy.patch (2.04 KB)

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

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?

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

I actually only switched them to be consistent with the way those two were already being called in Process::Destroy(). :slight_smile: Thanks!

I actually only switched them to be consistent with the way those two were already being called in Process::Destroy(). :slight_smile: Thanks!

Probably doesn't matter, then :wink:

Jim

Hey Andrew, I was just staging the detach patch from the other thread — does this one supersede the patch from the thread we were on this morning?

Hey Todd, sorry for the confusion. They’re actually two independent issues so this shouldn’t disrupt your testing, this one here is to do with removing breakpoints on detach and the other one I had pinged (that you’re looking at) is to do with detaching from a running (non-stopped) process causing an invalid SIGSTOP to be delivered. I will commit this one now but the other one will still be needed too. Thanks again!

Ok - got it. Thanks!