Bugzilla will undergo maintenance 2024-03-29 18h00 CET. Bugzilla will be placed in read-only mode at that time.

Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 332993 - Hardware breakpoint cannot be set from debug GUI
Summary: Hardware breakpoint cannot be set from debug GUI
Status: NEW
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug (show other bugs)
Version: 8.0   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: cdt-debug-inbox@eclipse.org CLA
QA Contact: Jonah Graham CLA
URL:
Whiteboard:
Keywords:
Depends on: 395404 336875 360588
Blocks:
  Show dependency tree
 
Reported: 2010-12-20 23:58 EST by Tien Hock Loh CLA
Modified: 2020-09-04 15:27 EDT (History)
9 users (show)

See Also:


Attachments
HW Breakpoint GUI (90.03 KB, patch)
2011-03-14 22:42 EDT, Tien Hock Loh CLA
no flags Details | Diff
Patch for the enhancement (18.70 KB, patch)
2011-05-17 05:53 EDT, Tien Hock Loh CLA
no flags Details | Diff
Patch that adds "Toggle Hardware Breakpoint" action. (15.56 KB, patch)
2013-02-19 18:38 EST, Nobody - feel free to take it CLA
nobody: iplog-
Details | Diff
Disassembly and images (21.87 KB, patch)
2014-02-07 09:07 EST, Teodor Madan CLA
teodor.madan: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tien Hock Loh CLA 2010-12-20 23:58:59 EST
Build Identifier: M20100211-1343

We can create a software breakpoint now, but hardware breakpoint is unsupported even though the back end of MIBreakpoint and CDIBreakpoint does support them. My suggestion is that we add a new IToggleBreakpointsTargetFactory that mimics ToggleCBreakpointsTargetFactory and ToggleBreakpointAdapter, but creates a hardware breakpoint instead of the regular ones (using CDIDebugModel.createLineBreakpoint, but using ICBreakpointType.HARDWARE instead). If you think this is a good suggestion, we can create a patch and contribute to Eclipse. 

Reproducible: Always
Comment 1 Tien Hock Loh CLA 2011-01-18 19:59:33 EST
The workflow we are currently planning is

1. Add a new breakpoint type in the Run -> Breakpoint Types menu called "Hardware Breakpoint"
2. Subsequent newly created breakpoint (eg. double clicking on the ruler) will be creating the breakpoint via a new IToggleBreakpointTargetFactory, and the ToggleBreakpointAdapter class will be duplicated, and the calls to CDIDebugModel.createLineBreakpoint will be passed in with ICBreakpointType.HARDWARE. 
3. This will enable user to create a hardware breakpoint.

