Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not clear when context menu items inside VARIABLES view must be shown #156

Open
AdrianOltean opened this issue Jan 30, 2025 · 4 comments
Open
Labels
bug Something isn't working

Comments

@AdrianOltean
Copy link

Description
Memory Inspector contributes two context menu items inside VARIABLES view - "Show in Memory Inspector" and "Store Memory to File". But it's not intuitive for users to determine which variables have these menu items enabled. Documentation does not help either and the enablement seems to also depend on the actual debug adapters used. From my experiments, when using CPPDG, menu items are always shown even for non-pointers or references:

Image

Moreover, with CPPDGB, Memory Inspector incorrectly renders memory contents from actual value of the "int" variable, not what's at the integer's address:

Image

When using Cortex Debug, menu items seem to have a more consistent enablement/visibility state, meaning that they are shown for (some) pointers or complex structs, but not for all of them. I couldn't easily figure out all the preconditions for the enablement. So it must be some implementation details from Memory Inspector that need to be documented in order to make it clear when the two menu items are shown.

Image

How to reproduce:

  1. NXP SDK, NXP LPC55S69 board, a J-Link debug probe. Should not matter as long as you can replicate what's written above.
  2. MCUXpresso for VS Code extension installed.
  3. Import a "Hello World" project using the MCUXpresso for VS Code extension.
  4. Debug.
  5. Step into "CLOCK_AttachClk".
  6. Open Variables view and right-click some variables and check the enablement state for the two context menu items.

Expected behavior

  1. Documentation should make it clear in what context must "Show in Memory Inspector" and "Store Memory to File" be enabled.
  2. Don't enable the menu items for non-pointers, unless the extension is capable to render what's at the variable's address, not at its actual value (e.g. for integers).

Environment

  • OS: WIndows 11
  • Theia or VS Code Version: VS Code 1.96.4
@AdrianOltean AdrianOltean added the bug Something isn't working label Jan 30, 2025
@AdrianOltean AdrianOltean changed the title Not clean when context menu items inside VARIABLES view must be shown Not clear when context menu items inside VARIABLES view must be shown Jan 30, 2025
@planger
Copy link
Contributor

planger commented Feb 13, 2025

@AdrianOltean Thanks for pointing out this issue!

I would need to double-check, I think the visibility of the menu items you are referring to is defined here, so the following when-expression applies canViewMemory && memory-inspector.canRead. canRead is set directly based on the debug capabilities sent by the debug adapter capabilities and published to the context key by the ContextTracker.

So we'd have to debug into that in more detail for your particular case to find out what's going on.

Would you be willing to debug that and suggest a fix?

Thank you very much in advance!

@colin-grant-work
Copy link

colin-grant-work commented Feb 13, 2025

The canViewMemory context key is set by VSCode and is true if the debug adapter for a given session has provided a memoryReference for the given variable. It's that memoryReference value that determines what the Memory Inspector loads, so if it appears to be loading the 'wrong' value for certain variables when hooked up to CPP Debug, that may indicate that CPP Debug is providing unexpected values. I know for example that the CDT-GDB adapter just provides the C-style address reference (&whatever) in that field, and then that is resolved by CDT-GDB when the memory is requested. It looks like for the integers you're interested in, CPP Debug must be sending the value of the int rather than its address.

@AdrianOltean
Copy link
Author

@planger @colin-grant-work Thanks for your comments. You're absolutely right about the functionality around canViewMemory and canRead context keys. But one would need to inspect the source code of the Memory Inspector + debug adapter (if available) to figure out how to simply view memory. As I mentioned in my initial description, Cortex Debug seems to do a slightly better job than CPPDBG when it comes to filling the memoryReference in a variable. CPPDBG is kind of broken when it comes to this aspect.

As a side-note, we (NXP) are developing the MCUXpresso for VS code extension and decided to use Memory Inspector as the default memory viewer/editor. But had a few reports from users and decided to log them directly here, as we were not able to work-around them from our extension. Also, strictly speaking about the memoryReference for variables, having our own debug adapter, we're now populating this with the real location/address (i.e. &(<var>)) for each variable or child member of a struct. So the inconsistent display of the context menu items contributed by Memory Inspector is no longer a problem for us. So we're really inspecting addresses now, not plain integers.

To conclude, I still think that (at least) the documentation should clearly describe when can memory be inspected and that this is tightly coupled to the debug adapter used.

@jreineckearm
Copy link
Contributor

jreineckearm commented Feb 14, 2025

@AdrianOltean , thanks for logging this user feedback here. This is really useful!

I agree that we need to improve our documentation and better explain how to open the Memory Inspector from various places. And how this works.

On the value vs pointer topic: I personally believe we need both. Including the distinction you mention about whether the location of a variable is in memory (at least the start in cases where a value is scattered over non-consecutive memory) and results in working "links". Appreciate that this can become a challenge and stands and falls with the used debug adapter and its characteristics. And in the case of the CDT-GDB debug adapter maybe even with the actually used GDB server.
But in general we should strive for supporting as many backends in the off-the-shelf extension versions here to make sure users don't need to readjust if they have to swap their debugger backend.

For whoever can pick this issue up, I'd suggest to start with the documentation improvements which helps us to capture the status quo. And then we can pick up further enhancements/fixes from there.

Note that I believe this is closely tied to #52 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants