Bugzilla – Full Text Bug Listing
| Summary: | [menu][breakpoints][cdi] Add support for toggling watchpoints in the variables and expressions views. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Pawel Piech <pawel.1.piech> | ||||||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | John Cortell <john.cortell> | ||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||||||||
| Severity: | enhancement | ||||||||||||||
| Priority: | P3 | CC: | dd.general-inbox, john.cortell, pawel.1.piech | ||||||||||||
| Version: | 0 DD 1.1 | Flags: | pawel.1.piech:
review+
marc.khouzam: review+ |
||||||||||||
| Target Milestone: | 7.0 | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Pawel Piech
Created attachment 159245 [details]
Solution
(In reply to comment #1) > Created an attachment (id=159245) [details] > Solution I'm a little confused by this solution. It creates an IWatchpointTarget interface, but it does not implement one for GDB. So it doesn't actually fix this bug. Besides that I have only a few of comments: 1) The enablement of the action is based on whether the selected element is an instance of IWatchpointTarget (i.e. there is no IWatchpointTarget.canCreateWatchpoin()). It greatly simplifies the implementation of the action, but some debuggers may be smart enough not to enable the action for expressions such as "1 + 1". I really don't know how significant this is, since the above is really a silly use case. 2) IWatchpointTarget should be retrieved using DebugPlugin.getAdapter(). 3) IMO GetSizeRequest should really be an interface (as should AddressRequest and DisassemblyRequest), and its implementation should be hidden. (In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=159245) [details] [details] > > Solution > I'm a little confused by this solution. It creates an IWatchpointTarget > interface, but it does not implement one for GDB. So it doesn't actually fix > this bug. I'm equally confused by this comment. I assure you I can successfully set watchpoints from the Variables and Expressions view in a DSF-GDB session with this patch, and that the watchpoints are hit. Wouldn't be much of a patch otherwise. :-) No GDB specific code is needed in the solution. The ability to create a backend watchpoint is already in place for DSF-GDB. Ah...I see I didn't select the DSF plugin when I created the patch. Mystery solved. (In reply to comment #3) > No GDB specific code is needed in the solution. The ability to create a backend > watchpoint is already in place for DSF-GDB. Since DSF is not specific to CDT breakpoint type, this feature should be enabled for DSF-GDB only (and separately for EDC, etc.). (In reply to comment #5) > (In reply to comment #3) > > No GDB specific code is needed in the solution. The ability to create a backend > > watchpoint is already in place for DSF-GDB. > > Since DSF is not specific to CDT breakpoint type, this feature should be > enabled for DSF-GDB only (and separately for EDC, etc.). I don't understand, and thus I'll ask a stupid question. The DSF-EDC solution supports watchpoints (though on a per-target basis, it not guaranteed, but the same can be said about DSF-GDB. You could in theory have GDB server that doesn't support watchpoints on a particular target.) What's the point of making the Add Watchpoint menu indepdendently available across DSF-GDB and DSF-EDC? You may have a DSF-based debugger which does not use CDT breakpoints at all, but a proprietary breakpoint type instead. Currently Wind River does this and I believe TI does also. Having an "Add Watchpoint" action which creates CDT Watchpoints would only be confusing in such debuggers. Actually, to avoid confusion it would also be helpful to rename IWatchpointTarget to ICWatchpointTarget. (In reply to comment #6) > > Since DSF is not specific to CDT breakpoint type, this feature should be > > enabled for DSF-GDB only (and separately for EDC, etc.). > > I don't understand, and thus I'll ask a stupid question. The DSF-EDC solution > supports watchpoints (though on a per-target basis, it not guaranteed, but the > same can be said about DSF-GDB. You could in theory have GDB server that > doesn't support watchpoints on a particular target.) Hmm... this is a better use case for having an asynchronous IWatchTarget.supportsWatchpoints(), than what I came up with. (In reply to comment #7) OK. I think I understand. I'll rework the patch tomorrow. Created attachment 159459 [details]
Solution
This has the missing DSF piece. I have not yet adjusted the solution as per Pawel's feedback. Just wanted to post the complete form of my initial attempt.
Created attachment 159500 [details]
Updated solution
Reworked solution to address all of Pawel's comments.
(In reply to comment #10) > Reworked solution to address all of Pawel's comments. Thank you for making these changes. Please add an @since tag to VariableVMProvider.configureLayout(). (In reply to comment #11) > (In reply to comment #10) > > Reworked solution to address all of Pawel's comments. > > Thank you for making these changes. Please add an @since tag to > VariableVMProvider.configureLayout(). Thanks for the review. Hm...I'm not getting a API tooling warning for this method, but I don't understand why. I have a baseline defined and I just confirmed that the dsf plugins are part of it. I even created a public foo method in VariableVMProvider and nothing. This is troubling... (In reply to comment #12) > Hm...I'm not getting a API tooling warning for this > method, but I don't understand why. I have a baseline defined and I just > confirmed that the dsf plugins are part of it. I even created a public foo > method in VariableVMProvider and nothing. This is troubling... Ah...so what's happening is that all of the org.eclipse.cdt.dsf.debug.ui.viewmodel.* are hidden from all plug-ins (except friends), which is surprising since they're not in an ".internal." package, but I suppose that's because you plan on making them public at some point? (I.e., they're provisional at this point). Anyway, I'll add the @since tag to the method since you requested it. (In reply to comment #13) > Ah...so what's happening is that all of the > > org.eclipse.cdt.dsf.debug.ui.viewmodel.* > > are hidden from all plug-ins (except friends), which is surprising since > they're not in an ".internal." package, but I suppose that's because you plan > on making them public at some point? (I.e., they're provisional at this point). > > Anyway, I'll add the @since tag to the method since you requested it. We talked about using the internal.provisional prefix on the package names and decided against it to avoid unnecessary churn when we do make them public. In all fairness, they should at least include a warning in their header comments. (In reply to comment #14) > We talked about using the internal.provisional prefix on the package names and > decided against it to avoid unnecessary churn when we do make them public. In > all fairness, they should at least include a warning in their header comments. BTW, at this point if the community wants it we can make them public now. We'll just have the awkward problem of leaking the provisional flexible hierarchy interfaces in this API. (In reply to comment #15) > (In reply to comment #14) > > We talked about using the internal.provisional prefix on the package names and > > decided against it to avoid unnecessary churn when we do make them public. In > > all fairness, they should at least include a warning in their header comments. > > BTW, at this point if the community wants it we can make them public now. > We'll just have the awkward problem of leaking the provisional flexible > hierarchy interfaces in this API. My vote is to keep them provisional until the upstream interfaces are public, and apply pressure on the platform folks to go final with FH...hey, wait a minute--you're one of them ;-) Committed to HEAD. (In reply to comment #16) > My vote is to keep them provisional until the upstream interfaces are public, > and apply pressure on the platform folks to go final with FH...hey, wait a > minute--you're one of them ;-) See bug 161435. (In reply to comment #17) > Committed to HEAD. Looks good John! I have a couple of comments: (The posted patch is not complete, I think it's missing the cdt.debug.core plugin changes, since I don't fully understand the changes, it made it harder to go over :-), but I think I was able to look over the different changes in HEAD.) 1- how come we don't need the <enablement> condition in the plugin.xml anymore? I'm worried about unecessary plugin activation. 2- CRequest does not have a copyright notice 3- Should we have a handleFailure() case for GdbVariableExpressionVMC.getSize() in drm? Since drm does not have a parent rm, nothing will call request.setSize(-1) and done() in the failure case. It's done right in canSetWatchpoint() for that class 4- Should you set request.setSize(-1) and done() in GdbVariableExpressionVMC.getSize() in the else case of if (expressionService != null). Same thing for canSetWatchpoint() 5- maybe ICWatchpointTarget.GetSizeRequest should be IGetSizeRequest since it's an interface? Same for CanCreateWatchpointRequest 6- Should AddWatchpointOnVariableActionDelegate.selectionChanged() have fVar=null as it's first line (or at least somewhere in there)? (In reply to comment #19) > (The posted patch is not complete, I think it's missing the cdt.debug.core > plugin changes, since I don't fully understand the changes, it made it harder > to go over :-), but I think I was able to look over the different changes in > HEAD.) There were no changed in the cdt debug core plugin. > 1- how come we don't need the <enablement> condition in the plugin.xml anymore? > I'm worried about unecessary plugin activation. The action is now an object contribution. It used to be a view contribution. > 2- CRequest does not have a copyright notice Will fix. Good catch. > 3- Should we have a handleFailure() case for GdbVariableExpressionVMC.getSize() > in drm? Since drm does not have a parent rm, nothing will call > request.setSize(-1) and done() in the failure case. It's done right in > canSetWatchpoint() for that class Indeed. Fixed. > 4- Should you set request.setSize(-1) and done() in > GdbVariableExpressionVMC.getSize() in the else case of if (expressionService != > null). Same thing for canSetWatchpoint() Yep. Fixed. Also, optimized things a bit to have the request objects set the result to the invalid/default value so an explicit call isn't necessary. > 5- maybe ICWatchpointTarget.GetSizeRequest should be IGetSizeRequest since it's > an interface? Same for CanCreateWatchpointRequest I've seen precedence in DSF for inner request interfaces not having the 'I'. I was just trying to follow precedence. > > 6- Should AddWatchpointOnVariableActionDelegate.selectionChanged() have > fVar=null as it's first line (or at least somewhere in there)? Definitely. Fixed. Will post a patch so you can review the changes in case I missed something. Created attachment 159725 [details]
Modifications resulting from Marc's review
Marc, please review.
BTW, there *were* changes in the cdt debug core plugin that I missed when I created the patch file. I think I've learned my lesson. I keep trying to optimize the patch creation process by choosing the particular plugins that show to have changed (">"), but I keep overlooking a plugin or two. So from now on, I'll choose them all :-/
(In reply to comment #21) > Created an attachment (id=159725) [details] > Modifications resulting from Marc's review Looks good, thanks > BTW, there *were* changes in the cdt debug core plugin that I missed when I > created the patch file. I think I've learned my lesson. I keep trying to > optimize the patch creation process by choosing the particular plugins that > show to have changed (">"), but I keep overlooking a plugin or two. So from now > on, I'll choose them all :-/ Been there (lost code that way :-( ). And for some reason, I still keep wanting to select the plugins manually. It's hard to resist :-) Created attachment 159821 [details]
Patched holes in error handling
Took a closer look at the error paths and found more gaps in the error handling. I'm finding that it's easy to fumble errors in this highly asynchronous environment.
Latest patch committed to HEAD. Marc, please review. (In reply to comment #24) > Latest patch committed to HEAD. Marc, please review. Over a month late... but the change looks good :-) |