Skip to content
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

Merged

Conversation

asimgunes
Copy link
Contributor

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:

  1. Enabling the breakpointModes as Hardware and Software:

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 over hardware flag, in other words, if mode is defined I considered the mode parameter, otherwise I checked the hardware flag.

  1. hardware flag became redundant after having the mode property:

Previously we have a hardware flag on MIBreakpointInsertOptions class, this flag became redundant after the having the mode property. I still left the hardware flag for backward compatibility. Moreover, the hardware controlled by launch arguments to force debug adapter to send the breakpoints as hardware breakpoints. Maybe we can consider to introduce a defaultBreakpointMode parameter and remove the hardwareBreakpoint parameter. I didnot make a change to avoid breaking the current users.

  1. Windows tests:

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

@asimgunes asimgunes force-pushed the improvement/breakpoint-modes branch from f41a90d to 4114131 Compare February 3, 2025 16:50
Copy link
Contributor

@jonahgraham jonahgraham left a 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.

src/gdb/GDBDebugSessionBase.ts Show resolved Hide resolved
src/gdb/GDBDebugSessionBase.ts Outdated Show resolved Hide resolved
src/integration-tests/test-programs/core Outdated Show resolved Hide resolved
src/mi/breakpoint.ts Show resolved Hide resolved
@jonahgraham
Copy link
Contributor

PS Please include a more complete commit message in your next commit so that suitable history can end up in git.

@asimgunes asimgunes force-pushed the improvement/breakpoint-modes branch from 4114131 to b35497e Compare February 4, 2025 16:27
@asimgunes
Copy link
Contributor Author

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?

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 skipHardwareBreakpointTest = os.platform() === 'win32' && (!isRemoteTest || !gdbAsync).

Local debugging with gdb

> gdb count.exe
GNU gdb (GDB) 15.1
...
(gdb) hbreak main
No hardware breakpoint support in the target.
(gdb) break main
Breakpoint 1 at 0x40146e: file count.c, line 2.

Remote debugging using gdb+gdbserver:

> gdb
GNU gdb (GDB) 15.1
...
(gdb) target remote localhost:2345
Remote debugging using localhost:2345
...
(gdb) hbreak main
Hardware assisted breakpoint 1 at 0x40146e: file count.c, line 2.

Secondly, it seems the while loop in the test (which I copied from 'breakpoints.spec.ts') is causing timeout. In Gitlab tests, we are not receiving the 'breakpoint-modified' output for 'hardware' breakpoints, on the other hand, I am not receiving the output for both 'hardware'&'software' breakpoints in my local environment (local environment uses GDB 15.1, github tests uses GDB 11).

let outputs;
while (!isCorrect) {
	// Cover the case of getting event in Linux environment.
	// If cannot get correct event, program timeout and test case failed.
	outputs = await dc.waitForEvent('output');
	isCorrect = outputs.body.output.includes('breakpoint-modified');
}

@asimgunes
Copy link
Contributor Author

Hi @jonahgraham ,

I updated the pull-request, could you please review when you are suitable?

Kind regards.

@asimgunes asimgunes requested a review from jonahgraham February 4, 2025 16:32
@asimgunes
Copy link
Contributor Author

PS Please include a more complete commit message in your next commit so that suitable history can end up in git.

Tried to made a more detailled commit message in recent update.

src/mi/breakpoint.ts Show resolved Hide resolved
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.
Copy link
Contributor

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.

Suggested change
* 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comments.

*
* - `'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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* - `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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comments.

src/gdb/GDBDebugSessionBase.ts Show resolved Hide resolved
@jonahgraham
Copy link
Contributor

PS Please include a more complete commit message in your next commit so that suitable history can end up in git.

Tried to made a more detailled commit message in recent update.

Thank you.

@jonahgraham
Copy link
Contributor

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 skipHardwareBreakpointTest = os.platform() === 'win32' && (!isRemoteTest || !gdbAsync).

That makes sense - async is better all the time anyway.


Secondly, it seems the while loop in the test [...]

