Allow to set data breakpoints #139

haydar-metin · 2024-06-06T08:30:47Z

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

Start the debugger
Open the memory inspector
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 · 2024-06-06T17:50:46Z

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 · 2024-06-06T14:30:57Z

src/plugin/breakpoints/breakpoint-tracker.ts

+        }
+
+        if (isSessionEvent('stopped', event)) {
+            // TODO: Only for demo purposes


When should this be removed?

colin-grant-work · 2024-06-06T17:52:33Z

src/webview/columns/data-column.tsx

        return <span
-            className='byte-group hoverable'
+            className={classNames('byte-group', 'hoverable', ...BreakpointService.inlineClasses(breakpointMetadata))}


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

colin-grant-work · 2024-06-06T17:53:21Z

src/webview/columns/data-column.tsx

            data-column='data'
            data-range={`${startAddress}-${endAddress}`}
            key={startAddress.toString(16)}
            onDoubleClick={this.setGroupEdit}
-            {...createGroupVscodeContext(startAddress, toOffset(startAddress, endAddress, this.props.options.bytesPerMau * 8))}
+            {...createGroupVscodeContext(startAddress, toOffset(startAddress, endAddress, this.props.options.bytesPerMau * 8), breakpointMetadata)}


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

colin-grant-work · 2024-06-06T17:55:11Z

src/webview/memory-webview-view.tsx

+                new Set(columnContributionService
+                    .getUpdateExecutors()
+                    .concat(decorationService.getUpdateExecutors())
+                    .concat(breakpointService)),


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

colin-grant-work · 2024-06-06T17:58:35Z

src/common/breakpoint.ts

+export interface TrackedDataBreakpoint {
+    type: TrackedBreakpointType;
+    breakpoint: DebugProtocol.DataBreakpoint;
+    /**
+     * The respective response for the breakpoint.
+     */
+    response: DebugProtocol.SetDataBreakpointsResponse['body']['breakpoints'][0]
+}
+
+export interface TrackedDataBreakpoints {


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.

colin-grant-work · 2024-06-06T17:59:38Z

src/common/breakpoint.ts

+    /**
+     * The respective response for the breakpoint.
+     */
+    response: DebugProtocol.SetDataBreakpointsResponse['body']['breakpoints'][0]


I believe that this parses to DebugProtocol.Breakpoint.

haydar-metin · 2024-06-07T11:35:46Z

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 · 2024-06-07T16:53:11Z

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 · 2024-06-18T09:26:27Z

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 · 2024-06-18T16:52:53Z

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 · 2024-06-18T19:20:02Z

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

haydar-metin requested review from jreineckearm and colin-grant-work June 6, 2024 08:31

haydar-metin self-assigned this Jun 6, 2024

Allow to create data breakpoints

fe7ae6d

haydar-metin force-pushed the issues/102_data_breakpoint branch from 973a7d7 to fe7ae6d Compare June 6, 2024 08:33

colin-grant-work reviewed Jun 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to set data breakpoints #139

Allow to set data breakpoints #139

haydar-metin commented Jun 6, 2024 •

edited

Loading

colin-grant-work commented Jun 6, 2024

colin-grant-work Jun 6, 2024

colin-grant-work Jun 6, 2024

colin-grant-work Jun 6, 2024

colin-grant-work Jun 6, 2024

colin-grant-work Jun 6, 2024

colin-grant-work Jun 6, 2024

haydar-metin commented Jun 7, 2024

colin-grant-work commented Jun 7, 2024 •

edited

Loading

jreineckearm commented Jun 18, 2024

colin-grant-work commented Jun 18, 2024

jreineckearm commented Jun 18, 2024

Allow to set data breakpoints #139

Are you sure you want to change the base?

Allow to set data breakpoints #139

Conversation

haydar-metin commented Jun 6, 2024 • edited Loading

What it does

Extension API

Debugger / Logs

How to test

Review checklist

Reminder for reviewers

colin-grant-work commented Jun 6, 2024

colin-grant-work Jun 6, 2024

Choose a reason for hiding this comment

colin-grant-work Jun 6, 2024

Choose a reason for hiding this comment

colin-grant-work Jun 6, 2024

Choose a reason for hiding this comment

colin-grant-work Jun 6, 2024

Choose a reason for hiding this comment

colin-grant-work Jun 6, 2024

Choose a reason for hiding this comment

colin-grant-work Jun 6, 2024

Choose a reason for hiding this comment

haydar-metin commented Jun 7, 2024

colin-grant-work commented Jun 7, 2024 • edited Loading

jreineckearm commented Jun 18, 2024

colin-grant-work commented Jun 18, 2024

jreineckearm commented Jun 18, 2024

haydar-metin commented Jun 6, 2024 •

edited

Loading

colin-grant-work commented Jun 7, 2024 •

edited

Loading