Community
Participate
Working Groups
When we detach from a process we should stop tracking breakpoints. This is mostly for multi-process, but is also applicable for any attach session once we fix Bug 336889. The solution is not as trivial as I thought. The reason is that the IBreakpointTargetDMC is now a Container and a Container has an IProcessDMC as a parent. When we start tracking breakpoints for a process we are about to start, the IProcessDMC does not have the pid yet since the process is not started. However, when we would stop tracking bp, the IProcessDMC does have the pid which make the MIBreakpointManager fail to find the breakpoints because the IBreakpointTargetDMC is not the same as the one we use to track breakpoints.
Created attachment 192324 [details] Prototype overriding equals method So, when setting a bp before the program starts, we don't know the processId. And this bpTargetContext, with the missing procId, gets stored in the MIBreakpointsManager maps and does not match the context use to stop tracking breakpoints, which does contain the procId. In this patch I tried to fix the issue by overriding the equals() method for GDBProcesses_7_0.MIProcessDMC. The idea is that if the procId is empty (""), we consider it equal to any other process id. Basically, we ignore the processId value in the context hierarchy in the case of an empty id. What I noticed is that because equals() is overridden, I would also need to override hashCode(). But how to choose a hashcode for "" in that case? It needs to have the same hashcode as any other possible procId value. The only way to do that is to make each and every hashcode the same. This patch seems to work. I probably should also override equals() and hashcode() for the MIProcesses version of MIProcessDMC too, to be thorough. It is not essential now because we don't allow to detach from a process that we started ourselves, so the issue does not present itself yet. But if we fix bug 306552, the issue would happen. What I like about this solution is that: - if a context with an empty procId is stored anywhere else than the breakpoint service, we would still be covered. As an example, in GDBRunControl_7_0_NS.IsTargetAvailableStep, we use the bpTargetContext as a containerDmc and we used to fail to find this context in the maps of GDBRunControl_7_0_NS because of the missing procId. With this patch, this issue is automatically resolved. - it does not change the API But what I don't like is: - the hashcode being the same for all procIds - the unexpected behavior caused by changing the behavior of equals(). This will be un-intuitive to anyone dealing with it. - the pid is not available from the bpTargetDmc The other solution I'm thinking of is to add a method to MIBreakpointsManager to updateContexts(IBpTargetDmc). As soon as we would have the procId, we would replace the stored bpTargetDmc with the fully-formed one. What is nice about that solution: - The proper context will be used in the vast majority of situations. This seems to protect against other unforeseen situations - the pid will be available from within the bpTargetDmc - easier to understand from someone looking at the code, as it really addresses the problem in a focused way What is not so nice: - new, inelegant API in MIBreakpointsManager to updateContexts - if another service decides to store the containerDmc before the pid is created, we will have the same issue and we'll need a new API to udpateContexts() for that service. Now is when a good face-to-face brainstorm would be nice :-\ I'll keep scratching my head and try to decide what I like best.
When thinking about the implementation of the other proposed solution (adding MIBreakpointsManager.updateContexts(IBpTargetDmc), which would replace an incomplete context with a complete one), I realized that to see if the properly filled IBPTargetDmc actually corresponded to an incomplete one, I would have to start comparing IMIContainerDMContext.getGroupId for the two contexts. This is completely loosing the genericity of using IBPTargetDmc. I really don't like that. Also, the new MIBreakpointsManager.updateContexts(IBpTargetDmc) API is awkward and we'd be somewhat stuck with it, even if it no longer makes sense down the line. So, I'm going cleanup the prototype I posted about the equals() method and go for that solution. It adds no API and therefore can easily be reverted if we find a better solution down the road.
Created attachment 192410 [details] Fix using a wildcard for the processId Here is the final patch using a wildcard for the pId which will match any pId. Committed to HEAD.
*** cdt cvs genie on behalf of mkhouzam *** Bug 336890: Allow IBreakpointTargetDMContext to be matched even if the processId is missing. [*] GDBProcesses.java 1.23 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses.java?root=Tools_Project&r1=1.22&r2=1.23 [*] GDBProcesses_7_2.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_2.java?root=Tools_Project&r1=1.7&r2=1.8 [*] GDBProcesses_7_0.java 1.44 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_0.java?root=Tools_Project&r1=1.43&r2=1.44 [*] MIProcesses.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIProcesses.java?root=Tools_Project&r1=1.12&r2=1.13 [*] GdbLaunch.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/GdbLaunch.java?root=Tools_Project&r1=1.14&r2=1.15
Created attachment 192894 [details] Fix to stopTrackingBreakpoints This patch stops tracking breakpoints whenever a process exists. This means that if we detach from a process, if we kill a process or if a process runs to completion, we stop tracking its breakpoints. I've only done this for GDB > 7.2. This behavior would make sense for all GDB version actually but I didn't do it for two reasons: 1- no real advantage without multi-process 2- if we ever support restart() after a process has exited, no breakpoints would work anymore. Note that for multi-process, this will work because we will support auto-attaching to a new process 3- for GDB 7.0 and GDB 7.1 we run into a problem where sometimes the groupId is known we startTracking breakpoints (when we attach) and sometimes it is not (when we start a process from scratch). That means that the call to stopTrackingBreakpoints would need to be done once with the groupId and if it fails to find anything (because we are in the case where we didn't know the groupId), we the try again with an empty groupId. Committed this fix to HEAD.
Fixed
*** cdt cvs genie on behalf of mkhouzam *** Bug 336890: [multi-process] Should stop tracking breakpoints when a process is no longer being debugged [*] GDBProcesses_7_2.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_2.java?root=Tools_Project&r1=1.10&r2=1.11
