eclipse-cdt-cloud / vscode-memory-inspector

vscode memory inspector
https://open-vsx.org/extension/eclipse-cdt/memory-inspector
Eclipse Public License 2.0
6 stars 10 forks source link

Allow to set data breakpoints #139

Open haydar-metin opened 3 months ago

haydar-metin commented 3 months ago

What it does

Allows to set data breakpoints within the memory table.

https://github.com/eclipsesource/vscode-memory-inspector/assets/13104167/44131d4e-a5e8-4d9b-bf98-a3ee7cc5c5d9

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).

https://github.com/microsoft/vscode/issues/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.

image

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

Reminder for reviewers

colin-grant-work commented 3 months ago

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?

haydar-metin commented 3 months ago

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 commented 3 months ago

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:

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 commented 2 months ago

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:

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 commented 2 months ago

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 commented 2 months ago

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....