-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hardware/Software Breakpoint Modes (Breakpoint Types) #350
Hardware/Software Breakpoint Modes (Breakpoint Types) #350
Conversation
f41a90d
to
4114131
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't managed to run the hardware breakpoint tests in the Windows, conditions like isRemoteTest or !isRemoteTest seems failing too, please let me know, if there is a better Windows test conditions.
I don't know what support Windows native has for hardware breakpoints. If it does support it I am sure we can find a way to enable the tests. What happens if do do "hbreak main" in a manually launched GDB?
For example, here is a simple test on Linux to compare to windows:
$ gdb ./empty1.exe
GNU gdb (Ubuntu 15.0.50.20240403-0ubuntu1) 15.0.50.20240403-git
(gdb) hbreak main
No hardware breakpoint support in the target.
(gdb) break main
Breakpoint 1 at 0x115e: file ./empty1.c, line 17.
(gdb) run
Starting program: /scratch/debug/git/cdt-amalgamator/sampleWorkspace/empty1.exe
Breakpoint 1, main () at ./empty1.c:17
17 int local_in_main = function1();
(gdb) hbreak main
Note: breakpoint 1 also set at pc 0x55555555515e.
Hardware assisted breakpoint 2 at 0x55555555515e: file ./empty1.c, line 17.
(gdb)
As shown above, hbreak may not be available before details of the target are known, which happens here when I run.
PS Please include a more complete commit message in your next commit so that suitable history can end up in git. |
4114131
to
b35497e
Compare
I have a couple of findings. Firstly, I observe that hardware breakpoints only available in remote debugging in Windows, as you can find gdb logs here. Additionally, there is a stop condition issue in gdb async off, so, I am planning to use the skip condition as Local debugging with gdb
Remote debugging using gdb+gdbserver:
Secondly, it seems the
|
Hi @jonahgraham , I updated the pull-request, could you please review when you are suitable? Kind regards. |
Tried to made a more detailled commit message in recent update. |
src/mi/breakpoint.ts
Outdated
export interface MIBreakpointInsertOptions { | ||
temporary?: boolean; | ||
|
||
/** | ||
* The `mode` property is priotized over the `hardware` property. If `mode` is defined, then the information in the `hardware` flag is ignored during the insert breakpoint operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prettier doesn't wrap/format comments, so my suggestion is just to wrap this very long line.
* The `mode` property is priotized over the `hardware` property. If `mode` is defined, then the information in the `hardware` flag is ignored during the insert breakpoint operation. | |
* The `mode` property is priotized over the `hardware` property. | |
* If `mode` is defined, then the information in the `hardware` flag is | |
* ignored during the insert breakpoint operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the comments.
src/mi/breakpoint.ts
Outdated
* | ||
* - `'hardware'`: If user explicitly selects the breakpoint mode as 'Hardware Breakpoint' at the user interface. | ||
* - `'software'`: If user explicitly selects the breakpoint mode as 'Software Breakpoint' at the user interface. | ||
* - `undefined`: If user didn't make an explicitly breakpoint mode selection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* - `undefined`: If user didn't make an explicitly breakpoint mode selection. | |
* - `undefined`: If user didn't make an explicitly breakpoint mode selection, in this case the `hardware` flag will be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the comments.
Thank you. |
That makes sense - async is better all the time anyway.
Is the breakpoint modified arriving before the cdt-gdb-adapter/src/integration-tests/pause.spec.ts Lines 43 to 46 in c8015bc
If that isn't the cause, let me know. |
b35497e
to
d8fd943
Compare
It might be a different issue, let me provide some logs for discussion. When a "-break-insert" command send in Windows (for software breakpoint), after the configuration you can see in the following log that we are receiving a
On the other hand, when the similar code run for inserting a hardware breakpoint, there is no "breakpoint-modified" event found at the output:
One more detail: As far as I searched from the Windows test logs (the gdb test logs downloaded from github pipeline), there is no previously implemented 'hardware' breakpoint test running for Windows. So I believe, this is the reason why we didnot observe this issue in the current breakpoint tests. (I simply searched for the following regex: |
@asimgunes yesterday I looked at this and couldn't figure out what was happening at all. Today with a clear head I can see in the hardware case that the breakpoint simply isn't working, the execution is never hitting the breakpoint. The breakpoint modified is sent in the software case because the breakpoint is being hit and the modification is that Maybe we can discuss what to do on the call today? |
During our call today we decided to defer hardware breakpoint support on Windows host development. Therefore the issues discussed in #350 (comment) are now resolved. See the follow-up #351 |
|
||
// Only do the hardware breakpoint tests in Windows in remote debugging with gdbAsync on | ||
const skipHardwareBreakpointTest = | ||
os.platform() === 'win32' && (!isRemoteTest || !gdbAsync); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make this test simply skip on win32 until #351 is addressed. See #350 (comment) also
os.platform() === 'win32' && (!isRemoteTest || !gdbAsync); | |
os.platform() === 'win32'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
// 'breakpoint-modified' output doesn't received in hardware breakpoints in Windows | ||
if (os.platform() === 'win32' && breakpointMode === 'hardware') { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the suggestion at line 97, I think this can be removed?
// 'breakpoint-modified' output doesn't received in hardware breakpoints in Windows | |
if (os.platform() === 'win32' && breakpointMode === 'hardware') { | |
return; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
In this implementation, we introduced the "Breakpoint Mode" usage, which is recently introduced to the DAP. This implementation sends the breakpoint mode capabilities at the initialisation phase of the debug adapter. We implemented the following modes: * hardware: The mode `hardware` indicates the hardware breakpoints. * software: The mode `software` indicates the software breakpoints. In VSCode, user can choose the breakpoint mode after adding the breakpoint by editing the breakpoint details. We also introduce `mode` property to the `MIBreakpointInsertOptions` interface. If there is a value in the `mode` property, the breakpoint insert operation is prioritising the value of the `mode` over the `hardware` property. Additionally, `hardware` property flagged as deprecate and planning to be removed soon.
d8fd943
to
80abeea
Compare
This change is included in v1.0.5 on npm: https://www.npmjs.com/package/cdt-gdb-adapter/v/1.0.5 |
Hi @jonahgraham ,
We like to send an update for new DAP feature "Breakpoint Modes". Here in this update I added hardware and software breakpoint types where the debug adapter sends this capability to the IDE, and uses this information during the breakpoint operations.
I would appreciate it if you can check the pull request when you are suitable.
By the way, there could be some arguable decisions here, let me emphasis the important points:
I implemented the idea over 'hardware' and 'software' modes. By default, I enabled both of the modes, however I implemented a separate function
getBreakpointModes
to let user to override the function if they needed. Thus, user can override the behaviour and return any desired combination.In this new implementation, I priotize the
mode
information overhardware
flag, in other words, ifmode
is defined I considered themode
parameter, otherwise I checked thehardware
flag.hardware
flag became redundant after having themode
property:Previously we have a
hardware
flag onMIBreakpointInsertOptions
class, this flag became redundant after the having themode
property. I still left thehardware
flag for backward compatibility. Moreover, thehardware
controlled by launch arguments to force debug adapter to send the breakpoints as hardware breakpoints. Maybe we can consider to introduce adefaultBreakpointMode
parameter and remove thehardwareBreakpoint
parameter. I didnot make a change to avoid breaking the current users.I couldn't managed to run the hardware breakpoint tests in the Windows, conditions like isRemoteTest or !isRemoteTest seems failing too, please let me know, if there is a better Windows test conditions.
Kind regards,
Asim