eclipse-cdt-cloud / vscode-memory-inspector

vscode memory inspector
https://open-vsx.org/extension/eclipse-cdt/memory-inspector
Eclipse Public License 2.0
6 stars 10 forks source link

Follow pointers #122

Closed gbodeen closed 5 months ago

gbodeen commented 6 months ago

What it does

Closes #123.

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

  1. 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.
    image
  2. Provide a "Follow Pointer" context menu on pointer variables in the memory inspector Variables column.
    image

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

  1. Right-click on child elements in the debug Variables view & choose "Show in Memory Inspector".
  2. 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

Reminder for reviewers

jreineckearm commented 6 months ago

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.

gbodeen commented 6 months ago

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".)

image

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

colin-grant-work commented 5 months ago

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

jreineckearm commented 5 months ago

@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.