Community
Participate
Working Groups
Currently the implementations for the MoveToLine, ResumeAtLine, RunToLine actions are in dsf-gdb. Move these to dsf for use by other debuggers and create an IRunControl2 interface to support these.
Created attachment 160449 [details] Move actions to DSF Moves actions from gdb ui to dsf ui and introduces IRunControl2 and implementation in dsf-gdb.
Ken, some of this may related to Marc's changes rather than your refacorting: 1. Why are the to-line adapters using the to-address interfaces instead of to-line ones?. E.g., why is DisassemblyMoveToLineAdapter getting and using the IMoveToAddress adapter instead of the IMoveToLine one? This is going on consistently in all three adapters. Perhaps there's something I'm not getting, but this looks odd to me. 2. Should we be using DebugPlugin.getAdapter() instead of calling IAdaptable.getAdapter() directly (e.g., in DisassemblyMoveToLineAdapter.moveToLine()) 3. In IRunControl2, javadoc for line number paramters should be explicit that they are 1-based. I'm not sure that we are doing so everywhere else, but we should, in my opinion. We should avoid ambiguity in public interfaces. 4. We shouldn't duplicate the javadoc for an interface in its implementations (see GDBRunControl_7_0_NS.runToLine(), e.g.). This increases maintenance efforts, and creates an opportunity for conflicting documentation. E.g., each IRunControl2's lengthy documentation is repeated three times throughout the code. The implementations should instead have the /* (non-Javadoc) * @see org.eclipse.cdt.... */ as generated by Source > Generate Element Comment
(In reply to comment #2) > Ken, some of this may related to Marc's changes rather than your refacorting: > > 1. Why are the to-line adapters using the to-address interfaces instead of > to-line ones?. E.g., why is DisassemblyMoveToLineAdapter getting and using the > IMoveToAddress adapter instead of the IMoveToLine one? This is going on > consistently in all three adapters. Perhaps there's something I'm not getting, > but this looks odd to me. > > 2. Should we be using DebugPlugin.getAdapter() instead of calling > IAdaptable.getAdapter() directly (e.g., in > DisassemblyMoveToLineAdapter.moveToLine()) I just moved the stuff that Marc had already created and didn't look at this closely. I propose I commit my refactoring and then we look at these issues separately. > 3. In IRunControl2, javadoc for line number paramters should be explicit that > they are 1-based. I'm not sure that we are doing so everywhere else, but we > should, in my opinion. We should avoid ambiguity in public interfaces. Good point, I'll add that. > > 4. We shouldn't duplicate the javadoc for an interface in its implementations > (see GDBRunControl_7_0_NS.runToLine(), e.g.). This increases maintenance > efforts, and creates an opportunity for conflicting documentation. E.g., each > IRunControl2's lengthy documentation is repeated three times throughout the > code. The implementations should instead have the > > /* (non-Javadoc) > * @see org.eclipse.cdt.... > */ > > as generated by Source > Generate Element Comment Thanks, I wasn't sure what to do there, I'll change the doc in the services.
(In reply to comment #3) > I just moved the stuff that Marc had already created and didn't look at this > closely. I propose I commit my refactoring and then we look at these issues > separately. Sounds good. We'll see what Marc says.
Committed to HEAD.
(In reply to comment #1) > Created an attachment (id=160449) [details] > Move actions to DSF Nice work Ken. I like the interface better the way you did it. Below are some minor (except the first one) comments: 1- Both GDBRunControl_7_0 and GDBRunControl_7_0_NS must also register with the IRunControl2 name. 2- in RunToLine.java, the top @since should be 2.1 and you can remove the couple of @since 2.1 that is within the file 3- In MIRunControl.java and GDBRunControl_7_0_NS.java, how about making resumeAtLocation() and runToLocation() private, now that you added a better interface? 4- In MIRunControl.moveToLine and moveToAdress, I would suggest only creating the breakpoint in the (!resume) case, instead of all the time. Same comment for GDBRunControl_7_0_NS.java 5- do we need some @since 3.0 tag in IMIRunControl since it now needs to implement the new IRunControl2 methods? I'm not sure about that... 6- maybe rename SuspendResumeAdapterFactory.GdbSuspendResume to remove the 'Gdb' 7- in MoveToLine.moveToLine I don't think you need the empty DataRequestMonitor<Object> when calling IRunControl2.moveToLine, I believe you can use 'rm' directly. Same comment for moveToAddress. Same comment for file ResumeAtLine.java and RunToLine.java 8- in RunToLine.java, MoveToLine.java and ResumeAtLine.java, there is a recurring typo (probably mine); 'Faild' instead of 'Failed' 9- In MIRunControl and GDBRunControl_7_0_NS.java, to convert an address to a string you used address.toHexAddressString(). This call keeps all the leading 0's, which I found annoying when looking at the gdb traces. Everywhere else you use address.toString(16) anyway, which I found to be nicer for the traces.
(In reply to comment #2) > 1. Why are the to-line adapters using the to-address interfaces instead of > to-line ones?. E.g., why is DisassemblyMoveToLineAdapter getting and using the > IMoveToAddress adapter instead of the IMoveToLine one? This is going on > consistently in all three adapters. Perhaps there's something I'm not getting, > but this looks odd to me. This is only done when dealing with the DisassemblyView. Although I got inspired from another piece of code, I believe the reason is that a location in Disassembly cannot always be sourceFile:line; in the DisassemblyView, if you select an assembly line, it does not correspond to a sourceFile:line, so we always use an address. > 2. Should we be using DebugPlugin.getAdapter() instead of calling > IAdaptable.getAdapter() directly (e.g., in > DisassemblyMoveToLineAdapter.moveToLine()) I copied that code from MoveToLineAdapter, which was already there. But I believe you are right that we should use DebugPlugin.getAdapter() instead, in all those cases.
Last thing is that Pawel should give his ok for the IRunControl2 interface.
(In reply to comment #8) > Last thing is that Pawel should give his ok for the IRunControl2 interface. I would request one clarification though: I assume that the "sourceFile" parameter refers to a source path that has been mapped by the source lookup to a debugger path... if there is a source lookup path mapping configured.
Created attachment 160995 [details] Fix for GDB 7.0 (In reply to comment #6) > 1- Both GDBRunControl_7_0 and GDBRunControl_7_0_NS must also register with the > IRunControl2 name. I've fixed this with the attached patch. Without this, RunToLine, MoveToLine and ResumeAtLine no longer worked for GDB 7.0. Committed to HEAD. I did not address the other comments on this bug.
(In reply to comment #10) > Created an attachment (id=160995) [details] > Fix for GDB 7.0 > > (In reply to comment #6) > > > 1- Both GDBRunControl_7_0 and GDBRunControl_7_0_NS must also register with the > > IRunControl2 name. > > I've fixed this with the attached patch. Without this, RunToLine, MoveToLine > and ResumeAtLine no longer worked for GDB 7.0. > > Committed to HEAD. > > I did not address the other comments on this bug. Thanks, I've been in meetings a lot and am just now addressing the other comments.
(In reply to comment #11) > Thanks, I've been in meetings a lot and am just now addressing the other > comments. No problem
Just committed changes that I think will address all of the comments. Thanks for all the feedback.
(In reply to comment #13) > Just committed changes that I think will address all of the comments. Thanks > for all the feedback. Thanks Ken. As a favor, can you attach to the bug any patch you will commit? They are often useful for reference. Also, I sometimes like to review the updates to a previous review, as it often makes other points come out. Thanks
Created attachment 161136 [details] Some missing things I committed this patch to HEAD. It adds a couple of missing cleanups. Importantly though, it puts back the overridden resumeAtLocation() and runToLocation() in GDBRunControl and GDBRunControl_7_0. When I suggested making those methods private in MIRunControl, I didn't realize they were being overridden. For ResumeAtLine, the GDBRunControl_7_0 uses the new -exec-jump which MIRunControl cannot use since it didn't exist. For RunToLine, GDBRunControl does the real work; MIRunControl uses -exec-until which is very limited; GDBRunControl uses temporary breakpoints instead (I had left MIRunControl as it was from the start, but I don't think we should use the -exec-until solution).
Created attachment 169777 [details] move editor and disassembly actions I found a little more to do here: While the re-targetable actions were moved to DSF UI, the actions themselves contributing to the editor and disassembly view were left behind in the DSF GDB UI plugin. It took a small refactoring but the patch basically just moves those actions and the enablement class from the GDB plugin to the generic DSF UI plug-in.
Marc, can you take a look at this last bit as well?
Created attachment 169833 [details] Simplified fix based on email discussion (In reply to comment #17) > Marc, can you take a look at this last bit as well? Sorry, there was some confusion about who was going to do this (see bug 300096 comment #29) Also, a discussion on the mailing list made me realize I over-complicated the solution. See http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg18539.html So here is a suggested simpler approach that should do what we need. Suggested changes are: - re-use the registration of the 4 actions for the editor that is already done by CDT by simply setting the same system property to make them visible. We no longer need to add them to dsf.ui/plugin.xml - make dsf.ui/EvaluationContextManager use the same system property as base CDT to re-use the same action registration. - move the 2 icons from dsf.gdb.ui to dsf.ui for the Disassembly view and make the dsf.ui/plugin.xml point to these new icons - delete the watch_exp icon from dsf.gdb.ui since we will re-use base CDT's one. Ken, I'll let you check this in if you agree with it.
Applied Marc's patch. Tested with GDB and EDC. Thanks - Marc.
[cdt cvs genie] kryall: added file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/icons/move_to_line.gif?root=Tools_Project&revision=1.1&view=markup [cdt cvs genie] kryall: added file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/icons/resume_at_line.gif?root=Tools_Project&revision=1.1&view=markup [cdt cvs genie] kryall: added file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/EvaluationContextManager.java?root=Tools_Project&revision=1.1&view=markup [cdt cvs genie] kryall: changed file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/plugin.properties?root=Tools_Project&r1=1.11&r2=1.12 [cdt cvs genie] kryall: changed file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/plugin.xml?root=Tools_Project&r1=1.18&r2=1.19 [cdt cvs genie] kryall: changed file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/internal/ui/DsfUIPlugin.java?root=Tools_Project&r1=1.5&r2=1.6 [cdt cvs genie] kryall: changed file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/plugin.properties?root=Tools_Project&r1=1.16&r2=1.17 [cdt cvs genie] kryall: changed file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/plugin.xml?root=Tools_Project&r1=1.36&r2=1.37 [cdt cvs genie] kryall: deleted http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/icons/full/obj16/watch_exp.gif?root=Tools_Project&view=markup [cdt cvs genie] kryall: deleted http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/icons/full/obj16/move_to_line.gif?root=Tools_Project&view=markup [cdt cvs genie] kryall: deleted http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/icons/full/obj16/resume_at_line.gif?root=Tools_Project&view=markup [cdt cvs genie] kryall: changed file http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/GdbUIPlugin.java?root=Tools_Project&r1=1.4&r2=1.5 [cdt cvs genie] kryall: deleted http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/EvaluationContextManager.java?root=Tools_Project&view=markup