Is the breakpoint modified arriving before the waitForEvent (you should see it in the log file)? If so, this issue is quite prevalent in the adapter tests. I have fixed them in some places but the basic solution is you have to issue the waitForEvent before the operation that may trigger the event. Here is a simple example

const waitForStopped = dc.waitForEvent('stopped');
const threads = await dc.threadsRequest();
const pr = dc.pauseRequest({ threadId: threads.body.threads[0].id });
await Promise.all([pr, waitForStopped]);

If that isn't the cause, let me know.

@asimgunes asimgunes force-pushed the improvement/breakpoint-modes branch from b35497e to d8fd943 Compare February 5, 2025 09:53
@asimgunes
Copy link
Contributor Author

Is the breakpoint modified arriving before the waitForEvent (you should see it in the log file)? If so, this issue is quite prevalent in the adapter tests. I have fixed them in some places but the basic solution is you have to issue the waitForEvent before the operation that may trigger the event. Here is a simple example

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 breakpoint-modified event.

[13:44:57.034 UTC] GDB command: 6 -break-insert --source "D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c" --line 4
[13:44:57.050 UTC] GDB result: 6 done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x00007ff670f6146b",func="main",file="count.c",fullname="D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c",line="4",thread-groups=["i1"],times="0",original-location="-source D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c -line 4"}
[13:44:57.050 UTC] To client: {"seq":0,"type":"response","request_seq":3,"command":"setBreakpoints","success":true,"body":{"breakpoints":[{"id":1,"line":4,"verified":true}]}}
[13:44:57.051 UTC] From client: configurationDone(undefined)
[13:44:57.051 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"console","output":"\nIn the Debug Console view you can interact directly with GDB.\nTo display the value of an expression, type that expression which can reference\nvariables that are in scope. For example type '2 + 3' or the name of a variable.\nArbitrary commands can be sent to GDB by prefixing the input with a '>',\nfor example type '>show version' or '>help'.\n\n"}}
[13:44:57.051 UTC] GDB command: 7 -exec-continue
[13:44:57.064 UTC] GDB result: 7 running
[13:44:57.064 UTC] GDB exec async: running,thread-id="all"
[13:44:57.065 UTC] To client: {"seq":0,"type":"event","event":"continued","body":{"threadId":1,"allThreadsContinued":true}}
[13:44:57.065 UTC] To client: {"seq":0,"type":"response","request_seq":4,"command":"configurationDone","success":true}
[13:44:57.067 UTC] GDB notify async: breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x00007ff670f6146b",func="main",file="count.c",fullname="D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c",line="4",thread-groups=["i1"],times="1",original-location="-source D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c -line 4"}
[13:44:57.068 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"\n"}}
[13:44:57.068 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"Thread 1 hit Breakpoint 1, main () at count.c:4\n"}}
[13:44:57.068 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"4\t        count ++; \n"}}
[13:44:57.068 UTC] GDB exec async: stopped,reason="breakpoint-hit",disp="keep",bkptno="1",frame={addr="0x00007ff670f6146b",func="main",args=[],file="count.c",fullname="D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c",line="4",arch="i386:x86-64"},thread-id="1",stopped-threads="all"
[13:44:57.069 UTC] To client: {"seq":0,"type":"event","event":"stopped","body":{"reason":"breakpoint","allThreadsStopped":true,"threadId":1}}
[13:44:57.069 UTC] From client: disconnect(undefined)
[13:44:57.070 UTC] GDB command: 8 disconnect

