Variables and Variable Decorations #16

colin-grant-work · 2023-03-31T18:06:59Z

What it does

Fixes Variable Labeling & Highlighting #12 - Adds plugin-side infrastructure for contributing non-DAP functionality (oriented toward variable retrieval, for now) and implements variable retrieval for GDB debugging.
Closes Flexible highlighting / decoration system #13 - Adds webview-side infrastructure for contributing CSS-based decorations to particular ranges of memory, and implements an example for variables
Resolves Modularize by columns #14 - Adds webview-side infrastructure for contributing new column renderers and implements an example for variables.

It also:

Replaces Longs with BigInts (they can go in Sets)
Reorganizes the code more clearly into entry-points, plugin-side code, webview-side code, and common code that works in both plugin and webview contexts.

Open questions:

Rather than integrating the new webview-side services into the React lifecycle, I've made the module singletons. They could be managed differently.

How to test

Start a GDB debug session.
Open a memory view and view memory.
It should not display variables by default, and no request to retrieve variables should be made to the plugin side.
Open the advanced options section of the options widget and reveal the variables column.
The local variables available in the current frame should be retrieved, the regions they inhabit should be colored, and their names should appear in a third column:

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

colin-grant-work · 2023-03-31T18:08:12Z

@thegecko, I know you said you wanted small changes... but as I started down the road to variables, it just seemed better to implement the more general infrastructure along the way.

thegecko · 2023-04-03T10:23:39Z

src/entry-points/browser/extension.ts


