Allow to set data breakpoints

[haydar-metin]

What it does

Allows to set data breakpoints within the memory table.

Peek.2024-05-30.12-16.mp4

Breakpoints set externally will have a dashed outline, internal breakpoints will be solid.

Extension API

The VSCode extension API does not support creating Data Breakpoints directly through their API. The method extHostDebugService.ts#addBreakpoints currently lacks the ability to forward data breakpoints (i.e., missing DTOs), while MainThreadDebugService.ts#$registerBreakpoints already supports it. However, we only have access to the extension API which uses the extHostDebugService internally. That means, we don't have a way to tell VSCode to show new Data Breakpoints within their UI! Consequently, we need to manually track our breakpoints (for now).

microsoft/vscode#195151

Debugger / Logs

Unfortunately, the debugger does not return the hitpoints on a stop event (it is not set). For that reason, to see the inline breakpoint marker, you need to set the data breakpoint through VSCode. For the prototype, all data breakpoints set from the outside will be assumed to be always hit for demonstration purposes.

The logs and workarounds for showcasing the prototype will be removed before merging.

https://microsoft.github.io/debug-adapter-protocol/specification

How to test

1. Start the debugger
2. Open the memory inspector
3. Set data-breakpoints through the context menu for variables and groups.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the contribution guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the code of conduct

[colin-grant-work]

I haven't looked at the code in detail, but I'm a little confused about the 'internal' vs. 'external' data breakpoint accounting. It looks like 'internal' breakpoints are those set through the Memory Inspector UI while 'external' are set by other means, but I'm not sure that that that should really matter. Are there differences in what the user can / should do with the two types?

[colin-grant-work]

This looks like it should perhaps be done as decorations rather than class names.

[colin-grant-work]

This also suggests that context menu context generation should perhaps be a contributable activity rather than hardcoded in a single function.

[colin-grant-work]

A bit suspicious that the breakpoint service stands alone here. Can its functionality be broken down into more composable parts?

[colin-grant-work]

Tracked here may be a bit of distracting detail that shouldn't be part of the name: it doesn't contrast with any untracked counterpart, but it raises the question of whether one should exist.

[haydar-metin]

I haven't looked at the code in detail, but I'm a little confused about the 'internal' vs. 'external' data breakpoint accounting. It looks like 'internal' breakpoints are those set through the Memory Inspector UI while 'external' are set by other means, but I'm not sure that that that should really matter. Are there differences in what the user can / should do with the two types?

@colin-grant-work You are absolutely right. However, due to the problem with not being able to set data breakpoints in the VS Code extension API, this was a way to overcame it.

We have the following situation right now:
The user can set new data breakpoints within the debugger UI (VSCode). Setting those will trigger a SetDataBreakpoints request with the local data VSCode itself has. That means, it will override our own data breakpoints we have set as we can not tell VSCode about those (See PR description - the API does not allow to forward any data breakpoints to the debugger UI).

For this reason, the current solution tracks all data breakpoints and splits them into external and internal (internal => MemoryInspector). Now, if someone sets from the outside a new data breakpoint, then our internal breakpoints will be appended to the request, so that our internal breakpoints will be still active.

If you can recommend a better/stable approach I would be glad to implement it to overcome the API issue.

[colin-grant-work]

I agree with the problem statement, but I think the details don't need to leak as widely as they currently do: we're strictly more knowledgeable / powerful than the VSCode UI, because we know about their breakpoints, but they don't know about our breakpoints.

This has two consequences:

• Breakpoints we create won't show up in their GUI
• Breakpoints they create and we delete (if we allow deletion of their breakpoints) won't disappear from their GUI

That is, both internal and external breakpoints have certain disabilities, and either way, we basically have to tell users to trust us and not trust the VSCode GUI. As long as that's true, I'm not sure it makes sense to make the distinction pervasive: in my view, only the service that handles reconciliation really needs to know which is which.

But it also suggests a further enhancement: a data breakpoints tree contributed to the debug view. There, we could show what we know to be the full set of current data breakpoints so that users have an authoritative record in the debug view as well as in our views. We still have the problem that if users use the VSCode builtin commands from the variable view, then their GUI will know about the breakpoint but not any others, but we also have our record right next to theirs.

[jreineckearm]

First of all, thanks a lot @haydar-metin for this PR! This is awesome work and I like the out-of-the-box thinking to overcome the data breakpoint gap in the VS Code API.

This is going to be a powerful feature once we get a handle on the disconnect to the VS Code breakpoint view.
I agree that the internal vs external terminology could be a bit misleading. But probably a minor issue.

