Feat: Setting Hardware/Software Breakpoint Types via Editor#211
Feat: Setting Hardware/Software Breakpoint Types via Editor#211asimgunes wants to merge 2 commits intoeclipse-cdt-cloud:mainfrom
Conversation
|
Hi @jreineckearm , @jonahgraham , I completed my checks and I would like to hear your feedback on this update when you have suitable time. Kind regards. Asim |
jreineckearm
left a comment
There was a problem hiding this comment.
Nice, thanks for the PR. Good to see this initiative reaching completion. 🙂
See a couple of questions/nits: most are not blocking.
We need to see how the CDT-GDB: prefix in the context menu is perceived by users. I personally don't mind it for now. But I think it would be equally fine to not have it in the context menu but rather document it well once we add some better documentation about non-standard GUI features coming from the extension (similar needed for the radix stuff).
During testing I found a few things that might need attention:
- 1.) set a breakpoint through the column/margin, 2.) use one of the two new commands => I always see the log output
No breakpoint number <n>.Is there anything we can do to suppress this? It's not super-blocking and only happens when explicitly using one of the new commands. But made me look twice to ensure this isn't a result of something going wrong. I expect similar confusion at users' end. - 1.) set a breakpoint through the column/margin, 2.) use "set software breakpoint", 3.) call a
> info bfrom the debug console => all breakpoints are gone in GDB (still visible in GUI). I am afraid this needs another look. This doesn't happen when you use "set hardware breakpoint" first. - We probably should disable the commands for the command palette (will add a comment to the package.json). If I understand correctly, you can't do anything without providing parameters. I even get an error pop up when using it.
|
I am away now until Monday. Happy to review again then unless you got it completed until then with Jonah. |
|
I am a bit confused – you seem to imply that there previously was no UI to choose between hardware and software breakpoints, but I do seem to see such UI: When I right-click on a breakpoint in the Breakpoints view, there is a context menu item Edit Mode that lets me select Hardware Breakpoint or Software Breakpoint. When I right-click on a breakpoint in the editor margin, I can choose Edit Breakpoint and get a Mode popup menu with the same choice. I have not tried if these work (our stub does not provide hardware breakpoints). So, it seems like the advantage of your addition is only that, during a running debug session, it allows me to set a breakpoint with the desired mode from the start, instead of having to set it with the default mode first and then change the mode? (Despite not needing hardware breakpoints, I am watching this with some interest because we have other breakpoint-like things that we will probably want to present as breakpoints of a separate type in the future.) |
Hi @cwalther , you're right, I might misinterpret this during the PR description. The VS Code does expose the mode selection through "Edit Breakpoint" and "Edit Mode" in the Breakpoints view. This PR adds the ability to set the desired mode directly from the line number context menu in a single step. This was a UX improvement we had already applied in practice and are now contributing upstream to the community. |
|
Hi @jreineckearm , @jonahgraham , I made some improvements depending on your feedback, is it possible to re-check the implemtation and the comments please? Kind regards Asim |
64c468b to
81b9b50
Compare
There was a problem hiding this comment.
Sorry, had forgotten about this one. The code changes look good, thanks for addressing the feedback.
I'll see if I can retry the two scenarios where I saw unexpected behavior (No breakpoint number <n>. messages, and losing breakpoints in GDB) tomorrow morning. I don't have a board at hand right now.
I think losing the breakpoints should at least be captured as a defect for later if still present. At the same time, I don't see it as a strong blocker. So, feel free to merge and prepare a release without me having tested the latest changes. Especially, since I am on Easter break from tomorrow night on. And I don't want to hold up releases you require for your product release.
|
These two problems haven't been addressed or commented
The second IMO at least needs an issue in the backlog to follow up on. It leaves behind a zombie-breakpoint in the VS Code Breakpoint dialog. As I wrote in my previous comment, I am away now and don't want to hold up the release. So, I am fine to go with this defect. |
|
Hi @jreineckearm , I investigate on your feedback about "No breakpoint number ..." and the second concern. I found an interesting behaviour which seems out of context for this update but we may need to check this in the breakpoint handling logic in In this new implementation, we are checking the current breakpoints and if the breakpoint mode is changed between hardware and software breakpoint types we are deleting the current one and creating a new breakpoint through In the logs below you can see both functions triggered I believe we need to include a mutex guard in the adapter layer to avoid such conditions since the protocol seems open for such invocations. I like to hear your thoughs. If we agree, I can arrange the adapter update. |
|
@asimgunes , would breakpoint I believe, you can send an updated mode. But might need some experimenting first: That way, you might be able to avoid the race condition by add/remove operations through the API. Otherwise, I agree we'd need some way of synchronization.... |
|
I don’t think that you can update a breakpoint mode using a DAP event (at least not officially). The DAP Breakpoint type that goes from adapter to client has fewer properties than the SourceBreakpoint type that goes from client to adapter. They are not subclasses of each other. That distinction had me confused for a while when I was recently examining a similar problem (trying to update a breakpoint condition from the adapter). Regardless, I agree that the adapter should be able to deal with breakpoint requests that come in before previous ones are completed. |
|
Another thought is whether If that neither helps, then yes. We'd need to guard sequences of MI breakpoint commands by a mutex or install a proper breakpoint command queue. And apply consistently wherever we use MI breakpoint commands. I am still fine to merge this PR here as long as we capture your findings in a GH issue to address separately. |
Summary
This PR exposes hardware and software breakpoint type selection to end users directly from the editor context menu, making a capability that has long existed at the adapter level easy to discover and use.
The underlying DAP support for breakpoint modes was already implemented in
cdt-gdb-adapter. This change wires that adapter capability into the VS Code extension UI so users no longer need to manually configure breakpoint types through launch configuration or GDB console commands.This feature was discussed and tracked as an initiative in CDT Cloud community meetings (starting February 2024), with evaluation of the GDB DAP adapter proof-of-concept carried out earlier. This PR represents the VS Code extension layer of that initiative.
Changes
src/BreakpointModesController.ts— New controller that registers two commands:cdt.debug.breakpoints.addHardwareBreakpointcdt.debug.breakpoints.addSoftwareBreakpointEach command removes any existing breakpoint on the target line and re-adds it with the desired mode injected via a workaround on the
SourceBreakpointobject (necessary due to a current VS Code API limitation — the mode is passed correctly through DAP but is not reflected in the breakpoints panel UI).The controller also exports
arePathsEqual, a path comparison utility that handles normalisation and Windows case/slash insensitivity.src/extension.ts— Activates theBreakpointModesController.package.json— Contributes the two commands and surfaces them in the editor line number context menu (editor/lineNumber/context), withinDebugModeguards in the Command Palette.How It Works
Right-clicking on a line number in the editor while a debug session is active shows:
If a breakpoint already exists on the line with the requested mode, the command is a no-op. Otherwise the existing breakpoint is replaced.
Known Limitation
VS Code does not currently expose a public API to set
modeon aSourceBreakpoint. The mode is injected via(sp as any).mode = mode, which works functionally and is passed correctly in DAPsetBreakpointsrequests — but the breakpoint mode is not visible in the VS Code Breakpoints panel.For more information: microsoft/vscode#304764