Discussion: Remove Memory Browser

[colin-grant-work]

The VSCode Memory Inspector is a sister project in CDT Cloud that derives ultimately from an enhancement of the Memory Browser in this repository. Would it make sense to remove the Memory Browser code from this repository and direct users to the Memory Inspector in some way?

@thegecko as a Memory Inspector contributor. @asimgunes as a consumer of the exports here.

[jonahgraham]

I think Renesas needs it still due to some handling of multiple contexts in multi-debug scenarios. But we should try to get some of that functionality into memory inspector so we can drop this duplicate functionality.

[thegecko]

I support this proposal and look forward to more features in memory inspector :)

[jonahgraham]

At the CDT Cloud call today I was asked to provide additional details. cc: @asimgunes

I don't recommend that we invest further in the integrated memory browser and invest future work only on the memory inspector. What prevents removing this in the short term is the bit of code that communicates with the amalgamator to send an additional field to the memory read request that passes which child to use:

The UI only appears if connected to amalgamator:

cdt-gdb-vscode/src/memory/client/MemoryBrowser.tsx

Line 99 in 36449f8

async getAvailableChildren() {

But IMHO the correct solution is to have the context (selected frame in the call stack) available to other views in the extension. As mentioned on the call today there is some progress on that from VSCode community (references needed)

[colin-grant-work]

@jonahgraham, thanks for the details. Can you (or others) elaborate a little on how the 'children' are presented to the client? Are they treated as separate sessions or just separate 'threads'?

[jonahgraham]

The child dropdown in the above screenshot is the same items as the top level items in the Call Stack view. For the call stack view it is populated by the standard call stack info in the DAP as separate 'threads', but in the dropdown it uses the custom command cdt-amalgamator/getChildDapNames

[thegecko]

cc @jreineckearm

[colin-grant-work]

@jonahgraham, it looks like this plugin has handling for cases where debugType == amalgamator, but it doesn't actually contribute the debug type amalgamator. Is that handled by other plugins, or who exposes that debug type to the IDE? I see the contribution in the cdt-amalgamator package.json, but I wouldn't expect that to get read as a plugin manifest by VSCode.

[jonahgraham]

I wouldn't expect that to get read as a plugin manifest by VSCode.

It may very well not work. That code was supposed to be "us" (Renesas) upstreaming our internal changes, but we still use the internal version so that code in this repo may not actually work.

[colin-grant-work]

I think I just misunderstood the mechanism: it looks like the code in this repo depends on the equivalent of an NPM package version of the Amalgamator code, but the Amalgamator is also a plugin, so it does get read as a plugin (if you also install it). Thanks!

[WyoTwT]

Hi all. I've been working on some changes to the VSCode Memory Inspector to replace the Memory Browser in VSCode for the Renesas application.

One thing I noticed is that the data return format from the cdt-gdb-adapter's memoryRequest() in GDBDebugSession.ts is only slightly different from the cdt-gdb-adapter's readMemoryRequest() in GDBDebugSession.ts . Although the input arguments are quite compatible, the biggest difference is in the return values (memoryRequest() returns hex while readMemoryRequest() returns base64).

The cdt-amalgamator is currently using the memoryRequest() for the VSCode's MemoryBrowser [link] while Memory Inspector is currently using readMemoryRequest() [link].

I can either

1. Add the readMemoryRequest() as a new custom request to cdt-amalgamator for consistency with the existing Memory Inspector calls or
2. Use the existing memoryRequest() and convert the return data to base64 in the Memory Inspector's amalgamator abstraction layer (AdapterCapabilities for Memory Inspector) before returning to the Memory Inspector's memory view. This is a little messier but can be done.

Option1 would be my preference. @colin-grant-work asked me to run it past @asimgunes and @jonahgraham since they're familiar with the Renesas requirements (but I also welcome comments from the rest of this list).

I've implemented Option1 in the recent amalgamator PR.

[thegecko]

A step towards adding this to the memory inspector in a generic way: eclipse-cdt-cloud/vscode-memory-inspector#152

[colin-grant-work]

A step towards adding this to the memory inspector in a generic way: eclipse-cdt-cloud/vscode-memory-inspector#152

@thegecko, I think your PR is good and useful, but I don't think it actually addresses either this requirement or the goals of Thor's PR directly, as the point in both cases is to offer sub-session selection. In the case of the amalgamator, there are two GDB sessions, but they're presented to VSCode as a single session with multiple 'threads', so adding a selector at the session level doesn't actually reach the level desired, and similarly, in Thor's PR, the 'context' is the thread level (matching the amalgamator's desired selection level, but with other uses in our context downstream, as well), also below VSCode's notion of session.

[thegecko]

@thegecko, I think eclipse-cdt-cloud/vscode-memory-inspector#152 is good and useful, but I don't think it actually addresses either this requirement or the goals of eclipse-cdt-cloud/vscode-memory-inspector#133 directly, as the point in both cases is to offer sub-session selection. In the case of the amalgamator, there are two GDB sessions, but they're presented to VSCode as a single session with multiple 'threads', so adding a selector at the session level doesn't actually reach the level desired, and similarly, in Thor's PR, the 'context' is the thread level (matching the amalgamator's desired selection level, but with other uses in our context downstream, as well), also below VSCode's notion of session.

Right, but these solutions only use threads as a way to hook into the VS Code UI effectively. My proposal is to extend the work to make this pluggable in order to display and select any context as it makes sense for individual debuggers.

[jonahgraham]

When this is made pluggable there needs to be a way of the extender intercepting messages to modify them. For the amalgamator case it means adding an addition field child - but it could require some other types of changes to the request to pass the selection in the drop-down to the target adapter.

Note that because child is used today by the amalgamator doesn't mean that is the best solution. @trongle0504 may have more thoughts on this, but it could be adding the threadId (or frameId) to the read memory request is better and more generic. Using frameid would make ReadMemory (and related) more similar to Evaluate Request so that the correct context can be passed from the UI to the adapter.