On the other hand, when the similar code run for inserting a hardware breakpoint, there is no "breakpoint-modified" event found at the output:
(the following log happens when we enter the while loop to wait the "breakpoint-modified" line at the output. as you can find, the disconnect request sent after a timeout duration. So the root cause doesn't seem like a timing/listening issue)

[13:44:43.675 UTC] GDB command: 6 -break-insert -h --source "D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c" --line 4
[13:44:43.691 UTC] GDB result: 6 done,bkpt={number="1",type="hw breakpoint",disp="keep",enabled="y",addr="0x00007ff670f6146b",func="main",file="count.c",fullname="D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c",line="4",thread-groups=["i1"],times="0",original-location="-source D:\\a\\cdt-gdb-adapter\\cdt-gdb-adapter\\src\\integration-tests\\test-programs\\count.c -line 4"}
[13:44:43.691 UTC] To client: {"seq":0,"type":"response","request_seq":3,"command":"setBreakpoints","success":true,"body":{"breakpoints":[{"id":1,"line":4,"verified":true}]}}
[13:44:43.692 UTC] From client: configurationDone(undefined)
[13:44:43.692 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"console","output":"\nIn the Debug Console view you can interact directly with GDB.\nTo display the value of an expression, type that expression which can reference\nvariables that are in scope. For example type '2 + 3' or the name of a variable.\nArbitrary commands can be sent to GDB by prefixing the input with a '>',\nfor example type '>show version' or '>help'.\n\n"}}
[13:44:43.692 UTC] GDB command: 7 -exec-continue
[13:44:43.705 UTC] GDB result: 7 running
[13:44:43.705 UTC] GDB exec async: running,thread-id="all"
[13:44:43.705 UTC] To client: {"seq":0,"type":"event","event":"continued","body":{"threadId":1,"allThreadsContinued":true}}
[13:44:43.706 UTC] To client: {"seq":0,"type":"response","request_seq":4,"command":"configurationDone","success":true}
[13:44:56.221 UTC] From client: disconnect(undefined)
[13:44:56.222 UTC] GDB command: 8 -exec-interrupt --all
[13:44:56.229 UTC] GDB result: 8 done
[13:44:56.480 UTC] GDB notify async: thread-created,id="3",group-id="i1"
[13:44:56.481 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"[New Thread 588]\n"}}
[13:44:56.481 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"\nThread "}}
[13:44:56.481 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"3 received signal SIGINT, Interrupt.\n"}}
[13:44:56.481 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"[Switching to Thread 588]\n"}}
[13:44:56.482 UTC] To client: {"seq":0,"type":"event","event":"output","body":{"category":"stdout","output":"0x00007ffb091c4c8b in KERNELBASE!CtrlRoutine () from target:C:\\Windows\\System32\\KernelBase.dll\n"}}
[13:44:56.482 UTC] GDB exec async: stopped,reason="signal-received",signal-name="SIGINT",signal-meaning="Interrupt",frame={addr="0x00007ffb091c4c8b",func="KERNELBASE!CtrlRoutine",args=[],from="target:C:\\Windows\\System32\\KernelBase.dll",arch="i386:x86-64"},thread-id="3",stopped-threads="all"
[13:44:56.482 UTC] GDB command: 9 disconnect

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: -break-insert(.*)-h)

@asimgunes asimgunes requested a review from jonahgraham February 5, 2025 14:02
@jonahgraham
Copy link
Contributor

@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 times is incremented.

Maybe we can discuss what to do on the call today?

@jonahgraham
Copy link
Contributor

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);
Copy link
Contributor

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

Suggested change
os.platform() === 'win32' && (!isRemoteTest || !gdbAsync);
os.platform() === 'win32';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment on lines 75 to 78
// 'breakpoint-modified' output doesn't received in hardware breakpoints in Windows
if (os.platform() === 'win32' && breakpointMode === 'hardware') {
return;
}
Copy link
Contributor

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?

Suggested change
// 'breakpoint-modified' output doesn't received in hardware breakpoints in Windows
if (os.platform() === 'win32' && breakpointMode === 'hardware') {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

src/integration-tests/breakpointModes.spec.ts Show resolved Hide resolved
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.
@asimgunes asimgunes force-pushed the improvement/breakpoint-modes branch from d8fd943 to 80abeea Compare February 7, 2025 15:18
@asimgunes asimgunes requested a review from jonahgraham February 7, 2025 15:33
@jonahgraham jonahgraham merged commit f666a26 into eclipse-cdt-cloud:main Feb 10, 2025
4 checks passed
@jonahgraham
Copy link
Contributor

This change is included in v1.0.5 on npm: https://www.npmjs.com/package/cdt-gdb-adapter/v/1.0.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants