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

[jreineckearm]

Description

The Memory Inspector has a variable tracker mechanism to determine valid variables and variable ranges for the Variable Decorator column. This works well in many cases. But the current implementation causes a few problems with some of our debug adapters.

• We have scopes that introduce artificial variables for grouping. For example by source file name.
• Some local variable locations depend on their lifetime and may not even live in memory but in registers.
• Depending on compiler toolchain and the optimization level of the application file build, there can be leftover symbols that have a bogus memory reference of 0x0. Probably one of the things that won't be easy/possible to deal with on Memory Inspector side.

The above can result in failing evaluate requests. Causing error output in our implementations' Debug Console log.

I believe a refinement of the variable-to-range mapping making clever use of memoryReference, value, type, and/or other fields could reduce those errors. Also, we could skip address-of evaluate requests if we have a memoryReference that is a number. And skip size-of evaluate requests if the address-of request already failed. Only the latter would already reduce the error message by 50%.

Additional information

I've found a previous discussion between @thegecko and @colin-grant-work here: #16 (review)

This shows that we might need different variable tracker implementations depending on the debug adapters with built-in support: https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/blob/main/src/plugin/manifest.ts#L24
Which admittedly would be the least favored yet probably necessary approach. I also see that the variable tracker architecture is already prepared for such flavors.

[jreineckearm]

Happy to take this one on myself and provide a PR for discussion when I have something that improves the situation.

[thegecko]

Is this a situation where a default implementation exists in the extension, but can be extended by other extensions? E.g. a debug adapter can register it's own tracker.

[jreineckearm]

I believe it will partly boil down to this. A few of the changes that I currently explore may still agree with the current CTracker implementation.

But I think in the longer term a debug adapter may need to be able to override such parts with more specific knowledge about expression evaluation capabilities that go beyond the assumption of talking to a C/C++ debugger that supports & and sizeof expressions. Or even custom DAP extensions that allow a more efficient extraction of variables, their ranges, or their hierarchy w/o reading values from target. The location info coming with the debug information in the application file is for example sufficient for globals or other variables that have a fixed memory location.

[jreineckearm]

I see now that #5 also mentions a potential contribution mechanism for the very same reasons. :-)

[colin-grant-work]

I think some of the suggestions might be appropriate for implementation in the CTracker, but others should perhaps be placed in a separate contribution for debug adapters with specific behaviors.

Also, we could skip address-of evaluate requests if we have a memoryReference that is a number.

This one is likely to be correct, and in line with the DAP's intended behavior. As implemented, the current behavior mostly reflects the behavior of the CDT-GDB adapter, which sends &name as the memory reference.

And skip size-of evaluate requests if the address-of request already failed.

I believe that at the moment these two requests are made concurrently, so we don't know whether one has succeeded before we make the other. I'd prefer to keep them concurrent for whatever speed increase that brings. If a different strategy is preferable for particular adapters, it may appropriate to implement a separate contribution for those adapters.

Some local variable locations depend on their lifetime and may not even live in memory but in registers.
Depending on compiler toolchain and the optimization level of the application file build, there can be leftover symbols that have a bogus memory reference of 0x0. Probably one of the things that won't be easy/possible to deal with on Memory Inspector side.

As you say, I think both of these may be tricky to handle on the Memory Inspector side. The former is part of the reason I added getResidents as an optional field on the AdapterCapabilities interface. The idea there is that if you're aware of different memory regions (stack, registers, heap, etc.), then it may be possible to use the memory reference of a given view to narrow the scope of the variable search. We use it downstream to distinguish between shared and local memory, for example, so that if we know that the memory in a view is in the shared region, we can resolve only variables in the shared region, and vice versa for local memory. If the memory reference of a variable in a register reflects that, a tracker aware of that marking could skip that variable.

We have scopes that introduce artificial variables for grouping. For example by source file name.

This seems certainly to fall in the area of adapter-specific behavior, but there may be ways to filter these out in an upstream-friendly way; for example, we could reasonably decide to filter out any variable whose memory reference is a negative number.

[jreineckearm]

Thank you for your input, @colin-grant-work !

I'd prefer to keep them concurrent for whatever speed increase that brings.

Yes, they happen concurrently and I appreciate the performance concerns. While this would work well enough in our case, it may cause performance degradation with other adapters. So, we need to be extra careful.

Thanks for the hint with getResidents. I'll have a look how this could be leveraged for our use cases.

This seems certainly to fall in the area of adapter-specific behavior, but there may be ways to filter these out in an upstream-friendly way; for example, we could reasonably decide to filter out any variable whose memory reference is a negative number.

This is actually an interesting idea. I was so far exploring if checking variable names for their compliance with C variable names would help to filter out such artificial variables. This would work well enough for filenames. But I am not fully convinced of that approach myself yet.

The current implementation of getLocals assumes a hierarchy without artificial groupings. It misses all variables underneath those after applying some local tweaks. All changes to overcome this were so far quite intrusive. Which could be again a reason to look into the use of getResidents instead. I need to think about this a little more...

[colin-grant-work]

The current implementation of getLocals assumes a hierarchy without artificial groupings. It misses all variables underneath those after applying some local tweaks. All changes to overcome this were so far quite intrusive.

I'm open to some intrusive changes in this area. I think deeper traversal of the tree is likely a good thing to implement, and if it lets us address other weaknesses at the same time, that's all to the good. There was just enough complexity to figuring out how to handle the various combinations of requests and available data that I shied away from it for the initial implementation.