-
Notifications
You must be signed in to change notification settings - Fork 56
Invalidate variables after writing memory #565
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3111,6 +3111,8 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession { | |
| const hexContent = base64ToHex(data); | ||
| 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'])); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would recommend to move the sending of the
GDB notifications would offload this hassle of checking writing memory errors from us
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. For this PR, my goal is to keep the change minimal and address the Variables refresh issue reported for Memory Inspector writes. |
||
| } catch (err) { | ||
| this.sendErrorResponse( | ||
| response, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.