Follow pointers #122

gbodeen · 2024-04-04T20:29:27Z

What it does

Closes #123.

Two closely related capabilities. In the example images, int *ii = &i .

Offer the "Show in Memory Inspector" on child elements of the tree in the debug Variables view. A common kind of child element is the reference of a pointer. Other kinds include array elements and struct members.
Provide a "Follow Pointer" context menu on pointer variables in the memory inspector Variables column.

Although (1) works in all conditions I've tested so far (using C++), arguably it should be handled upstream by VSCode or gdb or other. VSCode's canViewMemory context is set according to whether a tree item has a memoryReference property already provided; but this property is not set on child elements of the tree even when it's readily available. However, the memory inspector already contained code to handle cases with a missing memoryReference, so in my current view, the (1) feature simply makes this more thorough.

For (2), to use webview context in a command's when clause, I added flattened versions of nested keys as required by VSCode. It seemed like a good choice in case other webview context is to be used similarly in the future. However, an alternative could be simply adding a top-level key, e.g. memoryInspectorVariableIsPointer instead of memory-inspector.variable.isPointer.

How to test

Right-click on child elements in the debug Variables view & choose "Show in Memory Inspector".
The "Follow Pointer" context menu item should show only on pointer variables in the memory inspector Variables column, and should reveal an inspector at the pointer's reference.

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

colin-grant-work · 2024-04-04T20:39:25Z

src/plugin/adapter-registry/c-tracker.ts

@@ -77,4 +78,8 @@ export class CTracker extends AdapterVariableTracker {
        const response = await sendRequest(session, 'evaluate', { expression: CEvaluateExpression.sizeOf(variableName), context: 'watch', frameId: this.currentFrame });
        return notADigit.test(response.result) ? undefined : BigInt(response.result);
    }
