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:

https://github.com/eclipse-cdt-cloud/cdt-gdb-vscode/blob/36449f8ba2ce2a2fe47fa5b57adb3e25c3940269/src/memory/client/MemoryBrowser.tsx#L99

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.