Bugzilla – Full Text Bug Listing
| Summary: | [breakpoints] Target filter is not working when unchecking the entire target | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Nobody - feel free to take it <nobody> | ||||||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | Nobody - feel free to take it <nobody> | ||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | cdtdoug, malaperle, pawel.1.piech | ||||||||||||
| Version: | 8.0 | Flags: | nobody:
iplog-
marc.khouzam: review+ |
||||||||||||
| Target Milestone: | 8.0.2 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows 7 | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Nobody - feel free to take it
Interesting. In MIBreakpointsManager.extractThreads() we don't differentiate between the case where the breakpoint applies to all threads versus when it applies to no threads. But when I tried to distinguish those cases, other things broke. I think this is the first time the MIBreakpointsManager has a case where a breakpoint is created on the target but needs to be removed, without actually going through an actual breakpoint deletion. I would like to clarify what we think the user expects by unchecking all threads in the filter. I guess the expected behavior is that the breakpoint will never, ever hit. Isn't this like disabling the bp? Maybe we can offer a distinct behavior instead. If all threads are unselected, then the breakpoint will not hit those threads but will hit any new threads. But this would then need to be extended to the case when a subset of threads is selected; are new threads automatically selected or not? Right now, they are not. How about an enhancement bug to support the option of including/excluding new threads when doing filtering? (In reply to comment #1) > Interesting. In MIBreakpointsManager.extractThreads() we don't differentiate > between the case where the breakpoint applies to all threads versus when it > applies to no threads. > Unchecking all threads in the properties dialog will automatically uncheck the target. I don't think we can differentiate these two cases without changing the current UI. Created attachment 203283 [details] Differentiating between all threads vs no threads (not working) (In reply to comment #2) > (In reply to comment #1) > > Interesting. In MIBreakpointsManager.extractThreads() we don't differentiate > > between the case where the breakpoint applies to all threads versus when it > > applies to no threads. > > > Unchecking all threads in the properties dialog will automatically uncheck the > target. I don't think we can differentiate these two cases without changing the > current UI. Sorry, I wasn't clear. I mean that in the actual code, we don't differentiate the two cases. This patch shows what we could do. But things don't work with this patch because the MIBreakpointsManager wasn't ready for this case. Probably some minor tweaks needed. As for the enhancement of including new threads or not, we would indeed need a new UI button or something. (In reply to comment #3) > Sorry, I wasn't clear. I mean that in the actual code, we don't differentiate > the two cases. This patch shows what we could do. But things don't work with > this patch because the MIBreakpointsManager wasn't ready for this case. > Probably some minor tweaks needed. > > As for the enhancement of including new threads or not, we would indeed need a > new UI button or something. I have a patch for MIBreakpointManager that checks whether the target is filtered or not and installs or deletes the target breakpoint accordingly. I just need to more testing before submitting it. I'll adjust it with your patch. Created attachment 203310 [details]
Fix for target filtering.
This patch adds a simple support for target filtering. When the target or all threads are unchecked for a platform breakpoint the corresponding target breakpoint is deleted. Checking the target installs the breakpoint on the target.
Marc, I tried your patch but there are problems with it. The attached patch is very simple, it uses 'IDsfBreakpointExtension.getTargetFilters()', so it wouldn't affect your proposal to differentiate between all threads and no threads. The target filters are not persistent - don't know what to persist. Please let me know what you think. (In reply to comment #6) > Marc, I tried your patch but there are problems with it. > The attached patch is very simple, it uses > 'IDsfBreakpointExtension.getTargetFilters()', so it wouldn't affect your > proposal to differentiate between all threads and no threads. The target > filters are not persistent - don't know what to persist. > Please let me know what you think. I think your solution is safer than mine because it explicitly uninstall a breakpoint if it is filtered instead of relying on a side effect of no new bp being created because there are no threads it applies to. I found a couple of cases not covered: - create a bp - filter on some threads but not all - filter the entire bp - unfilter the bp, or filter on some but not all threads => Invalid bp error I haven't looked into how to fix this. It is probably an excessive safety check that needs to be removed. - disable a bp - launch a session - filter the entire bp => NPE To fix the second one, you can push your check !filtered inside inside the block that checked if the bp was installed, to give: if (!filtered && bpEnabled) I think this is easier to understand too. One little nit: ther terminology 'filtered' is confusing to me; I felt like a bp that applied to some but not all threads would still be considered filtered. Can we use isBreakpointEntirelyFiltered() instead of isBreakpointFiltered()? I think a method called isBreakpointFiltered() should return true if any thread is unselected. (In reply to comment #7) Thanks for reviewing the patch. I'll fix the cases you mentioned and rename the method. (In reply to comment #7) > - disable a bp > - launch a session > - filter the entire bp > => NPE I can't reproduce this case, everything works fine. Please confirm the steps. (In reply to comment #7) > - create a bp > - filter on some threads but not all > - filter the entire bp > - unfilter the bp, or filter on some but not all threads > => Invalid bp error In this case the corresponding map in 'MIBreakpointsManager.fTargetBPs' is not updated when a thread is filtered. New thread breakpoints are not put in the map and the old target breakpoint is not deleted. This is not a bug caused by my patch, but it is only visible for target filtering because the filtered target breakpoint remains in the map. (In reply to comment #9) > (In reply to comment #7) > > - disable a bp > > - launch a session > > - filter the entire bp > > => NPE > > I can't reproduce this case, everything works fine. Please confirm the steps. I create a new breakpoint then disable it. Then I launch a DSF-GDB session (it was non-stop and GDB 7.3), with more than one thread (5 in my case), I resume until another breakpoint until all 5 threads are created. I go to the disabled bp and filter all threads, and I get: java.lang.NullPointerException at org.eclipse.cdt.dsf.mi.service.MIBreakpointsManager.determineAttributesDelta(MIBreakpointsManager.java:1612) at org.eclipse.cdt.dsf.mi.service.MIBreakpointsManager.modifyBreakpoint(MIBreakpointsManager.java:902) at org.eclipse.cdt.dsf.mi.service.MIBreakpointsManager.access$16(MIBreakpointsManager.java:833) at org.eclipse.cdt.dsf.mi.service.MIBreakpointsManager$21$2.handleSuccess(MIBreakpointsManager.java:1254) at org.eclipse.cdt.dsf.concurrent.RequestMonitor.handleCompleted(RequestMonitor.java:374) at org.eclipse.cdt.dsf.concurrent.RequestMonitor$2.run(RequestMonitor.java:301) at org.eclipse.cdt.dsf.concurrent.DefaultDsfExecutor$TracingWrapperRunnable.run(DefaultDsfExecutor.java:371) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441) at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303) at java.util.concurrent.FutureTask.run(FutureTask.java:138) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:98) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:206) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908) at java.lang.Thread.run(Thread.java:662) Created attachment 203429 [details]
Proposed fix for case 1.
I created a separate patch for case 1 because the problem is not caused by the target filtering. (In reply to comment #10) > (In reply to comment #7) > > - create a bp > > - filter on some threads but not all > > - filter the entire bp > > - unfilter the bp, or filter on some but not all threads > > => Invalid bp error > > In this case the corresponding map in 'MIBreakpointsManager.fTargetBPs' is not > updated when a thread is filtered. New thread breakpoints are not put in the > map and the old target breakpoint is not deleted. > This is not a bug caused by my patch, but it is only visible for target > filtering because the filtered target breakpoint remains in the map. Good find! I guess this bug will happen whenever we called reinstallBreakpoint(), like when changing the line of a bp through its properties. The fix looks good. Thanks! Created attachment 203452 [details]
Fix for target filtering, includes fixes for cases 1 and 2.
Marc, please take a look at the latest patch. If it's OK, I'll commit it. (In reply to comment #16) > Marc, please take a look at the latest patch. If it's OK, I'll commit it. Looks good to me. I think the line: if (filtered && (breakpointIDs.containsKey(breakpoint) || targetBPs.containsValue(breakpoint))) { could be simply if (filtered) { since we checked above for !breakpointIDs.containsKey(breakpoint) && !targetBPs.containsValue(breakpoint)) but it does not really matter. Created attachment 203455 [details]
Final patch.
*** cdt git genie on behalf of Mikhail Khodjaiants ***
Bug 357515 - [breakpoints] Target filter is not working when unchecking
the entire target.
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=1f971379691d4b19285fd8ada25959e2c146e4ab
Committed to the HEAD. Marc, the review is just a formality since you already reviewed it :). Is 'Next' the right target milestone? Should I commit it to 8.0.x as well? (In reply to comment #20) > Committed to the HEAD. > Marc, the review is just a formality since you already reviewed it :). Is > 'Next' the right target milestone? Should I commit it to 8.0.x as well? 'Next' is right when checking only into master. I think this fix should go into the CDT_8_0 branch as well, which would make the target 8.0.2. Thanks! Thanks Marc. Committed to 8.0.2. *** cdt git genie on behalf of Mikhail Khodjaiants ***
Bug 357515 - [breakpoints] Target filter is not working when unchecking
the entire target.
[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=e3bb6338ba518988b37cb7211723fa079c62e6e8
Verified in 8.0.2 RC1. |