Community
Participate
Working Groups
Created attachment 170340 [details] Skip All breakpoints selected This seems to be caused by a commit referencing bug 79371. If the user has "Skip All Breakpoints" toggled then the breakpoints are disabled asynchronously as part of a MIBreakpointCreatedEvent. The result is they can be hit before being disabled on launch. It's reasonably easy to reproduce: - Create helloworld app - double click in the margin to set a breakpoint at every line - Enable "Skip all breakpoints" in the breakpoint view - launch - The mi log shows that the breakpoint is created and occurs long before the disable command is sent If the breakpoints are physically disabled by deselecting them, then they're disabled as soon as they're set. MILogs attached.
Created attachment 170341 [details] Individually disabled
Another interesting failure case of this async disable on created: while (1) { puts("!!!Hello World!!!"); /* prints !!!Hello World!!! */ } Turn on Skip All Breakpoints and double click in the margin rapidly. Eventually you can get the target to stop at the breakpoint. I think the solution is to disable the breakpoint on creation like we do if the breakpoint marker isn't enabled. That way we don't have this race. There's then no need to disable asynchronously after a created event.
Created attachment 170368 [details] patch 1 Patch for the issue: BreakpointManager: - When the backend CDI breakpoint is created: disable it if !DebugPlugin.getDefault().getBreakpointManager().isEnabled(). This is the same as if the breakpoint marker is disabled. - If disabling the breakpoint on creation, we should mark it as disabled (or we'll be out of sync for re-enabling...) CBreakpointManager: - No need to changeBreakpointPropertiesOnTarget on creation event, as the enablement is now correct. - changeBreakpointProperties, should consider DebugPlugin.getDefault().getBreakpointManager().isEnabled when setting CDI backend breakpoint enablement Mikhail, AFAICS it's entirely redundant to check enablement in: private void CBreakpointManager#changeBreakpointProperties( ICBreakpoint breakpoint, ICDIBreakpoint cdiBreakpoint ) Presumably the condition state is up to date here too...?
Mikhail can you take a look?
Created attachment 170380 [details] additional tidy (In reply to comment #3) > Mikhail, AFAICS it's entirely redundant to check enablement in: > private void CBreakpointManager#changeBreakpointProperties( ICBreakpoint > breakpoint, ICDIBreakpoint cdiBreakpoint ) > Presumably the condition state is up to date here too...? AFAICS as changeBreakpointProperties( ICBreakpoint breakpoint, ICDIBreakpoint cdiBreakpoint ) is only called on the breakpoint created event, and given that enablement and condition are correct at this point, this method is redundant? Am I missing something?
John CVS shows you've done some work on CDI breakpoints, if you could look over these patches for post 7 I'd be grateful.
(In reply to comment #6) > John CVS shows you've done some work on CDI breakpoints, if you could look over > these patches for post 7 I'd be grateful. Does this change reintroduce the issue resolved by bug 79371. If so, then it seems the first step should be having Mikhail evaluate the solution. It seems to me that the problem of breakpoints being hit that shouldn't be is a more serious one than that of sluggishness (79371). But before investing time in reviewing the fix, I'd like Mikhail to chime in.
I don't believe it does reintroduce bug 79371. All breakpoints are inserted at launch anyway. The change in the patch is just to set the enablement appropriately taking into account the global disable switch. This then led to the 'additional tidy' as I can't see why on breakpoint created we need to check whether enablement and condition are correct. Presumably these are always correct after creation... I'll take another look to see if the created event can be fired in some other way, but when I looked, it the additional check seemed superfluous... The patch is in my product, but for CDT this is deferred past 7. Input from you guys would be appreciated!
(In reply to comment #8) > The patch is in my product, but for CDT this is deferred past 7. Input from you > guys would be appreciated! James, I am really busy this week. But I'll try to review it next week.
James, I think it would be better to take into account the 'skip all breakpoints' global state when creating the CDI breakpoint object (in CBreakpointManager.setBreakpointsOnTarget0()). This will provide more consistent behavior. Consider the case where (a) the CDI client doesn't support ICDIBreakpointManagement2; i.e., you can't pass the enable flag when creating the CDI breakpoint (b) the 'skip' state if off (c) the breakpoint is disabled In that case, even with your changes, the breakpoint won't be disabled in the backend until b.setEnabled( icbreakpoint.isEnabled() ); is called in CBreakpointManager. Yet, your changes will provide specialized behavior if the 'skip' state is turned on. It will behave as if the client implemented ICDIBreakpointManagement2, and that inconsistency can only lead to subtle problems or differences in behavior. Again, if you just consider the 'skip' state within CBreakpointManager.setBreakpointsOnTarget0(), everything should just play out nicely and you won't need any of the logic you added in BreakpointManager.
(In reply to comment #10) > James, I think it would be better to take into account the 'skip all > breakpoints' global state when creating the CDI breakpoint object (in > CBreakpointManager.setBreakpointsOnTarget0()). Thanks John! I'm quite inexperienced in this code and easily get lost between debug.core and debug.mi.core (and in the various C*Managers vs *Managers...). Hopefully I'll get a chance to revisit this over the next couple days or so.
Created attachment 172535 [details] patch 2 An updated patch as per John's comment: In CBreakpointManager#setBreakpointsOnTarget0(...) the created CDIBreakpoint has it's initial enablement set to marker.enabled && breakpointManager.isEnabled(). This should then work correctly for all breakpoint manager backends. There are a couple other places which consider the breapoint marker (ICBreakpoint) enablement, reconciling the ICDIbreakpoint enablement appropriately. As marker enablement is orthogonal to Global breakpoint manager enablement, these places need to consider that. Specifically: - #changeBreakpointProperties(IMarkerDelta...) - #changeBreakpointProperties(ICBreakpoint, ICDIBreakpoint) It still feels clunky having created a breakpoint (at startup), that we need to check the enablement is ok. However I can believe that this is needed for handling cli created breakpoints. I've tested: - Staring a debug session with breakpoints globally disabled, then globally enabling - Repeatedly destroying and recreating the breakpoint marker - Repeatedly disabling and re-enabling the breakpoint marker - interrupting the target (with breakpoints globally disabled), creating a new breakpoint with the CLI, and resuming. Target doesn't stop on resume. Correctly stops when the global disable is unset. + other combination of changing the marker, toggling the global enablement, and using the CLI John + Mikhail please take a look if you get a chance
Looks good to me, James.
Committed to HEAD & 7.0.1. Thanks John.
*** cdt cvs genie on behalf of jblackburn *** Bug 314865 - When "Skip All Breakpoints" is ticked breakpoints at launch can be hit [*] CBreakpointManager.java 1.99 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-debug/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/internal/core/CBreakpointManager.java?root=Tools_Project&r1=1.98&r2=1.99 [*] CBreakpointManager.java 1.98.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-debug/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/internal/core/CBreakpointManager.java?root=Tools_Project&r1=1.98&r2=1.98.2.1
