Community
Participate
Working Groups
Our customers use both GDB and JTAG debugging at the same time and setting breakpoint between debug sessions are cumbersome, the toggle breakpoint type selection need to be switch to match the breakpoint type inorder to create the corresponding breakpoint for each debug session. I would like to change the way CDT toggle breakpoint target factory enablement, by making them more specific to the current debug context, project type and IDeclaration element. For Disassembly view: the factory will be enabled when the debug session is either cdi or dsf-gdb. For Editor: the factory will be enabled when the editor belongs to a project that supports ICBreakpoint Type, a CProject will have a new project scope preference. For IDeclaration: the factory will behave similar to editor.
Created attachment 191451 [details] patch This patch make couple assumptions for the disassembly view, it checks whether the session id is either a gdb-debug-model or cdi-, and enable the toggle breakpoint factory. I am not sure if this is a problem for other debug launch type or not. Toni, I added a method to DisassemblyPart to returns the sessionId. Pawel, Marc, Mikhail, it looks like you guys have involved in CDT breakpoint in the past, can you guys review these changes?
On a quick read, the patch seems fine to me. Thanks Patrick.
1. I am not happy with the project preference which gets set by default. As you need to support existing projects without this preference anyway, I'd suggest to just keep the default (i.e. no explicit preference) for standard CDT projects. 2. The preference and the code should live in the org.eclipse.cdt.debug.core namespace. 3. I saw that you also changed showSourceRootsAtTopOfProject(). I assume you want to make it adhere to preference defaults, but please don't use a deprecated method for that (and a separate bug would be in order). 4. There are a few since tags missing. 5. As for the disassembly, I'd suggest to test the debug model identifier of the current debug context. The existing ToggleBreakpointsTargetFactory already does this to select the default target id (see also ICBreakpoint.C_BREAKPOINTS_DEBUG_MODEL_ID and its javadoc). This should actually be sufficient for you to override the default for your debug model.
Created attachment 192040 [details] patch reworked (In reply to comment #3) Toni, Thanks for the great comments and suggestions. I have reworked the patch to do what you have suggested. I would like to commit this patch if it is ok with you. > 1. I am not happy with the project preference which gets set by default. > As you need to support existing projects without this preference anyway, I'd > suggest to just keep the default (i.e. no explicit preference) for standard > CDT projects. Corrected in the latest patch, does not set the preference by default in CDT projects. CDT extender will need to set the preference and the ToggleCBreakpointTester will make use of the preference to determine whether the CBreakpoint toggle action should be enabled. > 2. The preference and the code should live in the org.eclipse.cdt.debug.core > namespace. The code is placed in the org.eclipse.cdt.debug.core.CDebugUtils class and the preference is added to org.eclipse.cdt.debug.core.ICDebugConstants. > 3. I saw that you also changed showSourceRootsAtTopOfProject(). I assume you > want to make it adhere to preference defaults, but please don't use a > deprecated method for that (and a separate bug would be in order). Wasn't intended to make any changes to this class, must be CVS update got messed up. > 4. There are a few since tags missing. Fixed, updated my API tooling baseline. > 5. As for the disassembly, I'd suggest to test the debug model identifier of > the current debug context. The existing ToggleBreakpointsTargetFactory > already does this to select the default target id (see also > ICBreakpoint.C_BREAKPOINTS_DEBUG_MODEL_ID and its javadoc). This should > actually be sufficient for you to override the default for your debug model. Great suggestion, I was looking for something like this, the latest patch make use of the debug model Id.
(In reply to comment #4) > Toni, Thanks for the great comments and suggestions. I have reworked the patch > to do what you have suggested. I would like to commit this patch if it is ok > with you. Thanks, I still see a few issues. 1) External (non-workspace) files There are two cases: Either the file is associated with a CProject or not. If it is associated with a CProject, the project might have been chosen at random, ie. the evaluation whether CBreakpoints are supported could be wrong. In the second case (no project), the breakpoint toggle factory is disabled, ie. you can no longer toggle a breakpoint in such an editor. To avoid this regression, I think the tester should return true if the project is null. To be able to override the default in this case, you could evaluate the preference also on workspace level. 2) I see that adding the constant to ICDebugConstants is flagged as a breaking change by API tooling. I'd suggest to mark ICDebugConstants as @noimplement and @noextend to avoid breaking changes when adding a constant in the future. 3) Property tester DisassemblyToggleBreakpointTester should use IDisassemblyPart instead of DisassemblyView. 4) In case of the disassembly, it is still a breaking change. I'd suggest to use the workspace preference above to control whether the tester should evaluate the debug model ids, ie. as long as the preference is not set, the tester returns true for all IDisassemblyParts and only when the preference is set, debug model ids are considered.
Created attachment 192121 [details] patch with additional rework (In reply to comment #5) Toni, here is the patch with the latest changes, let me know if there is any additional change. Thanks for taking the time to review the patch. > 1) External (non-workspace) files > There are two cases: Either the file is associated with a CProject or not. > If it is associated with a CProject, the project might have been chosen at > random, ie. the evaluation whether CBreakpoints are supported could be wrong. > In the second case (no project), the breakpoint toggle factory is disabled, > ie. you can no longer toggle a breakpoint in such an editor. > To avoid this regression, I think the tester should return true if the project > is null. I've updated the patch to do this. Not sure if you have already aware of, perhaps you do, the CProject is wrongly assigned to external file. The EditorUtility#getEditorInputForLocation take the first project and assign it to the file. This is a problem for my implementation, as well as other bugs in the outline view. So, I do an additional check to see if the file really exist, if not than return true for the tester. User will need to make it's own choice for which breakpoint type at this point. Do you know if the incorrect project for external file is a known issue? Is there a bug#? > To be able to override the default in this case, you could evaluate the > preference also on workspace level. Sure, I have added a system property to override the default behavior for not breaking current behavor, the testers will return true if this property is not set. > 2) I see that adding the constant to ICDebugConstants is flagged as a breaking > change by API tooling. I'd suggest to mark ICDebugConstants as @noimplement > and @noextend to avoid breaking changes when adding a constant in the future. I added API filter in my previous patch, it should filter the API tooling error. Anyway, I was debating yesterday whether I should add the @noextend and @noimplement to the interface, but wasn't sure whether it will cause backward compatible issue or not. And ended up minimized the change locally by filtering the constant only. I updated the patch to have @noextend and @noimplement and update the error filter well. > 3) Property tester DisassemblyToggleBreakpointTester should use > IDisassemblyPart instead of DisassemblyView. Fixed. > 4) In case of the disassembly, it is still a breaking change. I'd suggest to > use the workspace preference above to control whether the tester should > evaluate the debug model ids, ie. as long as the preference is not set, the > tester returns true for all IDisassemblyParts and only when the preference is > set, debug model ids are considered. Same as above, used the system property.
(In reply to comment #6) > Do you know if the incorrect project for > external file is a known issue? Is there a bug#? This is in fact intentional to make features like outline view, navigation, etc., working for such files, but it certainly has some drawbacks and does not work if no CProject exists. A better solution would be to use a synthetic CProject instead, but this is tricky. See also bug 323673. > > To be able to override the default in this case, you could evaluate the > > preference also on workspace level. > Sure, I have added a system property to override the default behavior for not > breaking current behavor, the testers will return true if this property is not > set. Ok, this should make it water-proof. However, if used as a system property, the constant should have a "org.eclipse.cdt.debug.core." prefix. Thanks for the changes! +1 from my POV.
Thanks, added the prefix to the variable and committed to HEAD.
*** cdt cvs genie on behalf of pchuong *** Bug 340177 - [breakpoints] Update CDT ToggleBreakpointTargetFactory enablement [*] plugin.xml 1.43 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.42&r2=1.43 [*] .api_filters 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/.settings/.api_filters?root=Tools_Project&r1=1.4&r2=1.5 [*] ICDebugConstants.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/ICDebugConstants.java?root=Tools_Project&r1=1.18&r2=1.19 [*] CDebugUtils.java 1.35 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/CDebugUtils.java?root=Tools_Project&r1=1.34&r2=1.35 [*] plugin.xml 1.29 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/plugin.xml?root=Tools_Project&r1=1.28&r2=1.29 [+] DisassemblyToggleBreakpointTester.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyToggleBreakpointTester.java?root=Tools_Project&revision=1.1&view=markup [*] plugin.xml 1.258 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/plugin.xml?root=Tools_Project&r1=1.257&r2=1.258
Patrick, fixing Bug 375227 made me look into this bug in detail. Could you clarify a couple of points that I don't understand? 1- CDebugUtils.isCustomToggleBreakpointFactory() checks if ICDebugConstants.PREF_TOGGLE_BREAKPOINT_MODEL_IDENTIFIER is set to true (Boolean.valueOf(customModel)). However, CDebugUtils.setToggleBreakpointFactory() sets that preference to a factoryId. It seems suspicious to me that we check for that pref to be 'true'. This doesn't affect CDT but may affect extenders of CDT. 2- The property isDisassemblyEditorSupportsCBreakpoint which is associated with ToggleCBreakpointTester is not being tested by that tester. My guess is that it would break the disassembly editor (if someone enables it). 3- org.eclipse.cdt.debug.ui/plugin.xml does not test the property isDisassemblyEditorSupportsCBreakpoint in the case of tracepoints (ToggleCTracepointsTargetFactory) while it does for breakpoints. Is that intended? 4- In org.eclipse.cdt.debug.ui/plugin.xml, for both bp and tp, we used to check if the selection was: <or> <instanceof value="org.eclipse.cdt.core.model.IFunction"/> <instanceof value="org.eclipse.cdt.core.model.IMethod"/> <instanceof value="org.eclipse.cdt.core.model.IVariable"/> This has been replaced by <test property="org.eclipse.cdt.debug.ui.isCDeclarationSupportsCBreakpoint"/> but looking at ToggleCBreakpointTester.java for isCDeclarationSupportsCBreakpoint, it seems to immediately returns true if we are not using a custom toggleBreakpointFactory. How come we don't want to check for IFunction, IMethod, or IVariable here? I'm not very comfortable with some of those plugin.xml extensions, so I may be missing something. Thanks for your help.
(In reply to comment #10) > Patrick, > 1- CDebugUtils.isCustomToggleBreakpointFactory() checks if > ICDebugConstants.PREF_TOGGLE_BREAKPOINT_MODEL_IDENTIFIER is set to true > (Boolean.valueOf(customModel)). However, > CDebugUtils.setToggleBreakpointFactory() sets that preference to a factoryId. > It seems suspicious to me that we check for that pref to be 'true'. This > doesn't affect CDT but may affect extenders of CDT. isCustomToggleBreakpointFactory works at a global level to override all testers, it is a system property and not a preference. Where as setToggleBreakpointFactory required a project, where CDT extenders can set the toggle breakpoint factory on a project level. I guess it is a bit confussion to name the system property and project preference to have the same id. > 2- The property isDisassemblyEditorSupportsCBreakpoint which is associated with > ToggleCBreakpointTester is not being tested by that tester. My guess is that > it would break the disassembly editor (if someone enables it). Yes it will, disassembly editor was never make use of this tester, if some one want to enable disassembly editor, then this tester has to be extended. > 3- org.eclipse.cdt.debug.ui/plugin.xml does not test the property > isDisassemblyEditorSupportsCBreakpoint in the case of tracepoints > (ToggleCTracepointsTargetFactory) while it does for breakpoints. Is that > intended? There was no need to have tracepoints test when I implement this change, feel free to add additional test of tracepoints. > 4- In org.eclipse.cdt.debug.ui/plugin.xml, for both bp and tp, we used to check > if the selection was: > <or> > <instanceof value="org.eclipse.cdt.core.model.IFunction"/> > <instanceof value="org.eclipse.cdt.core.model.IMethod"/> > <instanceof value="org.eclipse.cdt.core.model.IVariable"/> > This has been replaced by > <test property="org.eclipse.cdt.debug.ui.isCDeclarationSupportsCBreakpoint"/> > > but looking at ToggleCBreakpointTester.java for > isCDeclarationSupportsCBreakpoint, it seems to immediately returns true if we > are not using a custom toggleBreakpointFactory. How come we don't want to > check for IFunction, IMethod, or IVariable here? I'm not very comfortable with > some of those plugin.xml extensions, so I may be missing something. This looks like a bug to me and it applies to all testers, the condition should be: if (CDebugUtils.isCustomToggleBreakpointFactory()) return false; Do you agree? I'll file a bug for it if this is the desire behavior.
(In reply to comment #11) > (In reply to comment #10) > > Patrick, > > 1- CDebugUtils.isCustomToggleBreakpointFactory() checks if > > ICDebugConstants.PREF_TOGGLE_BREAKPOINT_MODEL_IDENTIFIER is set to true > > (Boolean.valueOf(customModel)). However, > > CDebugUtils.setToggleBreakpointFactory() sets that preference to a factoryId. > > It seems suspicious to me that we check for that pref to be 'true'. This > > doesn't affect CDT but may affect extenders of CDT. > > isCustomToggleBreakpointFactory works at a global level to override all > testers, it is a system property and not a preference. Where as > setToggleBreakpointFactory required a project, where CDT extenders can set the > toggle breakpoint factory on a project level. I guess it is a bit confussion to > name the system property and project preference to have the same id. Ah, I see now. Yes the same name made me assume it was the same thing. Probably not a great idea :) but I think its fine to leave it now that its done. > > 2- The property isDisassemblyEditorSupportsCBreakpoint which is associated with > > ToggleCBreakpointTester is not being tested by that tester. My guess is that > > it would break the disassembly editor (if someone enables it). > > Yes it will, disassembly editor was never make use of this tester, if some one > want to enable disassembly editor, then this tester has to be extended. The plugin.xml file used to accept the disassembly editor, but the new tester does not. Although we don't use it in open-source, maybe someone else uses the disassembly editor? Maybe it would be safer to add a check for that property in the new tester, even if it always returns true. What do you think? > > 3- org.eclipse.cdt.debug.ui/plugin.xml does not test the property > > isDisassemblyEditorSupportsCBreakpoint in the case of tracepoints > > (ToggleCTracepointsTargetFactory) while it does for breakpoints. Is that > > intended? > > There was no need to have tracepoints test when I implement this change, feel > free to add additional test of tracepoints. I'm just trying to future-proof the code. The day we decide to enable the disassembly editor in CDT, I'm sure we will forget to add this test for tracepoints. I think we should add it now, especially since it was there before. > > 4- In org.eclipse.cdt.debug.ui/plugin.xml, for both bp and tp, we used to check > > if the selection was: > > <or> > > <instanceof value="org.eclipse.cdt.core.model.IFunction"/> > > <instanceof value="org.eclipse.cdt.core.model.IMethod"/> > > <instanceof value="org.eclipse.cdt.core.model.IVariable"/> > > This has been replaced by > > <test property="org.eclipse.cdt.debug.ui.isCDeclarationSupportsCBreakpoint"/> > > > > but looking at ToggleCBreakpointTester.java for > > isCDeclarationSupportsCBreakpoint, it seems to immediately returns true if we > > are not using a custom toggleBreakpointFactory. How come we don't want to > > check for IFunction, IMethod, or IVariable here? I'm not very comfortable with > > some of those plugin.xml extensions, so I may be missing something. > > This looks like a bug to me and it applies to all testers, the condition should > be: > > if (CDebugUtils.isCustomToggleBreakpointFactory()) > return false; > > Do you agree? I'll file a bug for it if this is the desire behavior. What does the extra code in the test for each property do? When I first read the code, I thought the idea was that the new code would only run for a custom toggleBreakpointFactory. But now, I'm not sure what was the original intent. Did you mean to always return false if we do use a custom toggleBreakpointFactory, and that the extra code is meant for the custom case?
(In reply to comment #12) > The plugin.xml file used to accept the disassembly editor, but the new tester > does not. Although we don't use it in open-source, maybe someone else uses the > disassembly editor? Maybe it would be safer to add a check for that property > in the new tester, even if it always returns true. What do you think? Yes it would better to have the disassembly editor check. Would you like me to to file a bug and provide the patch, or you will add this check yourself? I just don't want both of us to work on this at the same time. > I'm just trying to future-proof the code. The day we decide to enable the > disassembly editor in CDT, I'm sure we will forget to add this test for > tracepoints. I think we should add it now, especially since it was there > before. Sure, this should be just another line in the plugin.xml file. Do we still need to test for tracepoint action set? as in the plugin.xml file and what goes into the gut for tracepoint tester? > What does the extra code in the test for each property do? When I first read > the code, I thought the idea was that the new code would only run for a custom > toggleBreakpointFactory. But now, I'm not sure what was the original intent. > Did you mean to always return false if we do use a custom > toggleBreakpointFactory, and that the extra code is meant for the custom case? I confused myself, the tester should only continue to preform the additional checks if isCustomToggleBreakpointFactory()==true. :(
(In reply to comment #13) > (In reply to comment #12) > > The plugin.xml file used to accept the disassembly editor, but the new tester > > does not. Although we don't use it in open-source, maybe someone else uses the > > disassembly editor? Maybe it would be safer to add a check for that property > > in the new tester, even if it always returns true. What do you think? > > Yes it would better to have the disassembly editor check. Would you like me to > to file a bug and provide the patch, or you will add this check yourself? I > just don't want both of us to work on this at the same time. I'm at EclipseCon this week, so I won't be able to get to it until next week. If you have some time and can do it that would be great. > > I'm just trying to future-proof the code. The day we decide to enable the > > disassembly editor in CDT, I'm sure we will forget to add this test for > > tracepoints. I think we should add it now, especially since it was there > > before. > > Sure, this should be just another line in the plugin.xml file. Do we still need > to test for tracepoint action set? as in the plugin.xml file and what goes into > the gut for tracepoint tester? The plugin.xml file still does the test for the action set, and then goes to the tester, so I think we simply need to add that one line to plugin.xml. > > What does the extra code in the test for each property do? When I first read > > the code, I thought the idea was that the new code would only run for a custom > > toggleBreakpointFactory. But now, I'm not sure what was the original intent. > > Did you mean to always return false if we do use a custom > > toggleBreakpointFactory, and that the extra code is meant for the custom case? > > I confused myself, the tester should only continue to preform the additional > checks if isCustomToggleBreakpointFactory()==true. :( So, do we need to do anything about the fact that those lines where removed? > <or> > <instanceof value="org.eclipse.cdt.core.model.IFunction"/> > <instanceof value="org.eclipse.cdt.core.model.IMethod"/> > <instanceof value="org.eclipse.cdt.core.model.IVariable"/> Maybe there should be some additional checks in the testers for the custom case?
Patrick fixed all points in Bug 375871
