Add Debug Context Selector

WyoTwT commented 4 months ago

What it does

This PR fixes #67. In a system with multiple children such as the cdt-gdb-amalgamator, the Memory Inspector needs a way to select the correct context (as the cdt-gdb-vscode Memory Browser does).

This PR has a dependency on cdt-gdb-amalgamator PR 20 which puts the assignment of the Child IDs in the cdt-gdb-amalgamator instead of the Memory Inspector and also uses the readMemory since the data returned is in the correct format for Memory Inspector.

How to test

Run this branch of the vscode-memory-inspector with the mainline cdt-gdb-vscode and cdt-gdb-adapter along with the modified cdt-gdb-amalgamator in PR 20.

Start the debugger as a Desktop Extension. In the newly launched VSCode, open the cdt-amalgamator/sampleWorkspace folder and place a breakpoint at the first instruction in empty1.c's main and empty2.c's main. Start debugging with Amalgamator Example and you should run to the breakpoints. Open a new Memory Inspector with the Command Palette's Memory: Show Memory Inspector to open a new view. The dropdown should list both proc1 and proc2 listed in the Call Stack.

Select the appropriate proc by clicking on the corresponding main in the call stack. View the variable value in memory by typing: &local_in_main into the Address textfield and pressing the Go button. Go back to the corresponding C file (empty1.c or empty2.c) and single step. The value of the Local variable in the Variables view will update. Open the Memory tab and the Data should have changed as well to match the Variables view.

Review checklist

[x] 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

WyoTwT commented 4 months ago

Because these changes impact their work, notifying @asimgunes and @jonahgraham

jreineckearm commented 4 months ago

Thanks for the contribution, @WyoTwT !

I appreciate where this is going. And that this is something we need for allowing contexts on windows like this, e.g. in multi-core debug scenarios.

For the obvious reason that the amalgamator allows the use of context, it is very much tailored for that debug adapter. I am just curious if there have been any efforts yet to drive such extensions into the Microsoft DAP protocol? (Apologies, I don't know all the history of the amalgamator and its development yet).

WyoTwT commented 4 months ago

That is a good question, @jreineckearm . I've been operating under the assumption that the Microsoft DAP protocol is fairly rigid and hard to change. You make a great point. For instance, including an optional context for a read/write operation makes sense from a multi-core or multi-thread perspective so maybe this should be a change to the DAP protocol.

Our downstream implementation differs from the Amalgamator but leverages this same PR with slight changes. Our implementation is a multi-core, single DAP instance which defines different threads for each core. In this case, the context is the thread. Context made sense for both our implementation (multi-thread, single DAP) and the Amalgamator case (multiple, single-threaded DAPs).

Unfortunately, I don't know the history of the Amalgamator either but my understanding from eclipse-cdt-cloud/cdt-gdb-vscode/#110 was that the issue preventing us from using Memory Inspector instead of the cdt-gdb-vscode Memory Browser was the Amalgamator implementation so this PR was tailored toward supporting the Amalgamator.

Thank you for your insight!

colin-grant-work commented 4 months ago

In principle, I think that this should be something the DAP would be open to, since it would be an optional extension to an existing request with clear parallels in other requests, but it doesn't look like an issue that has been explicitly raised for ReadMemory or WriteMemory requests.

jreineckearm commented 4 months ago

I'd also suspect that this is something worth to follow up on. Especially considering that looking into multi-core debug in general is on the New Initiatives list in CDT Cloud: https://github.com/eclipse-cdt-cloud/cdt-cloud/wiki/CDT-Cloud-Meetings

I am out of office for the rest of the week. But will get back to this PR here after if no one else did until then. :-)

WyoTwT commented 3 months ago

Rebased on top of current Memory Inspector changes.

Removed the Amalgamator additions from desktop/extensions.js so for testing the attached patch should be applied. patch.txt

colin-grant-work commented 3 months ago

As we've discussed a bit downstream, the word 'context' is now fairly polyvalent in this repo. The three kinds I see from a casual search are:

SessionContext: knowledge about the session (e.g. capabilities) that we pass to the frontend.
Webview*Context: metadata relevant to context menu behavior in the webview.
Context (introduced in this PR): data stored on the frontend relevant to its interactions with its debug session.

Each is distinct, so I think it makes sense to keep them separate. In particular, the SessionContext is controlled by the plugin side and not user-visible, while the Context values can be set by users to change some parameter of the interaction with the session.

On the other hand, labeling them all as *Context is inviting confusion, so maybe we can do some renaming to try to clarify the role of each.

