Invalidate variables after writing memory - #565
Conversation
jonahgraham
left a comment
There was a problem hiding this comment.
LGTM - build needs to succeed, seems to have failed for unrelated reasons.
| await mi.sendDataWriteMemoryBytes(gdb, memoryReference, hexContent); | ||
| this.sendResponse(response); | ||
| // inform to Variables view that need to refresh data on the view | ||
| this.sendEvent(new InvalidatedEvent(['variables'])); |
There was a problem hiding this comment.
While this is probably enough invalidated for most actions. Did you look at whether we should invalidate stack, and other items? (dap link)
While most writes won't change stack state, if a memory write changes stored location of return pointer, or other saved registers in the stack things could change.
There was a problem hiding this comment.
Dear @jonahgraham ,
Thank you for bringing this up. It made me think about scenarios where a memory write could affect stack-related data as well.
In theory, we could use InvalidatedEvent(['all']) to refresh additional debugger views after a memory write.
However, I found it difficult to come up with a reliable test case for scenarios where a user modifies a saved return address or saved PC through writeMemory. Reproducing such cases would likely require preparing a specific stack layout, identifying the saved return address (or saved PC), and then modifying it through writeMemoryRequest().
For this change, I focused on the original Variables/Watch refresh issue, so I limited the invalidation scope to "variables".
I haven't explored the stack-related scenarios in detail yet. That feels like a separate investigation, since it would require understanding how memory writes can affect stack frames, saved registers, or return addresses, and how those changes should be reflected in the UI.
There was a problem hiding this comment.
Dear @jonahgraham ,
I investigated this scenario further on a Cortex-R52 target.
I modified a saved LR value on the stack through Memory Inspector. The saved return address changed from 0x40806758 to 0x4080675C.
With InvalidatedEvent(['variables']), VS Code refreshed scopes and variables, but did not request a new stack trace.
I also tested InvalidatedEvent(['all']). In that case, VS Code did issue new stackTrace requests. However, GDB initially returned the original stack frame because it was still using its cached frame information.
After explicitly invalidating GDB's cached frames with gdb.invalidate_cached_frames(), the backtrace changed from: 0x40806758 in main() at loader_main.c:100
to: 0x4080675C in main() at loader_main.c:108
Therefore, invalidating all DAP areas causes VS Code to refresh the Call Stack, but it may not be sufficient by itself because GDB's frame cache also needs to be considered.
This seems broader than the original Variables synchronization issue and may be better handled as a separate investigation.
So, I limited the invalidation scope to "variables". How about your idea?
There was a problem hiding this comment.
@duyhihi91 that sounds like a reasonable conclusion and you fully answered my question now of "Did you look at whether we should invalidate stack, and other items?"
I am marking this resolved.
|
I don't see any problem with merging this as is. I will give @jreineckearm a moment to review (or assign to others to review) before it gets merged. |
|
I am good with this, too. We'll probably receive now two events on a memory write: a variables Invalidate and a Memory event (see this location where we fire the Memory Event in response to a GDB/MI memory-changed notification). But IMO views need to be able to gracefully deal with many events in quick succession. So all good. @omarArm , do you have any concerns about this? You added invalidate events in other locations and hence may have a better feel for such change and its impact. Happy to merge tomorrow, if Omar doesn't see a problem with it. |
| await mi.sendDataWriteMemoryBytes(gdb, memoryReference, hexContent); | ||
| this.sendResponse(response); | ||
| // inform to Variables view that need to refresh data on the view | ||
| this.sendEvent(new InvalidatedEvent(['variables'])); |
There was a problem hiding this comment.
I would recommend to move the sending of the invalidated event to be under handleGDBNotify for the following reason:
- If a user triggers a writeMemoryRequest with the same exact data, then an
invalidatedevent would be sent anyway. - If for any reason the memory didn't properly change, the
invalidatedevent would be sent nonetheless.
GDB notifications would offload this hassle of checking writing memory errors from us
There was a problem hiding this comment.
@duyhihi91 What do you think of @omarArm's suggestion? I think having the invalidated where it is currently works well and is pretty simple. I am not sure that the overhead of an extra invalidated event matters that much here. Does GDB elide the async notify if a memory write does not change values? If not, then this seems the same effect.
There is a different case that means using async notify would be good to do too. If a user uses CLI to modify memory, then it would be useful to generate the invalidated event.
There was a problem hiding this comment.
Dear @omarArm and @jonahgraham ,
Thank you for your feedback.
Omar's idea gives me a wide view of the general issues. I can see the benefit of moving the invalidated event closer to a GDB notification source, especially if we want to cover memory modifications coming from places other than the DAP writeMemory request (for example CLI commands).
I spent some time investigating the stack-related scenarios as well. During testing I found that GDB frame caching adds another layer of complexity. Even when the memory value changed and VS Code requested a new stack trace, GDB continued to return the old frame information until its cached frames were invalidated.
Because of that, the broader "any source of memory modification" case feels a bit beyond the scope of the original Variable refresh issue and may be better handled as a separate follow-up investigation.
That said, I agree there is value in the direction you're suggesting, especially if we decide to generalize memory change handling in the future.
For this PR, my goal is to keep the change minimal and address the Variables refresh issue reported for Memory Inspector writes.
Summary
Refresh Variables views after a successful writeMemory request by emitting a DAP InvalidatedEvent.
Problem
The target memory is correctly updated through the DAP writeMemory request, but the Variables views can continue displaying stale values until another debugger action (such as Step, Continue, or Refresh) causes VS Code to reevaluate the variables.
This behavior can be reproduced through the Memory Inspector (https://github.com/eclipse-cdt-cloud/vscode-memory-inspector):
Debug the target.
Open a variable in Memory Inspector view.
Modify the memory value.
Enter the change.
The memory is updated successfully on Memory Inspector view, but the corresponding value displayed in Variables view is not updated/refreshed immediately.
Cause
writeMemoryRequest() writes the requested bytes to target memory and returns a successful response, but it does not notify the debug client that previously displayed variable values may no longer be valid.
As a result, VS Code keeps using a stale snapshot of variable data until another debugger action triggers reevaluation.
Solution
new InvalidatedEvent(['variables'])
Test
Update "src/integration-tests/mem.spec.ts":
Extended the existing memory write test to verify:
memory is written successfully;
an invalidated event is emitted;
the invalidated area is variables;
memory contents remain correct after the write
Dear @jonahgraham ,
Could you please help to review this fixing?
Thank you so much.