Add Debug Context Selector

[WyoTwT]

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

• 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]

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

[jreineckearm]

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]

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]

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]

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]

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]

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?

[colin-grant-work]

It sounds like we likely want to move this over to the Amalgamator repository and make it dependent on this plugin (or at least propose it).

[jreineckearm]

I agree with @colin-grant-work . I think this is a great example of how a custom adapter implementation could look like.
But ideally it would be contributed by the debug adapter. Or was the intention to use it in a downstream version of the Memory Inspector, @WyoTwT ?

Maybe we can build some "how to implement your own adapter contribution" docs around it in https://github.com/eclipse-cdt-cloud/vscode-memory-inspector?tab=readme-ov-file#the-memory-provider ? It could be a nice story to demonstrate this with two Eclipse CDT Cloud components.

Another thing this PR shows me is that we may want to consider to somehow provide access to the default adapter implementation for reuse in custom implementations. Beyond asking developers to include the according sources from this repo.

[WyoTwT]

It doesn't appear there is an easy way to get access to the AdapterCapabilities, AdapterVariableTracker, and VariableTracker if this code is moved to the Amalgamator repository.

I could duplicate the interfaces in Amalgamator which is what I did with the Context because that wasn't shared from the Amalgamator. Would restructuring the code differently facilitate future movement of some parts into a form that could be reused for custom implementations?

I'm rather new to this so all suggestions to better structure the code are welcome but I may need additional clarification.

[jreineckearm]

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.

[WyoTwT]

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]

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

[thegecko]

@WyoTwT In my opinion, this extension should be completely agnostic from any underlying debugger and not have any knowledge of the debugger its driving (apart from capabilities we have defined, e.g. the memory read and write functionality).

To that end, I wouldn't expect any mention of adapters (e.g. amalgamater) and in order to add support, any functionality should be added to the adapter itself where possible.

If there are certain areas we need additional functionality (e.g. dealing with multiple debug sessions in a compound or amalgamation), these should be implemented in a generic way so any other adapters can also take advantage of this support. This could be exposed as some sort of extension contribution point, optional API or similar.