+
+    protected isPointer(variable: DebugProtocol.Variable): boolean {
+        return (!variable.type || variable.type.endsWith('*')) && (`0x${Number(variable.value).toString(16)}` === variable.value);


I think the * check will likely suffice, but if we want to include the check for numerical value, let's use one that accommodates at least bases 10 and 16, which is the informal convention we've arrived at for formats we generally want to support.

I just tried this with one of our debug adapters. The check for a numerical value is a bit of a problem there.

Background is that we show the pointer type as part of the value in the Variables display. For example

type alone would however work, see the info in the variable decorator hover-over:

What is the motivation behind verifying that a pointer is displayed as a hex value?

Another question is why not providing a type would resolve to isPointer(...) === true?

OK, I see now why it checks for a numerical value. It's passed to the address field of the new Memory Inspector instance as is.

On the other hand, readMemory request doesn't mandate that a memoryReference (and hence the address field value) must be a numerical value. It could as well be a symbol or an expression. In fact something we would like to utilize in future with out debug adapters to allow for example arithmetic expressions beyond the given offset field.

Yes, in principle it could just be the variable itself. If we believe that x is a pointer to y, then x should be the same &y, which is what CDT GDB, at least, uses as the memoryReference for y.

package.json

src/plugin/memory-webview-main.ts

jreineckearm · 2024-04-08T16:14:34Z

src/webview/utils/vscode-contexts.ts

+    if (!src || typeof src !== 'object') { return dst; }
+    for (const [key, value] of Object.entries(src)) {
+        if (value && typeof value === 'object' && !Array.isArray(value)) {
+            flattenKeys(value, dst, `${prefix}${key}.`);


Might be a matter of taste. But I prefer the way you implemented it here (with the dots) over a top-level key as you proposed as an alternative in the description. This makes the key easier to read and helps to understand the hierarchy of the key.

jreineckearm · 2024-04-08T16:23:24Z

I like adding that feature back. I am just curious if it would make sense to expand this to following a value in general? That could spare some heuristics around "what is a pointer". And would allows showing memory at an address that is for example temporarily stored in a non-pointer variable until its casted into a pointer type just before using it as such.

I appreciate though that this requires a user to double-check from time to time if the value is a valid address in the systems memory map. Maybe the follow a value could come separately later.

This reverts commit eac8f67.

gbodeen · 2024-04-11T19:15:02Z

I like adding that feature back. I am just curious if it would make sense to expand this to following a value in general? That could spare some heuristics around "what is a pointer". And would allows showing memory at an address that is for example temporarily stored in a non-pointer variable until its casted into a pointer type just before using it as such.

I appreciate though that this requires a user to double-check from time to time if the value is a valid address in the systems memory map. Maybe the follow a value could come separately later.

A second draft is now up. It generalizes from following a pointer to following a value, as you mentioned. The command is available as a context menu entry for variables in the Variables column of the memory inspector.

The CTracker is responsible for identifying what values can be followed. It assumes that all variables with a type ending in "*" can be followed. For other variables, it tests that the value would point to a valid address. (The memory map is acquired when the CTracker first calls "getLocals".)

Other trackers for other languages may need to implement this differently.

src/plugin/adapter-registry/c-tracker.ts

colin-grant-work · 2024-04-29T18:26:42Z

@jreineckearm, I'm happy with the current state here - do you have any concerns about the approach?

src/common/debug-requests.ts

src/plugin/adapter-registry/c-tracker.ts

jreineckearm · 2024-04-30T09:07:47Z

package.json

@@ -85,6 +85,11 @@
        "title": "Show in Memory Inspector",
        "category": "Memory"
      },
+      {
+        "command": "memory-inspector.go-to-value",
+        "title": "Go to value in Memory Inspector",


Suggested change

"title": "Go to value in Memory Inspector",

"title": "Go to Value in Memory Inspector",

In fact I am wondering if Show Value in Memory Inspector would be more consistent with Show in Memory Inspector.

I was thinking "Go to value" would be clearer than "Show value", since the latter might be interpreted as toggling the data column. For the moment I'll just update the capitalization.

OK, understood. I am good with that.

jreineckearm

Apologies for the long period of silence.
I tried the latest state again. See some minor comments.

Overall, I believe we should go with this change. We may need to do some adjustments further down the road. I am having a few issues with testing all facets due to some problems that we need to overcome in our debug adapter first. And the lack of going deeper into the variable tree hierarchy. Something that @martin-fleck-at plans to look into.

Adjustments that we may need to consider in future

Contribute the Go to value in Memory Inspector also to the Variables window context menu.
Consider array type objects in the maybe check. Effectively they are pointers, too.
Really fix defects and deal with formats our debug adapters return as values. For example: (EventRecord_t*) 0x200BF840 . Ideally we find something generic or some clever filters one can contribute through the Adapter Capability API rather than re-implementing the whole C-Tracker to do some minor adjustments. But this is something beyond this PR.

@gbodeen , @colin-grant-work , please check if some of my feedback is worth to address with this PR. Otherwise, I am good with this change getting merged.

jreineckearm · 2024-05-04T06:47:34Z

@colin-grant-work , @gbodeen , just checked latest response on this thread. I am fine with them. And what I was able to test at this point with our Arm Debugger extension looked good. Happy to get this merged.

gbodeen added 2 commits April 4, 2024 22:02

Allow inspecting child tree elements in Variables view

eac8f67

Follow pointers from variable context menu

7804cfd

colin-grant-work reviewed Apr 4, 2024

View reviewed changes

jreineckearm reviewed Apr 8, 2024

View reviewed changes

gbodeen added 2 commits April 9, 2024 21:34

Revert "Allow inspecting child tree elements in Variables view"

9107bbc

This reverts commit eac8f67.

Generalize pointer-following to value-following

1cc7ded

colin-grant-work reviewed Apr 17, 2024

View reviewed changes

src/plugin/adapter-registry/c-tracker.ts Outdated Show resolved Hide resolved

src/plugin/adapter-registry/c-tracker.ts Outdated Show resolved Hide resolved

gbodeen added 2 commits April 23, 2024 20:38

Handle GDB, ArmDebugger, other

cf34847

Too many edge cases, so simplify.

1292d0f

colin-grant-work reviewed Apr 24, 2024

View reviewed changes

src/plugin/adapter-registry/c-tracker.ts Outdated Show resolved Hide resolved

Don't check validity

bc8d49b

jreineckearm reviewed Apr 30, 2024

View reviewed changes

src/common/debug-requests.ts Show resolved Hide resolved

jreineckearm reviewed Apr 30, 2024

View reviewed changes

src/plugin/adapter-registry/c-tracker.ts Show resolved Hide resolved

jreineckearm reviewed Apr 30, 2024

View reviewed changes

jreineckearm approved these changes Apr 30, 2024

View reviewed changes

colin-grant-work approved these changes May 6, 2024

View reviewed changes

colin-grant-work merged commit 36db026 into eclipse-cdt-cloud:main May 6, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow pointers #122

Follow pointers #122

gbodeen commented Apr 4, 2024 •

edited by colin-grant-work

Loading

colin-grant-work Apr 4, 2024

jreineckearm Apr 8, 2024

jreineckearm Apr 8, 2024

colin-grant-work Apr 8, 2024

jreineckearm Apr 8, 2024

jreineckearm commented Apr 8, 2024 •

edited

Loading

gbodeen commented Apr 11, 2024 •

edited

Loading

colin-grant-work commented Apr 29, 2024

jreineckearm Apr 30, 2024

jreineckearm Apr 30, 2024

gbodeen Apr 30, 2024

jreineckearm May 4, 2024

jreineckearm left a comment

jreineckearm commented May 4, 2024

	"title": "Go to value in Memory Inspector",
	"title": "Go to Value in Memory Inspector",

Follow pointers #122

Follow pointers #122

Conversation

gbodeen commented Apr 4, 2024 • edited by colin-grant-work Loading

What it does

How to test

Review checklist

Reminder for reviewers

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

jreineckearm commented Apr 8, 2024 • edited Loading

gbodeen commented Apr 11, 2024 • edited Loading

colin-grant-work commented Apr 29, 2024

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

jreineckearm left a comment

Choose a reason for hiding this comment

jreineckearm commented May 4, 2024

gbodeen commented Apr 4, 2024 •

edited by colin-grant-work

Loading

jreineckearm commented Apr 8, 2024 •

edited

Loading

gbodeen commented Apr 11, 2024 •

edited

Loading