Please let me know if this workflow is reasonable.
Comment 2 Marc Khouzam CLA 2011-01-19 15:48:15 EST
Moving to general debug since those changes will be in general CDT and can be used by any debugger integration.
Comment 3 Marc Khouzam CLA 2011-01-19 16:02:24 EST
(In reply to comment #1)
> The workflow we are currently planning is
> 
> 1. Add a new breakpoint type in the Run -> Breakpoint Types menu called
> "Hardware Breakpoint"
> 2. Subsequent newly created breakpoint (eg. double clicking on the ruler) will
> be creating the breakpoint via a new IToggleBreakpointTargetFactory, and the
> ToggleBreakpointAdapter class will be duplicated, and the calls to
> CDIDebugModel.createLineBreakpoint will be passed in with
> ICBreakpointType.HARDWARE. 

I think you are right that you will need to duplicate ToggleBreakpointAdapter.  

Will you be using an actionSet to hide the concept of Hardware breakpoints, until the user chooses to make them accessible?  If so, then you do need a new class that implements IToggleBreakpointTargetFactory.  But if you won't use an actionSet, then you can probably simply add a new target within ToggleCBreakpointsTargetFactory instead of creating a new class.

> 3. This will enable user to create a hardware breakpoint.
> 
> Please let me know if this workflow is reasonable.

Looks good to me.

Will you support adding hw breakpoints from the Disassembly view?  Then you will need add stuff to org.eclipse.cdt.dsf.debug.internal.ui.ToggleBreakpointsTargetFactory or create a new class for it.  You can look at org.eclipse.cdt.dsf.gdb.internal.ui.breakpoints.ToggleTracepointsTargetFactory which supports Tracepoints from the disassembly view.
Comment 4 Tien Hock Loh CLA 2011-01-24 00:07:58 EST
(In reply to comment #3)
> I think you are right that you will need to duplicate ToggleBreakpointAdapter.  
> 
> Will you be using an actionSet to hide the concept of Hardware breakpoints,
> until the user chooses to make them accessible?  If so, then you do need a new
> class that implements IToggleBreakpointTargetFactory.  But if you won't use an
> actionSet, then you can probably simply add a new target within
> ToggleCBreakpointsTargetFactory instead of creating a new class.

I think we won't need a new actionSet to hide the Hardware breakpoints. I'll look into adding a new target within the ToggleCBreakpointsTargetFactory.
Comment 5 Marc Khouzam CLA 2011-01-24 11:58:00 EST
(In reply to comment #4)
> I think we won't need a new actionSet to hide the Hardware breakpoints. I'll
> look into adding a new target within the ToggleCBreakpointsTargetFactory.

+1
Comment 6 Pawel Piech CLA 2011-01-24 12:12:21 EST
(In reply to comment #1)
> Please let me know if this workflow is reasonable.

This was one of my intended use cases for the toggle target extension point, so I hope it's reasonable :-)
Comment 7 John Cortell CLA 2011-01-28 16:48:15 EST
(In reply to comment #5)
> (In reply to comment #4)
> > I think we won't need a new actionSet to hide the Hardware breakpoints. I'll
> > look into adding a new target within the ToggleCBreakpointsTargetFactory.
> 
> +1

I think being able to hide the HW breakpoint option is important. For one, not all target environments support them. Secondly, I think companies have been coming up with custom solutions. Switching to this common approach may not be feasible or desirable. If we can hide the breakpoint type via a Capability, then that's good enough for me. I just don't know off the top of my head if that's currently supported. One way or another, though, we should effectively make this new breakpoint type "avoidable".
Comment 8 Marc Khouzam CLA 2011-01-29 13:15:31 EST
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I think we won't need a new actionSet to hide the Hardware breakpoints. I'll
> > > look into adding a new target within the ToggleCBreakpointsTargetFactory.
> > 
> > +1
> 
> I think being able to hide the HW breakpoint option is important. For one, not
> all target environments support them. Secondly, I think companies have been
> coming up with custom solutions. Switching to this common approach may not be
> feasible or desirable. If we can hide the breakpoint type via a Capability,
> then that's good enough for me. I just don't know off the top of my head if
> that's currently supported. One way or another, though, we should effectively
> make this new breakpoint type "avoidable".

Those are good points.  I don't know about Capabilities, but I now the actionSet will work.  It can be handled exactly like Tracepoints.
Comment 9 Tien Hock Loh CLA 2011-01-31 01:44:41 EST
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I think we won't need a new actionSet to hide the Hardware breakpoints. I'll
> > > look into adding a new target within the ToggleCBreakpointsTargetFactory.
> > 
> > +1
> 
> I think being able to hide the HW breakpoint option is important. For one, not
> all target environments support them. Secondly, I think companies have been
> coming up with custom solutions. Switching to this common approach may not be
> feasible or desirable. If we can hide the breakpoint type via a Capability,
> then that's good enough for me. I just don't know off the top of my head if
> that's currently supported. One way or another, though, we should effectively
> make this new breakpoint type "avoidable".

Hiding and not hiding has their pros and cons IMO. 

I think the reason behind companies coming up with custom solutions is that currently there's really no way to set hardware breakpoint, and if this kind of changes get integrated into Eclipse, it might even be beneficial to them. 

I'll be implementing actionSet to allow user to hide them as per your comment.
Comment 10 Tien Hock Loh CLA 2011-02-09 23:54:57 EST
I'm unable to add a new factory for the Disassembler View in DSF. It currently only picks up the first factory and always uses that for adding a new breakpoint. 

I'll not add the support for creating a new hardware breakpoint in the Disassembly view in DSF.
Comment 11 Marc Khouzam CLA 2011-02-10 12:23:24 EST
(In reply to comment #10)
> I'm unable to add a new factory for the Disassembler View in DSF. It currently
> only picks up the first factory and always uses that for adding a new
> breakpoint. 

Amazingly you made me realize that setting Tracepoints from the Disassembly view has been broken from the start!

I just fixed it in Bug 336847.
If you do the same fix for your factory (5 lines) I believe it will work.
Comment 12 Nobody - feel free to take it CLA 2011-02-10 18:08:14 EST
Please look at https://bugs.eclipse.org/bugs/show_bug.cgi?id=336875. Would it be helpful for you?
Comment 13 Tien Hock Loh CLA 2011-02-13 20:56:57 EST
> (In reply to comment #10)
> > I'm unable to add a new factory for the Disassembler View in DSF. It currently
> > only picks up the first factory and always uses that for adding a new
> > breakpoint. 
> 
> Amazingly you made me realize that setting Tracepoints from the Disassembly
> view has been broken from the start!
> 
> I just fixed it in Bug 336847.
> If you do the same fix for your factory (5 lines) I believe it will work.

I'll add in the the factory for the Disassembler View and try it out. Thanks for the quick fix. 

(In reply to comment #12)
> Please look at https://bugs.eclipse.org/bugs/show_bug.cgi?id=336875. Would it
> be helpful for you?

Yeah, it would reduce code duplication. Any ETA on the integration of this patch? I'll just apply the patch locally for now. 
Thanks for the great work.
Comment 14 Nobody - feel free to take it CLA 2011-02-14 11:18:25 EST
(In reply to comment #13)
> Yeah, it would reduce code duplication. Any ETA on the integration of this
> patch? I'll just apply the patch locally for now. 
I was just waiting for a feedback from you, now I can commit it. Thanks.
Comment 15 Tien Hock Loh CLA 2011-02-25 03:19:27 EST
Changes are done, but I haven't got time to create patch and thoroughly verify.
Comment 16 Marc Khouzam CLA 2011-03-03 14:13:38 EST
(In reply to comment #15)
> Changes are done, but I haven't got time to create patch and thoroughly verify.

Can you generate a patch for what you have and post it?  Maybe someone else can test it.
Comment 17 Tien Hock Loh CLA 2011-03-14 22:42:53 EDT
Created attachment 191184 [details]
HW Breakpoint GUI
Comment 18 Nobody - feel free to take it CLA 2011-03-24 12:29:39 EDT
(In reply to comment #17)
> Created attachment 191184 [details]
> HW Breakpoint GUI

Is this patch against the HEAD? I am having trouble applying it. Please, check if it works with the latest CDT.
Comment 19 Tien Hock Loh CLA 2011-03-28 03:44:37 EDT
Hi Mikhail,
I'll check on this against HEAD in a couple of days.
Comment 20 Tien Hock Loh CLA 2011-05-17 05:53:27 EDT
Created attachment 195828 [details]
Patch for the enhancement

Attached patch to HEAD.
Comment 21 Tien Hock Loh CLA 2011-07-25 20:50:29 EDT
Hi,

Its been a while since the patch is submitted. Would the patch be integrated? 

Thanks
Comment 22 Branko Drevensek CLA 2012-03-01 16:54:19 EST
I'd really like to see this in Eclipse. :) Is there a way I could help (except pinging here)?
Comment 23 Pawel Piech CLA 2012-03-01 17:59:06 EST
I'm in process of expanding various breakpoint creation APIs (see but 360588).  You should be able to create your own customized toggle breakpoint adapter given that there is some disagreement here about whether this option should be turned on by default.
Comment 24 Marc Khouzam CLA 2012-11-29 15:24:18 EST
This bug may be affected by bug 395404
Comment 25 Marc Khouzam CLA 2012-11-29 16:30:43 EST
To help the review, I've pushed the patch to Gerrit with author: Tien Hock Loh.
https://git.eclipse.org/r/8945
Comment 26 Nobody - feel free to take it CLA 2013-02-19 18:38:00 EST
Created attachment 227302 [details]
Patch that adds "Toggle Hardware Breakpoint" action.
Comment 27 Nobody - feel free to take it CLA 2013-02-19 18:38:41 EST
Attaching a patch that adds the "Toggle Hardware Breakpoint" action to the C/C++ and ASM editors and the Disassembly view. The patch was requested by Marc during the multi-core call.
In my opinion the better way would be to make the "Type" field of the new "Add Breakpoint" dialog editable and support three types "Regular", "Hardware" and "Temporary".
Marc, what do you think?
Comment 28 Marc Khouzam CLA 2013-02-20 09:22:47 EST
(In reply to comment #27)
> Attaching a patch that adds the "Toggle Hardware Breakpoint" action to the
> C/C++ and ASM editors and the Disassembly view. The patch was requested by
> Marc during the multi-core call.
> In my opinion the better way would be to make the "Type" field of the new
> "Add Breakpoint" dialog editable and support three types "Regular",
> "Hardware" and "Temporary".
> Marc, what do you think?

I like that.  It is less intrusive in the UI and gives access to both hardware and temporary breakpoints.

I'm assuming that hardware or temporary breakpoints are not going to be used a lot and therefore that it won't be too annoying to have to manually specify that type for each breakpoint.

We could even add the other bp types, such as Tracepoints.
Comment 29 Derek Morris CLA 2014-01-24 10:57:54 EST
Is this going to be supported in a standard CDT release any time soon?
Comment 30 Teodor Madan CLA 2014-02-07 09:07:31 EST
Created attachment 239738 [details]
Disassembly and images

Follow-up patch from https://git.eclipse.org/r/#/c/8945/
- Move ToggleCHWBreakpointsTargetFactory/ToggleHWBreakpointAdapter along the rest of breakpoint factories/adapters
- Add ToggleHWBreakpointAdapter for disassembly part
- Add missing HW break images
- Enable breakpoint type field readonly display in property page.

I could not upload directly to gerrit as the original patch from Tien Hock Loh does not have CLA.

To apply all source to cdt master issue the following commangs:
git fetch https://git.eclipse.org/r/cdt/org.eclipse.cdt refs/changes/45/8945/2 && git cherry-pick FETCH_HEAD
git apply 0001-Bug-332993_part2.patch

where 0001-Bug-332993_part2.patch is attached patch
Comment 31 Teodor Madan CLA 2014-02-07 09:16:48 EST
Mikhail, 

Do you think it is Ok for me to follow on HW breakpoints enhancements in a separate bug entry? 
This patch would address the complete lack of the way to set a HW breakpoint and adding incrementally the enhancements.
Comment 32 Teodor Madan CLA 2014-02-11 08:18:07 EST
(In reply to Mikhail Khodjaiants from comment #27)
> In my opinion the better way would be to make the "Type" field of the new
> "Add Breakpoint" dialog editable and support three types "Regular",
> "Hardware" and "Temporary".
> Marc, what do you think?

I have created Bug 427898 for editable "Type" field to track that work.
Comment 33 Teodor Madan CLA 2014-02-11 12:48:47 EST
(In reply to Teodor Madan from comment #30)
> Created attachment 239738 [details]
> Disassembly and images
> 
> Follow-up patch from https://git.eclipse.org/r/#/c/8945/
> - Move ToggleCHWBreakpointsTargetFactory/ToggleHWBreakpointAdapter along the
> rest of breakpoint factories/adapters
> - Add ToggleHWBreakpointAdapter for disassembly part
> - Add missing HW break images
> - Enable breakpoint type field readonly display in property page.
> 
> I could not upload directly to gerrit as the original patch from Tien Hock
> Loh does not have CLA.
> 

I have pushed to a separate gerrit review to be easy to apply/try-out.
The new gerrit review link is:

https://git.eclipse.org/r/#/c/21827/
Comment 34 Teodor Madan CLA 2014-02-11 12:49:34 EST
Comment on attachment 239738 [details]
Disassembly and images

Obsolete. The code is pushed into gerrit
Comment 35 Teodor Madan CLA 2014-02-11 12:50:35 EST
Comment on attachment 195828 [details]
Patch for the enhancement

The patch has been pushed to gerrit