-export const activate = async (context: vscode.ExtensionContext): Promise<void> => {
+export const activate = async (context: vscode.ExtensionContext): Promise<AdapterRegistry> => {


If we are returning an API, it could be good to publish the API to npm (or even on the GitHub npm registry).

We do this internally by having an api folder with the interface exposed (which is published as a separate package) and then have the main class implement this interface.

I think that's a good idea, since it will make it easier to inform people of changes. On the one hand, I think that allowing others to contribute handling for their own adapters is valuable, on the other hand managing the API when VSCode's dependency resolution is fairly crude could be tricky. I know that we would like to support at least memory watchpoints on the plugin side - is there anything that falls on the margins of the DAP that you'd like to support? Perhaps we can start releasing the API once we've got at least a first draft of the functionality we know we want?

Using our browser debugger and the gdb debugger as test adapters should really help us uncover the correct abstraction points and any common code we don't need to duplicate in plugins. Let's let this evolve as we add features and see where it goes :)

thegecko · 2023-04-03T10:29:33Z

src/plugin/adapter-registry/gdb-capabilities.ts

@@ -0,0 +1,137 @@
+/********************************************************************************


Our use case doesn't use gdb (and gdb doesn't work in the browser), so I'd be keen to see a basic adaptertracker implemented which does as much as possible, perhaps with an example gdb one extending the base one?

For example, scopes, variables and evaluation requests are all supported in DAP (we could check the capabilities request?) and even the local variables can be detected using the DAP presentation hints.

Sounds good - I think the only GDB specific bit in the current implementation is the actual evaluate request, in the sense that even though the evaluate request is part of the DAP, what expressions get sent and their syntax may depend on the language and adapter being used. Another tricky thing, though, is that the GDB adapter (currently) sends its memory references formatted as pointers rather than addresses, so without the evaluate request, it isn't possible to match the variables to memory at all. Other adapters may work around the specification a little less and provide addresses in that field, but we're shooting in the dark a bit.

src/plugin/memory-provider.ts

thegecko · 2023-04-03T10:33:12Z

Some questions for now... I'm going to look at getting the new features working with our adapter which may suggest a better abstraction point and/or default functionality. We may even be able to get away with a common tracker....

thegecko · 2023-04-03T10:41:33Z

src/entry-points/browser/extension.ts

-
-    await memoryProvider.activate(context);
+    const registry = new AdapterRegistry();
+    context.subscriptions.push(registry);


Prefer the architecture of having an activate function in the class isolating functionality together.

I prefer not to have a two-step lifecycle of construction and then activation if it can be avoided, so if the class is going to handle its own lifecycle, I'd rather it do it in the constructor. What do you think of that approach?

My rationale for isolation is it keeps the activation needs along with the module being added. It makes it trivial to add/remove/change these modules and keeps the extension activation function from growing too large (a common problem I've seen in many extensions).

A couple of reasons why the two-step approach works better:

Constructors aren't asynchronous, but often activation is :(. This can be worked around using static class factories, but makes it a little messy

Instantiation is different to activation, we need to consider scenarios where classes may need to be deactivated, too (exposing a deactivate function, called in the global deactivate).

I'm not too precious about this, so happy to try adding the context to all constructors if you'd prefer?

Sounds fair. Consistency has some value, too, so I'll switch the registry to use an activate method, as well.

thegecko · 2023-04-03T11:43:54Z

I've been unable to set up an environment which triggers the variables functionality (even with gdb), could I have some hints?

Starting debug and opening the inspector shows nothing, there isn't an expression
Should I enter an expression which somehow signifies the current context?
Viewing a variable shows it in the inspector (as before), but variables view is empty
Should I be using some sort of complex variables?

colin-grant-work · 2023-04-03T14:24:45Z

I've been unable to set up an environment which triggers the variables functionality (even with gdb), could I have some hints?

Hm, my setup wasn't complex: I used the program from the example workspace from the old Theia CPP extension, built and ran as described there, and the variables worked as expected. I did add one complex variable to that program when I was checking the sequence of scope request -> variable request -> variable request (when resolving fields), but just viewing the ints also worked. I'll add some logging to the output channel during variable resolution to see if that helps track down the problem.

- Add logging to variable retrieval - Refactor into abstract variable tracker class

colin-grant-work · 2023-04-03T21:17:49Z

@thegecko, I've pushed up some changes that (may) address (some) of your comments:

I've added a fair bit of logging to the variable retrieval process, so setting the log verbosity to debug should give you a better idea of what (if anything) is happening when you reveal the variables column.
I've refactored the variable tracker so that only the variablesToVariableRange logic is specific to GDB.
I'm not quite sure what you mean by using the presentationHints field, in general, but I've modified the check for name === Local to include presentationHint === 'locals', since that's a possible bit of metadata for scopes.

thegecko · 2023-04-04T13:53:04Z

I've added a fair bit of logging to the variable retrieval process, so setting the log verbosity to debug should give you a better idea of what (if anything) is happening when you reveal the variables column.

Thanks, I'll give it a try.

I'm not quite sure what you mean by using the presentationHints field, in general, but I've modified the check for name === Local to include presentationHint === 'locals', since that's a possible bit of metadata for scopes.

That's exactly what I meant!

thegecko · 2023-04-05T11:49:18Z

OK, I have this working with gdb (there are a few hard-coded adapter type checks still in the code).

I'd like to get this working with out adapter too...

colin-grant-work · 2023-04-07T20:55:53Z

@thegecko,

there are a few hard-coded adapter type checks still in the code

Are there particular checks you'd like me to fix, or are you going to see what it takes to get it working with your adapter so that those changes can be included here?

thegecko · 2023-04-13T16:12:15Z

Back on this today.

Is there any reason we restrict the variables view to locals only? Variables in the global scope work for us in the existing theia-based memory inspector, but are ignored with this.

e.g all non-register scopes are shown:

https://github.com/eclipse-theia/theia-cpp-extensions/blob/86c7a683a4810e740575a09708ad1cf7076366f1/packages/cpp-debug/src/browser/utils/memory-widget-variable-utils.ts#L59

thegecko · 2023-04-13T16:14:05Z

src/plugin/adapter-registry/gdb-capabilities.ts

+import { logger } from '../logger';
+
+class GdbAdapterTracker extends AdapterVariableTracker {
+    protected override async variableToVariableRange(variable: DebugProtocol.Variable, session: vscode.DebugSession): Promise<VariableRange | undefined> {


I think this approach should be the default that other implementations can optionally override. Using &(ref) and sizeof(ref) seem quite common in adapters.

They're fairly C-specific, I'd say. I know that e.g. the LLDB Rust adapter can't use them. Maybe rather than tying it to GDB, it can be extracted as a function with the logger as an additional argument, so the logic can be used by other implementations, but not built into the default?

That could be a good approach

thegecko · 2023-04-13T16:14:54Z

src/plugin/adapter-registry/gdb-capabilities.ts

+
+class GdbAdapterTracker extends AdapterVariableTracker {
+    protected override async variableToVariableRange(variable: DebugProtocol.Variable, session: vscode.DebugSession): Promise<VariableRange | undefined> {
+        if (!variable.memoryReference || this.currentFrame === undefined) {


Should this check be for variable.name as that is what is used to lookup the data?

I'm not sure. The existence of memoryReference is what tells us that the adapter believes it's something it can look up in memory. I don't believe that it would be legal, either, to send a variable with no name, but we can add it as an additional check to make sure.

Checking the previous implementation (specifically reading global variables) it's possible to have a named variable without a memoryReference. In my tests on our adapter, all global variables have a name used for the expression, but no memoryReference (and it works in the old inspector)

We can implement that, but it's non-DAP-adherent behavior on the part of that adapter. The old implementation predated the addition of memoryReference to the interface for variables - it predated the readMemory command entirely; now that that's available, it's supposed to be the most reliable indicator that the adapter can handle memory-related requests for that variable.

Erk, its the only way this works with our adapter.

I'll need to investigate populating the memoryReference when we only have a &ref

That's all the GDB adapter has, right? It just sends &name as its memory references. Which I think isn't exactly what the DAP intends, but it does at least indicate how the adapter wants the client to refer to the variable's memory location. In any case, I've pushed a change that checks only the name for those eval requests.

colin-grant-work · 2023-04-13T16:32:46Z

Is there any reason we restrict the variables view to locals only? Variables in the global scope work for us in the existing theia-based memory inspector, but are ignored with this.

Just faulty memory / reading, I think. For some reason I thought the old implementation was limited to Locals, but you're quite right it wasn't. I'll adjust this and make sure the tree is flexible enough to handle other scopes.

thegecko · 2023-04-17T10:44:38Z

I think we are blocked on getting this working in the browser debugger due to some shortcomings in it.

So in the interests of progress, let's merge this. It can always be refactored later.

Colin Grant added 7 commits March 24, 2023 15:34

Most of the infrastructure

c80caa3

Big reorganization

79f325d

BigInts are going to be better

83d0a6d

BigInts are better

3df454a

Memory decorations working

9d977b1

Working pretty well

8a0ec06

Rename old Long references

b587554

colin-grant-work requested a review from thegecko March 31, 2023 18:07

thegecko requested changes Apr 3, 2023

View reviewed changes

thegecko reviewed Apr 3, 2023

View reviewed changes

RM Comments

f5ee09f

- Add logging to variable retrieval - Refactor into abstract variable tracker class

Fix JSDoc type comments

0e05f6d

Use activate method

9bb81f7

thegecko reviewed Apr 13, 2023

View reviewed changes

Colin Grant added 5 commits April 13, 2023 11:43

Use all scopes other than registers

c0a3533

Add name check

ac340ee

Really add name check :-)

83dd93e

Use only name; ignore memoryReference

0d00dee

Typo

81b83fd

Go back to watch, since they're expressions

22e83a8

thegecko merged commit 09b7738 into eclipse-cdt-cloud:main Apr 17, 2023

jreineckearm mentioned this pull request Feb 17, 2024

Refine Variable Tracker to make use of more hints from DAP Variable type #68

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variables and Variable Decorations #16

Variables and Variable Decorations #16

colin-grant-work commented Mar 31, 2023

colin-grant-work commented Mar 31, 2023

thegecko Apr 3, 2023

colin-grant-work Apr 3, 2023

thegecko Apr 4, 2023

thegecko Apr 3, 2023

colin-grant-work Apr 3, 2023

thegecko commented Apr 3, 2023

thegecko Apr 3, 2023

colin-grant-work Apr 3, 2023

thegecko Apr 4, 2023

colin-grant-work Apr 4, 2023

thegecko commented Apr 3, 2023

colin-grant-work commented Apr 3, 2023

colin-grant-work commented Apr 3, 2023

thegecko commented Apr 4, 2023

thegecko commented Apr 5, 2023

colin-grant-work commented Apr 7, 2023

thegecko commented Apr 13, 2023

thegecko Apr 13, 2023

colin-grant-work Apr 13, 2023

thegecko Apr 14, 2023

thegecko Apr 13, 2023

colin-grant-work Apr 13, 2023

thegecko Apr 14, 2023

colin-grant-work Apr 14, 2023 •

edited

Loading

thegecko Apr 14, 2023

colin-grant-work Apr 14, 2023

colin-grant-work commented Apr 13, 2023

thegecko commented Apr 17, 2023


		export const activate = async (context: vscode.ExtensionContext): Promise<void> => {
		export const activate = async (context: vscode.ExtensionContext): Promise<AdapterRegistry> => {

		@@ -0,0 +1,137 @@
		/********************************************************************************

Variables and Variable Decorations #16

Variables and Variable Decorations #16

Conversation

colin-grant-work commented Mar 31, 2023

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work commented Mar 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thegecko commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thegecko commented Apr 3, 2023

colin-grant-work commented Apr 3, 2023

colin-grant-work commented Apr 3, 2023

thegecko commented Apr 4, 2023

thegecko commented Apr 5, 2023

colin-grant-work commented Apr 7, 2023

thegecko commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-grant-work Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-grant-work commented Apr 13, 2023

thegecko commented Apr 17, 2023

colin-grant-work Apr 14, 2023 •

edited

Loading