Community
Participate
Working Groups
When "Show Breakpoints Supported by Selected Target" option is set, the Breakpoints view should only show the breakpoints installed on the target associated with the current debug context.
Created attachment 205071 [details] Proposed fix. This patch filters the breakpoints in the view when the "Show Breakpoints Supported by Selected Target" is set by showing the breakpoints installed in 'IBreakpointsTargetDMContext' acquired from the current debug context.
Cool! I never tried this feature before. I also noticed it does not work with CDI either. I tried the patch (very quickly) and it looks nice. I did get an NPE when trying to switch contexts with multiple launches, but I haven't really looked yet if it is my env causing it: Caused by: java.lang.NullPointerException at org.eclipse.cdt.dsf.ui.viewmodel.DefaultVMModelProxyStrategy$12.handleSuccess(DefaultVMModelProxyStrategy.java:780) 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.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135)
Mikhail, I remember thinking the same when I first looked at the action many years ago. But what I look further into it, I found out it wasn't meant to behave in that way. The idea was to show only breakpoints that the active debug context *could* support, not to show breakpoints actually installed on the active debug context. E.g., if you have an Eclipse instance where you're doing both Java and C/C++ development, the breakpoints view would, by default, show all breakpoints you have defined--both Java and C/C++ ones. Now, when you're actually debugging something, the idea was to allow the user to tell the breakpoints view to show only Java or C/C++ breakpoints, but not both. The action was not meant to filter out, e.g., breakpoints C/C++ breakpoints when you have two CDT debug sessions going. How exactly I came to this conclusion, I don't remember. Maybe it was based on looking at the implementation, or maybe I asked Darin at the time. I just wanted to point out my understanding of the action's intent. Note the key words in the action label, though..."supported by", not "installed on".
(In reply to comment #3) > Mikhail, I remember thinking the same when I first looked at the action many > years ago. But what I look further into it, I found out it wasn't meant to > behave in that way. The idea was to show only breakpoints that the active debug > context *could* support, not to show breakpoints actually installed on the > active debug context. E.g., if you have an Eclipse instance where you're doing > both Java and C/C++ development, the breakpoints view would, by default, show > all breakpoints you have defined--both Java and C/C++ ones. Now, when you're > actually debugging something, the idea was to allow the user to tell the > breakpoints view to show only Java or C/C++ breakpoints, but not both. The > action was not meant to filter out, e.g., breakpoints C/C++ breakpoints when > you have two CDT debug sessions going. > > How exactly I came to this conclusion, I don't remember. Maybe it was based on > looking at the implementation, or maybe I asked Darin at the time. I just > wanted to point out my understanding of the action's intent. Note the key words > in the action label, though..."supported by", not "installed on". John, I think you are right. But it is confusing for customers who normally work with only one language. The issue was reported by our QA team. They are focused on the C/C++ development and may not even know that Java or other languages can share the same views. At the same time the ability to synchronize the breakpoints view with the debug context is more useful feature, I would rather have an option for "installed" than "supported". It still can be done by adding a new "Synchronize" option to the view.
(In reply to comment #4) > John, I think you are right. But it is confusing for customers who normally > work with only one language. The issue was reported by our QA team. They are > focused on the C/C++ development and may not even know that Java or other > languages can share the same views. > At the same time the ability to synchronize the breakpoints view with the debug > context is more useful feature, I would rather have an option for "installed" > than "supported". It still can be done by adding a new "Synchronize" option to > the view. I couldn't agree with you more. I doubt the action, as intended, has benefited many Eclipse users. Filtering on breakpoints installed on the active context is far more likely to be helpful. The question is whether we change the behavior (and hope we don't piss off anyone who has been relying on it), or play it safe and create a new action labeled "Show Breakpoints Installed in Selected Target". It's a tough call. Because I think the current action is so useless, part of me prefers your approach. But at the same time, I just feel we don't have the right to change an action that's been around for so long. I guess we can take the old approach of polling the mailing list and assuming no objections means it's "safe" to change something. :-)
I just thought that pending breakpoints will give trouble for such a feature. Would the user want to see pending breakpoints in the filter? Which effectively means no filtering at all. And even if the user does not want to see pending bp, I assume they want to see them once they are installed, but we currently don't handle such a case well, see Bug 324300.
(In reply to comment #6) > I just thought that pending breakpoints will give trouble for such a feature. > Would the user want to see pending breakpoints in the filter? Which > effectively means no filtering at all. > > And even if the user does not want to see pending bp, I assume they want to see > them once they are installed, but we currently don't handle such a case well, > see Bug 324300. Indeed. Here we start getting into the murky waters of what exactly it means for a breakpoint to be "installed". Technically speaking, is a disabled breakpoint installed? That's debatable. I say we leave such difficult questions to the implementation. The feature is simply about filtering out breakpoints that aren't installed, whatever "installed" means for the particular debugger associated with the active debug context.
(In reply to comment #6) > I just thought that pending breakpoints will give trouble for such a feature. > Would the user want to see pending breakpoints in the filter? Which > effectively means no filtering at all. My goal is to show only the breakpoints that are currently set on the target. As you said, not filtering pending breakpoints is meaningless. There is a DSF/GDB specific problem though. DSF/GDB doesn't set a breakpoint if it is disabled. If the filtering is on, these breakpoints are not shown and to enable them the filtering should be turned off. > And even if the user does not want to see pending bp, I assume they want to see > them once they are installed, but we currently don't handle such a case well, > see Bug 324300. Re. #324300 I was told the GDB patch that adds breakpoint notifications would be available in December. We already have it working with our GDB, so it it's not a problem. BTW, I've just noticed that even the traditional "Sync" icon is already used by "Link With Debug View".
(In reply to comment #8) > There is a DSF/GDB specific problem though. DSF/GDB doesn't set a breakpoint if > it is disabled. If the filtering is on, these breakpoints are not shown and to > enable them the filtering should be turned off. The reason we had to not install disabled bp at launch time is that we used to have to plant the breakpoint and send a separate MI command to disable it. This meant that in cases like non-interrupting attach (which we use in non-stop mode), a disabled breakpoint could interrupt the target before we had time to disable it. With GDB 7.0 we can use -break-insert -d which creates the breakpoint in disabled state. So, we could, with GDB 7.0, install disabled breakpoint even a launch time. But we'd need to keep the current functionality for 6.8 and earlier. This change was not worth doing before, but now it is up to you if you feel it is worth it. > Re. #324300 I was told the GDB patch that adds breakpoint notifications would > be available in December. We already have it working with our GDB, so it it's > not a problem. Nice!
(In reply to comment #9) Hi Mikhail, I will be looking into breakpoint improvements now. > Re. bug 324300 I was told the GDB patch that adds breakpoint notifications > would be available in December. We already have it working with our GDB, so > it it's not a problem. Will you be making that contribution soon, or should I implement it myself? Thanks
(In reply to comment #10) > (In reply to comment #9) > > Hi Mikhail, > > I will be looking into breakpoint improvements now. > > > Re. bug 324300 I was told the GDB patch that adds breakpoint notifications > > would be available in December. We already have it working with our GDB, so > > it it's not a problem. > > Will you be making that contribution soon, or should I implement it myself? It is still on my list, but I will be busy working on some urgent internal stuff this week and maybe next week. I can post the patch I have but not sure if I can work on it right now.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > > Hi Mikhail, > > > > I will be looking into breakpoint improvements now. > > > > > Re. bug 324300 I was told the GDB patch that adds breakpoint notifications > > > would be available in December. We already have it working with our GDB, so > > > it it's not a problem. > > > > Will you be making that contribution soon, or should I implement it myself? > > It is still on my list, but I will be busy working on some urgent internal > stuff this week and maybe next week. I can post the patch I have but not > sure if I can work on it right now. I can assume that the solution will eventually be available and instead focus on the current bug and the proposed fix you posted. Or, if you are comfortable with posting the patch to bug 324300, I can continue working on it myself and ask you to review the final result. Whatever you feel would be more efficient for you.
(In reply to comment #12) > I can assume that the solution will eventually be available and instead > focus on the current bug and the proposed fix you posted. Or, if you are > comfortable with posting the patch to bug 324300, I can continue working on > it myself and ask you to review the final result. Whatever you feel would > be more efficient for you. I'll post the patch for 324300 or push it to Gerrit.
(In reply to comment #5) > (In reply to comment #4) > > John, I think you are right. But it is confusing for customers who normally > > work with only one language. The issue was reported by our QA team. They are > > focused on the C/C++ development and may not even know that Java or other > > languages can share the same views. > > At the same time the ability to synchronize the breakpoints view with the debug > > context is more useful feature, I would rather have an option for "installed" > > than "supported". It still can be done by adding a new "Synchronize" option to > > the view. > > I couldn't agree with you more. I doubt the action, as intended, has > benefited many Eclipse users. Filtering on breakpoints installed on the > active context is far more likely to be helpful. The question is whether we > change the behavior (and hope we don't piss off anyone who has been relying > on it), or play it safe and create a new action labeled "Show Breakpoints > Installed in Selected Target". It's a tough call. Because I think the > current action is so useless, part of me prefers your approach. But at the > same time, I just feel we don't have the right to change an action that's > been around for so long. I guess we can take the old approach of polling the > mailing list and assuming no objections means it's "safe" to change > something. :-) I think we should move forward with this feature improvement. If I understand it right, the current behavior of "Show Breakpoints Supported by Selected Target" is a super-set of what we want: it will show all breakpoints that the active context _can_ support. The proposed approach is to only show breakpoints _installed_ on the active context, which means that it is a more aggressive filtering. I suggest we add a CDT preference to "Make breakpoint filtering aggressive". This preference should be on by default. If users want to revert to the old behavior they would just disable that preference. We should still go through the cdt-dev list to check for objections, but I wanted your opinion first. Thanks
(In reply to comment #14) > I suggest we add a CDT preference to "Make breakpoint filtering aggressive". > This preference should be on by default. If users want to revert to the old > behavior they would just disable that preference. That seems reasonable to me, assuming the enhanced filtering can be limited to CDT contexts (I haven't looked at the patch). I do have one request if we go this way, though. I ask that we add F1 help for the checkbox. There are far too many options in prefs and properties pages that make lots of sense to the developers who add the feature, but little or no sense to users who discover them. I think that would hold true for this option. Adding help to a page is a bit of a pain, so I think people just skip it and hope the user is super intuitive. I know in the past I've gone out of my way to add help where there was none when I added an option, so I fell I can preach here without being hypocritical.
(In reply to comment #15) > (In reply to comment #14) > > I suggest we add a CDT preference to "Make breakpoint filtering aggressive". > > This preference should be on by default. If users want to revert to the old > > behavior they would just disable that preference. > > That seems reasonable to me, assuming the enhanced filtering can be limited > to CDT contexts (I haven't looked at the patch). I do have one request if we > go this way, though. I ask that we add F1 help for the checkbox. Agreed. I'll give it a shot. Mikhail, I assume you are still ok with your proposed solution :) I'll build on top of it for the help and preference.
(In reply to comment #14) > I suggest we add a CDT preference to "Make breakpoint filtering aggressive". > This preference should be on by default. If users want to revert to the old > behavior they would just disable that preference. Yes, that's probably the best we can do. I don't like the wording though. Maybe something like "Show all breakpoints supported by selected target"? Or, "Show only active breakpoints for selected target"?
(In reply to comment #17) > (In reply to comment #14) > > I suggest we add a CDT preference to "Make breakpoint filtering aggressive". > > This preference should be on by default. If users want to revert to the old > > behavior they would just disable that preference. > > Yes, that's probably the best we can do. I don't like the wording though. > Maybe something like "Show all breakpoints supported by selected target"? > Or, "Show only active breakpoints for selected target"? I like the latter best, but I'm worried the word 'active' is gonna be interpreted as 'enabled'. We could use 'installed', although I'm not sure users will know what we mean.
(In reply to comment #18) IMO, there's no chance of picking 6-8 words that will clearly explain this feature. The wording of the platform action is at best ambiguous, and at worst misleading. Finding terse language that adequately describes a variation on that confusion is destined to be just as confusing, if not more. Given that, I'd prefer unclear terminology to potentially misleading terminology, and rely on F1 Help to provide the needed clarity. That's why I liked the "aggressive" wording--not because it aptly describes the new behavior, but because it doesn't try to.
I posted a message to the cdt-dev list to see if any one had objections to the proposed change of behavior. Let's see what happens.
I thought I'd update the title of the bug to reflect our findings.
(In reply to comment #6) > And even if the user does not want to see pending bp, I assume they want to > see them once they are installed, but we currently don't handle such a case > well, see Bug 324300. Turns out I was wrong about this dependency. Mikhail's patch fetches the latest list of breakpoints from GDB and therefore knows which ones are truly pending and which ones are not. I'm currently trying to get the bp filtering to work for multi-process.
I believe I got the solution nicely working now. I will do the submission to this feature in multiple Gerrit reviews. The idea is to isolate each change so that it can be reviewed easily. Mikhail, I will put you as the reviewer for when you will have some time. I hope that keeping the reviews separate makes it easier. If you prefer a single review, just let me know and I'll squash all my commits into a single one on top of yours. The different reviews are: Review #1 Mikhail's original patch as posted Bug 360735. This commit will have Mikhail as the author. Since it was written by one committer (Mikhail) and reviewed by another (me) I think it is good to go, once the next reviews are also approved. https://git.eclipse.org/r/7921 Review #2 Adding the preference for "aggressive breakpoint filtering". This is fairly mechanical except maybe for the extra delta handling to refresh the view. https://git.eclipse.org/r/7922 I've used the terminology 'aggressive breakpoint filtering' as John mentioned he preferred it because it didn't try (and fail) to explain the behavior. I updated the doc to explain the preference properly. But the naming can easily be changed if we find something better. I created GdbBreakpointVMNode to add new delta handling so that the breakpoint view would refresh whenever the new preference changes. Without that code, the user needed to manually toggle the "Show Breakpoints Supported by Selected Target" button to force the new preference setting to take effect. Review #3 Updating the solution to handle multi-process. https://git.eclipse.org/r/7923 The original patch would check if a bp was specific to a thread and then filter appropriately. When dealing with multiple processes, we also need to filter breakpoints that are applicable to one process and not another. The latest GDB does not provide us in MI with the information of which thread-group (inferior) the breakpoint applies to. Therefore, I had to use the CLI command 'break info', which does show that information. Although I don't like it much, I've chosen to send 'info break' directly from GdbBreakpointVMProvider. I would have preferred to do it from the IBreakpoints service but when I tried, I realized that the missing information affects multiple MI commands (e.g., -break-list, -break-insert) and I thought it was a waste to trigger 'info break' in all cases, when it was only needed for this particular case. I'm curious about your opinion on this decision. Review #4 Improve the multi-process solution to make use of a new 'thread-group' MI field that I'm trying to get into GDB. This will avoid using 'info break'. This is a small change. https://git.eclipse.org/r/7924 We should only commit this part when and if GDB accepts the proposed change. The GDB change was first posted here: http://sourceware.org/ml/gdb-patches/2012-09/msg00435.html
(In reply to comment #23) Marc, how to make all these patches appear in one branch?
(In reply to comment #24) > (In reply to comment #23) > Marc, how to make all these patches appear in one branch? I think that if you "Fetch from Gerrit..." in EGit, you can choose the last review (7924) and it will automatically fetch all the dependencies. I tried it myself and it seemed to work but I can say for sure because those commits were already in my local repo.
(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > Marc, how to make all these patches appear in one branch? > > I think that if you "Fetch from Gerrit..." in EGit, you can choose the last > review (7924) and it will automatically fetch all the dependencies. > > I tried it myself and it seemed to work but I can say for sure because those > commits were already in my local repo. I do it in a twisted way. For each patch I use "Fetch from Gerrit..." with "Update FETCH_HEAD only" and then "Cherry Pick".
I got a patch to handle multiple selections as Mikhail suggested. I'm not sure how to submit it with multiple reviews going on at once... I don't want to do an update to the last review because that one needs to wait for GDB to be ready. I could submit it as yet another review to keep that change isolated again. I'll think about it and do it tomorrow. Also, I noticed a problem with the current solution. The use of '-break-list' and 'info break' is not cached. This means that every time we change the selection in the Debug view, we send one new '-break-list' and as many 'info break' as there are breakpoints. We could cache this information, but we'll need to be careful about '-break-list' because caching it would make us miss updates like a breakpoint no longer being pending. With the use of MI events for breakpoints we can know when to clear the cache, but we'll have to handle things differently for GDB versions that don't have these events. I think that the mapping from bp to process can be safely cached though, since it won't change. I'll start with that.
It turns out that multi-select is more complicated than I thought. It can work well within the same debug session, but it cannot with a multi-selection spanning multiple debug sessions. I believe the problem is that the debug platform only calls the breakpoint provider of the first element of the multi-select. Therefore, when I select two debug sessions, only the breakpoints of the first one are shown. This is easy to confirm by launching a JDT session and CDT one, and selecting both. Only the breakpoints of the session that was first started will be shown. I see the following options for us: 1- leave multi-select as it stands now, which means we would only show breakpoints for the first (top-most) selected entry. 2- Support multi-select within the same session but not across sessions. This may be confusing to the user, although running multiple sessions is rare, I think. 3- don't support multi-select at all, which means that for multi-selections, we'd show all C/C++ bps instead of using the new aggressive filtering. 4- Enhance platform. I think this multi-select support makes sense for platform too, so that if you select both a JDT and CDT sessions, you would see breakpoints for both. I'll quickly look at how complex the change would be for platform, but if someone has a recommendation or another approach, please chime in.
(In reply to comment #28) > It turns out that multi-select is more complicated than I thought. It can > work well within the same debug session, but it cannot with a > multi-selection spanning multiple debug sessions. > > I believe the problem is that the debug platform only calls the breakpoint > provider of the first element of the multi-select. Therefore, when I select > two debug sessions, only the breakpoints of the first one are shown. This > is easy to confirm by launching a JDT session and CDT one, and selecting > both. Only the breakpoints of the session that was first started will be > shown. > > I see the following options for us: > > 1- leave multi-select as it stands now, which means we would only show > breakpoints for the first (top-most) selected entry. > > 2- Support multi-select within the same session but not across sessions. > This may be confusing to the user, although running multiple sessions is > rare, I think. > > 3- don't support multi-select at all, which means that for multi-selections, > we'd show all C/C++ bps instead of using the new aggressive filtering. > > 4- Enhance platform. I think this multi-select support makes sense for > platform too, so that if you select both a JDT and CDT sessions, you would > see breakpoints for both. > > I'll quickly look at how complex the change would be for platform, but if > someone has a recommendation or another approach, please chime in. Fixing #4 would be nice, #2 is good too. Otherwise it is better leaving the patch as it is (#1). Thanks for looking at it, I didn't realize it would be difficult.
(In reply to comment #29) > > 1- leave multi-select as it stands now, which means we would only show > > breakpoints for the first (top-most) selected entry. > > > > 2- Support multi-select within the same session but not across sessions. > > This may be confusing to the user, although running multiple sessions is > > rare, I think. > > > > 3- don't support multi-select at all, which means that for multi-selections, > > we'd show all C/C++ bps instead of using the new aggressive filtering. > > > > 4- Enhance platform. I think this multi-select support makes sense for > > platform too, so that if you select both a JDT and CDT sessions, you would > > see breakpoints for both. > > > > I'll quickly look at how complex the change would be for platform, but if > > someone has a recommendation or another approach, please chime in. > > Fixing #4 would be nice, #2 is good too. Otherwise it is better leaving the > patch as it is (#1). > Thanks for looking at it, I didn't realize it would be difficult. No problem, I think it would be nice to support multi-select. Turns out that the problem is not in platform but in DSF. When I use JDT and CDI, multi-select properly works across sessions. With DSF though, things don't work like that, but I haven't figured out why yet.
(In reply to comment #30) > Turns out that the problem is not in platform but in DSF. When I use JDT > and CDI, multi-select properly works across sessions. With DSF though, > things don't work like that, but I haven't figured out why yet. I've opened Bug 390685 to track what I believe is a DSF limitation. Until that is fixed I think we can go with > 2- Support multi-select within the same session but not across sessions. > This may be confusing to the user, although running multiple sessions is > rare, I think. and I believe this will automatically work across sessions if we fix Bug 390685. I pushed such a solution to Gerrit as another review. This small change is isolated and meant to support multi-select, so I thought it would be easier as a separate review. https://git.eclipse.org/r/7974 It is based on review #3 (7923) so that we don't need to commit the change of review #4 before commit this one. Let's call this one review #3.5
As mentioned in the multicore debug call, I'm working on improving the solution a little and will post the change to Gerrit soon. I wanted to mention that I realized that the solution does not work with GDB 7.0 and 7.1. But this is because the IBreakpoints.getBreakpoints() always returns an empty list for those two versions of GDB. It has something to do with not knowing the pid at startup. I have to look more into it.
(In reply to comment #23) > Although I don't like it much, I've chosen to send 'info break' directly > from GdbBreakpointVMProvider. I would have preferred to do it from the > IBreakpoints service but when I tried, I realized that the missing > information affects multiple MI commands (e.g., -break-list, -break-insert) > and I thought it was a waste to trigger 'info break' in all cases, when it > was only needed for this particular case. I'm curious about your opinion on > this decision. I have fixed this craziness and moved the use of 'info break' into the GDBBreakpoints_7_2 service. This change is only needed for multi-process which is only available starting with GDB 7.2. Note that 'info break' is only used when calling IBreakpoints.getBreakpoints() because that is the time that this new filtering feature needs the information about processes (thread-groups). We don't use IBreakpoints.getBreakpoints() anywhere else, so the use of 'info break' is limited to this feature. To use 'info break' in the service, I needed most of the changes of review #4 so I merged review #3 and #4 together and abandoned review #4. Mikhail, the only left for you to review is review #3 https://git.eclipse.org/r/7923. Thanks! Summary of the reviews: > Review #1 - DONE > https://git.eclipse.org/r/7921 > > Review #2 - DONE > https://git.eclipse.org/r/7922 > > Review #3 - TODO > https://git.eclipse.org/r/7923 > > The original patch would check if a bp was specific to a thread and then > filter appropriately. When dealing with multiple processes, we also need to > filter breakpoints that are applicable to one process and not another. > > The latest GDB does not provide us in MI with the information of which > thread-group (inferior) the breakpoint applies to. Therefore, I had to use > the CLI command 'info break', which does show that information. > > Review #4 - ABANDONED (changes are now part of review #3) > https://git.eclipse.org/r/7924 I'm trying to get GDB 7.6 to return "thread-groups" in MI commands and once that is accepted, we'll need to use that information in a new GDBBreakpoints_7_6 class. http://sourceware.org/ml/gdb-patches/2012-09/msg00435.html Finally, this feature does not work with GDB 7.0 and 7.1 because of an un-related bug in the breakpoint context. I suggest that we don't block ourselves because of these old GDBs but that I open an other bug to track.
I forgot to mention that it is not currently possible to do caching for the "break-list" or "info break" commands. The reason is that we don't know if a breakpoint has created/modified between such commands. However, Mikhail fix for Bug 392512 will address this, and we'll be able to use caching for breakpoints.
After Mikhail's review I committed the feature to master. I've opened Bug 392900 to remember to implement caching when dependencies are resolved, and Bug 392899 about the GDB 7.0 and 7.1 limitation. Thanks for the reviews Mikhail and John.
I've updated the N&N: http://wiki.eclipse.org/CDT/User/NewIn82#Breakpoint_Filtering