I'd be in favor of renaming SessionContext to something like SessionDetails - it's almost equally vague, but I think it communicates a little better the idea that we're just dealing with metadata. The Context added in this PR could maybe be changed to something with Arguments in it, since its intended to supplement the arguments to various requests in some adapter-specific way. At present, it's effectively ThreadArguments or StackArguments, though there's also potential to view them as RegionArguments or AddressSpaceArguments, but I'm not sure we want to commit it to such a particular interpretation. @jreineckearm, do you see other likely uses for supplemental arguments?

jreineckearm commented 3 months ago

As we've discussed a bit downstream, the word 'context' is now fairly polyvalent in this repo. The three kinds I see from a casual search are:
* `SessionContext`: knowledge about the session (e.g. capabilities) that we pass to the frontend.

* `Webview*Context`: metadata relevant to context menu behavior in the webview.

* `Context` (introduced in this PR): data stored on the frontend relevant to its interactions with its debug session.
Each is distinct, so I think it makes sense to keep them separate. In particular, the SessionContext is controlled by the plugin side and not user-visible, while the Context values can be set by users to change some parameter of the interaction with the session.

On the other hand, labeling them all as *Context is inviting confusion, so maybe we can do some renaming to try to clarify the role of each.

I'd be in favor of renaming SessionContext to something like SessionDetails - it's almost equally vague, but I think it communicates a little better the idea that we're just dealing with metadata. The Context added in this PR could maybe be changed to something with Arguments in it, since its intended to supplement the arguments to various requests in some adapter-specific way.

I think ultimately the added context is a ConnectionContext. AFAIK, the amalgamator exposes and manages multiple connections for a debug session. So, we would have a session context (which holds details but also methods for example to send custom requests) that can have multiple child (connection) contexts. TBF, I think the term context is right here. But I see your point about potential confusion in the code. And we may want to be stricter on variable names clearly indicating which context we are looking at.

At present, it's effectively ThreadArguments or StackArguments, though there's also potential to view them as RegionArguments or AddressSpaceArguments, but I'm not sure we want to commit it to such a particular interpretation. @jreineckearm, do you see other likely uses for supplemental arguments?

In fact there are some arguments on my mind when it comes to this. Among them are apparently address regions, address spaces, but also physical access parameters interpreted by the involved debug logic or translated into bus signals issued to the target memory system. Some could be covered by qualifiers in memory references. But others not so much. See for example https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/issues/104 But I think this is an area that needs to be reviewed first on a Debug Adapter Protocol level.

WyoTwT commented 2 months ago

As we've discussed a bit downstream, the word 'context' is now fairly polyvalent in this repo. The three kinds I see from a casual search are:
* `SessionContext`: knowledge about the session (e.g. capabilities) that we pass to the frontend.

* `Webview*Context`: metadata relevant to context menu behavior in the webview.

* `Context` (introduced in this PR): data stored on the frontend relevant to its interactions with its debug session.
Each is distinct, so I think it makes sense to keep them separate. In particular, the SessionContext is controlled by the plugin side and not user-visible, while the Context values can be set by users to change some parameter of the interaction with the session. On the other hand, labeling them all as *Context is inviting confusion, so maybe we can do some renaming to try to clarify the role of each. I'd be in favor of renaming SessionContext to something like SessionDetails - it's almost equally vague, but I think it communicates a little better the idea that we're just dealing with metadata. The Context added in this PR could maybe be changed to something with Arguments in it, since its intended to supplement the arguments to various requests in some adapter-specific way.
I think ultimately the added context is a ConnectionContext. AFAIK, the amalgamator exposes and manages multiple connections for a debug session. So, we would have a session context (which holds details but also methods for example to send custom requests) that can have multiple child (connection) contexts. TBF, I think the term context is right here. But I see your point about potential confusion in the code. And we may want to be stricter on variable names clearly indicating which context we are looking at.

At present, it's effectively ThreadArguments or StackArguments, though there's also potential to view them as RegionArguments or AddressSpaceArguments, but I'm not sure we want to commit it to such a particular interpretation. @jreineckearm, do you see other likely uses for supplemental arguments?

In fact there are some arguments on my mind when it comes to this. Among them are apparently address regions, address spaces, but also physical access parameters interpreted by the involved debug logic or translated into bus signals issued to the target memory system. Some could be covered by qualifiers in memory references. But others not so much. See for example #104 But I think this is an area that needs to be reviewed first on a Debug Adapter Protocol level.

I see your point about the Amalgamator view using a connectionContext. However, in another implementation (such as our downstream implementation) that defined a thread for each CPU, naming it threadContext would be more intuitive than connectionContext.

I'm not against picking connectionContext as a name and going with it - I'm just pointing this out because naming is hard.

Either way, I like using *Arguments to better describe the additional arguments that are passed.

Whatever is decided, I'm happy to use.

WyoTwT commented 2 months ago

Rebased on latest vscode-memory-inspector/main and implemented the changes from the conversations marked as resolved above.

eclipse-cdt-cloud / vscode-memory-inspector