What's worrying me more in the meanwhile is the disconnect between custom data breakpoints and VS Code.
This can make things tricky when you work with data breakpoints that require HW debug logic with limited resource. Which is why we'd need a central view showing all data breakpoints of a session.

However, I'd advise against creating a new view for that. We need to ensure this integrates with the existing centralized breakpoint view. Otherwise, this gets difficult to manage for users.

I've played around with a couple of things, but neither found a good way yet without fixing this in VS Code:

• No good way to update the DTOs/breakpoint view. Made worse by the fact that I can't see a way to list all installed breakpoints via MS-DAP. Which would allow refreshing the view in VS Code based on what's really going on the debugger.
• No way of injecting custom breakpoint events from outside the debugger to update the breakpoint list. Or I missed it on the session API.

I'll keep on thinking about other ways to achieve that sync. But we may also want to consider creating a PR on VS Code. If I understand correctly, an issue exists already there.

[colin-grant-work]

No good way to update the DTOs/breakpoint view. Made worse by the fact that I can't see a way to list all installed breakpoints via MS-DAP. Which would allow refreshing the view in VS Code based on what's really going on the debugger.

I think that this might be the easiest way to attack the problem from a VSCode standpoint. I think that data breakpoints have languished a bit because they've backed themselves into a corner: other breakpoints can be added without reference to a session and then sent to any session, but the way data breakpoints have been implemented, they're at least partly bound to a session. That means that they can't really treat data breakpoints the same in the plugin API - I once started down the naive path and then realized that expectations differ quite a bit - and that means that they'd have to implement entirely new API for data breakpoints. It wouldn't really be that bad - either an addDataBreakpoint method on a DebugSession or a variant of vscode.debugaddBreakpoinst that accepts a session would work, for example - but since it isn't entirely parallel to the other two breakpoint kinds, it's likely to require a fair bit of discussion and time. On the other hand, some way for the protocol to fetch breakpoints in addition to setting breakpoints might be more straightforward, or an extension of the breakpoint event to allow it to handle more cases, might face less resistance.

[jreineckearm]

Thanks a lot, @colin-grant-work , for sharing your findings from previous investigations! This is really useful!
Particularly the fact that there currently is no session-awareness for other breakpoint types is an interesting one....

[planger]

We certainly recognize the issue with not being able to display data breakpoints consistently to the user due to the current limitations in the VS Code API. For UX reasons, we would prefer not to introduce a separate view just to manage data breakpoints.

Therefore, we've opened a PR in VS Code that extends the vscode.debug API to allow users to manage data breakpoints and display them in the standard breakpoint view. This should resolve the inconsistency issue once it’s merged.

Given that this PR is in progress and hopefully will be merged soon, can we proceed with merging this PR, or are there any objections or additional considerations we should address beforehand? @colin-grant-work @jreineckearm

Thank you!

[colin-grant-work]

Given that this PR is in progress and hopefully will be merged soon, can we proceed with merging this PR.

If the idea is that this PR should use the new API when available, it would seem odd to me to proceed with it before that API is available.

[planger]

@colin-grant-work Right, the idea was that we already include the data breakpoint feature already now, as it already adds value, and solve the remaining UX deficiency by adding data breakpoints in the VS Code breakpoint view, once the new VS Code API is available.

[planger]

Oh, as I just realized (thanks for the hint @haydar-metin), we need to revert ad8062a to enable the functionality before merging (before or after the VS Code API is available).

[colin-grant-work]

@colin-grant-work Right, the idea was that we already include the data breakpoint feature already now, as it already adds value, and solve the remaining UX deficiency by adding data breakpoints in the VS Code breakpoint view, once the new VS Code API is available.

@planger, yes I understand, but if we were holding up this PR because of the deficiencies in the current API, then I don't understand why we would proceed before those deficiencies have been remedied. Nothing has changed yet from the time that we decided not to proceed with this approach because of its UX problems imposed by the VSCode API.

[planger]

I understand your concern! I was just thinking, this PR already adds value as is and the UX deficiency may not be that big of an issue, if you are aware that data breakpoints don't show up in the VS Code breakpoint list. But you are right, we need to weigh the value it adds against the confusion it may entail.

[colin-grant-work]

I'm open to an argument that we should proceed, and perhaps should have proceeded, but the fact that the API may be improved in the future doesn't directly bear on that question: we would need to decide whether we believe that the value it provides now outweighs the concerns about UX that led to our pausing in the first place.