Support storing and applying of memory content #96

martin-fleck-at · 2024-03-07T11:51:50Z

What it does

Add commands to store and apply memory content as Intel HEX file

Encapsulate behavior in new MemoryStorage class
Trigger 'store' from Memory view, Variables view and command palette
Trigger 'apply' from Memory view, Explorer view and command palette
Use nrf-intel-hex library for read/write file licensed under BSD-3

Use quick inputs to guide user through necessary input

Initialize as much of the input as possible through command args

Communicate with webview through messenger requests and notifications

Request to trigger store and apply from webview
Notify webview about any written memory so it can update properly

Minor improvements

Move some common types and functionality into 'common' area
Avoid bleeding Debug Adapter types into webview, use messaging types
Common style: 'getVariables' -> 'getVariablesType'
Provide utility functions and types for debug requests
Fix 'Enter' handling for numpad by checking key value of event

memory-store-apply.mp4

Closes #50

How to test

Run a debug session
Check how you can store memory (from the inspector, from the command, from the Variables window)
Try applying your stored memory (from the inspector, from the file context menu, from the file explorer context menu)

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

colin-grant-work · 2024-03-07T15:47:25Z

src/common/debug-requests.ts

@@ -0,0 +1,39 @@
+/********************************************************************************
+ * Copyright (C) 2018 Red Hat, Inc. and others.


Never quite sure what to do with headers in copied code. Feels funny to have a header date older than the repo, though :-).

Haha, that is a very interesting observation! I try to opt for the safest option but this really results in this slight repository paradox. If you want me to change it, let me know or maybe @planger knows whether there are any particular rules for the project.

Now I'd say that you've rewritten fairly thoroughly, so it likely makes sense to put your company and the current date in here.

colin-grant-work · 2024-03-07T17:09:55Z

src/common/debug-requests.ts

+export interface DebugRequestTypes {
+    'evaluate': [DebugProtocol.EvaluateArguments, DebugProtocol.EvaluateResponse['body']]
+    'readMemory': [DebugProtocol.ReadMemoryArguments, DebugProtocol.ReadMemoryResponse['body']]
+    'writeMemory': [DebugProtocol.WriteMemoryArguments, DebugProtocol.WriteMemoryResponse['body']]
+}


This sort of thing is maybe the only time I wish we had macros (and that the DAP had types for the body of responses!).

It sounds like you don't like the idea of using the DebugProtocol interfaces directly, but making this the primary declaration makes for ugly code in messaging.ts where we refer to the things declared here by array index.

How strongly do you feel about not using the DAP types? We were already aliasing the body types for ergonomics, and we are in fact using them, so aliasing them doesn't change much.

If you strongly prefer not using them directly anywhere but where we alias them, I think I'd prefer that we alias the types here for use elsewhere:

Suggested change

export interface DebugRequestTypes {

'evaluate': [DebugProtocol.EvaluateArguments, DebugProtocol.EvaluateResponse['body']]

'readMemory': [DebugProtocol.ReadMemoryArguments, DebugProtocol.ReadMemoryResponse['body']]

'writeMemory': [DebugProtocol.WriteMemoryArguments, DebugProtocol.WriteMemoryResponse['body']]

}

export type ReadMemoryArguments = DebugProtocol.ReadMemoryArguments;

export type ReadMemoryResult = DebugProtocol.ReadMemoryResponse['body'];

export type WriteMemoryArguments = DebugProtocol.WriteMemoryArguments;

export type WriteMemoryResult = DebugProtocol.WriteMemoryResponse['body'];

export type EvaluateArguments = DebugProtocol.EvaluateArguments;

export type EvaluateResult = DebugProtocol.EvaluateResponse['body'];

export interface DebugRequestTypes {

'evaluate': [EvaluateArguments, EvaluateResult]

'readMemory': [ReadMemoryArguments, ReadMemoryResult]

'writeMemory': [WriteMemoryArguments, WriteMemoryResult]

}

Absolutely true! I wonder if the VS Code people had a discussion about that because as soon as you actually start using it, you really see how accessing the body of the responses makes for really long and really weird types ;-)

So my thought process in the split between the debug-requests.ts and messaging.ts is that the messaging is really the protocol between the extension's main side and the extension webview whereas the debug protocol messages are between the extension's main side and the debug adapter.

Therefore, bleeding any types from the debug protocol into the webview does not make much sense as it cannot directly access the debug adapter. Following that, it seemed logical to have a messaging layer in-between where we define the arguments and results of the main-webview communication. Sure, some of those types may be a 1:1 alias for a debug protocol message but we may also want to extend them at some point (like I do with the WriteMemoryArguments), make parts required or optional (with a lot of the Partial going around) or re-define them. So in a way, having all those DebugProtocol.ReadMemoryArguments in the webview and the options is really more a "coincidence" because it obviously represents memory reading well but and also avoids giving some of the usages a more specific semantic imho.

Based on that, I'm absolutely not opposed to using the DebugProtocol interfaces on the extension's main side within all the debug-related classes (which is why I also left them in the memory-provider). Of course I wished for their names to be shorter, so we could alias them as in the debug-requests.ts as you suggested but for me personally, that would not replace the type definitions that we do in messaging.ts.

Long story short: I think we should distinguish between the types used in the main-debug communication and the ones in the main-webview communication. However, maybe I just overthought the whole thing so I don't plan to die on this hill ;-) I'd be very happy to get your thoughts on that though.

src/common/debug-requests.ts

src/webview/memory-webview-view.tsx

src/common/memory-range.ts

src/plugin/memory-webview-main.ts

src/webview/components/memory-table.tsx

src/webview/components/memory-widget.tsx

src/webview/memory-webview-view.tsx

jreineckearm · 2024-03-08T10:56:20Z

package.json

@@ -92,19 +104,45 @@
        {
          "command": "memory-inspector.show-variable",
          "when": "false"
+        },
+        {
+          "command": "memory-inspector.store-file",


We should disable the commands/buttons in the window if no debug session is active.
At the moment, I can click the buttons outside the debug session and get errors as expected.

That is a very good point!

We can see that the command is already disabled as it does not show up in the command palette anymore (F1 + 'memory' because we can no longer read or write. I moved the enablement condition directly to the command but it seems that VS code still allows us to simply execute the command via code and I don't see any other way to determine whether our command is enabled or not. The documentation even states it like that:

(Optional) Condition which must be true to enable the command in the UI (menu and keybindings). Does not prevent executing the command by other means, like the `executeCommand`-api.

I added a new commit that now shares the current session state with the memory inspector so we can properly determine the read/write capabilities and disable our buttons accordingly.

martin-fleck-at · 2024-03-08T12:13:06Z

@colin-grant-work Thank you for your detailed feedback! I tried to address everything in a follow up commit (after a rebase) and added another commit on top based on the feedback from Jens. Besides the type placement, I think I have addressed everything.

package.json

src/plugin/memory-storage.ts

jreineckearm · 2024-03-12T09:00:26Z

@martin-fleck-at , re-tested functionality.

Did the ultimate test with storing memory from one of our existing debug tools to a hex file. And applying it with the Memory Inspector. And vice versa. Looks all healthy! :-)

The recent change to connect a memory window instance to a debug session ID has the unfortunate side effect of not being able to reuse a memory window anymore. This was possible, would be good to get that back. Use case: I debug something and use the Memory Inspector, I make a code change, then restart debug, and would like to inspect related changes in the previously opened window instance.

Some minor suggestions inline.

One bigger change that may be required and is possibly out of scope for this one is to honor MS-DAP memory events. Our debug adapter correctly sends them on memory contents change. But I see that the Memory Inspector debug tracker doesn't make use of them to trigger a refresh if in the loaded memory range of a window instance. At the moment you need to manually trigger a refresh after loading a hex file into memory.

martin-fleck-at · 2024-03-12T14:02:16Z

@jreineckearm Thank you very much for your feedback! I rebased the change and added the new functionality on top: The memory inspector should now properly refresh automatically and we also support the memory event as requested here: #105.

Furthermore, I added the missing context menu entries for #51.

Could you please have another look at it?

colin-grant-work · 2024-03-12T16:08:21Z

Quite a few changes since my last look - I'll read things over later today.

martin-fleck-at · 2024-03-12T16:54:52Z

@colin-grant-work Thank you very much! I added them as separate commits so it shouldn't be too bad, I hope! I'll do a quick rebase then.

Add commands to store and apply memory content as Intel HEX file - Encapsulate behavior in new MemoryStorage class - Trigger 'store' from Memory view, Variables view and command palette - Trigger 'apply' from Memory view, Explorer view and command palette - Use nrf-intel-hex library for read/write file licensed under BSD-3 Use quick inputs to guide user through necessary input - Initialize as much of the input as possible through command args Communicate with webview through messenger requests and notifications -- Request to trigger store and apply from webview -- Notify webview about any written memory so it can update properly Minor improvements - Move some common types and functionality into 'common' area - Avoid bleeding Debug Adapter types into webview, use messaging types - Common style: 'getVariables' -> 'getVariablesType' - Provide utility functions and types for debug requests - Fix 'Enter' handling for numpad by checking key value of event Closes eclipse-cdt-cloud#50

- Move evaluation expression for addressOf and sizeOf to adapter - Make 'toHexStringWithRadixMarker' more flexible - Shorten validation messages for memory - Align props and method names with existing method names, no prefixes - Replace 'MemoryVariableNode' with 'IVariablesContext' - Add comment on overlap calculation - Make sure command enablement fails early

- VS Code always allows to execute a command programmatically -- There is no way to determine whether a command is enabled or not - Forward current session context to memory inspector

- Integrate commands with context menu - React to 'memory' event from debug adapter - Use 'Store Memory to File' as proper storage label - Use workspace folder as default save location - Minor fix for numpad key enter on memory options

martin-fleck-at · 2024-03-12T17:27:12Z

@colin-grant-work @jreineckearm Everything should be good to review again ;-)

jreineckearm · 2024-03-12T17:28:57Z

Thanks, @martin-fleck-at !

From a functionality point of view, things work as outlined in your last comment. This is a really nice new feature!

Reusing existing and configured Memory Inspector windows for subsequent debug sessions works again.
The apply/store buttons are now disabled if debug session is not active to prevent users from provoking errors when functionality simply can't work.
Context menu has been extended by the two commands. Getting crowded there. Need to consider grouping commands by sections/sub-menus when adding new functionality. But I guess we need think about a similar "redesign" for the in-window options menu going forward.
Apply/store still works as expected.
Open and save dialogs now start at ${workspaceFolder}.
"memory" event support works nicely. I can see the effect now when editing variables and using commands in the Debug Console of our Arm Debugger debug adapter extension.

Minor improvement for later (not this PR)

We have a context menu entry on the variables view to store data for variables living in memory. But there is no counter-part to quickly load a data set into a variable.

jreineckearm

~~Nice, thanks for incorporating the feedback! LGTM!~~
Sorry, wrong Github window. But applies here of course, too. :-)
(Anyone who has the power feel free to delete this comment, and please do not merge until @colin-grant-work had a chance to review).

colin-grant-work · 2024-03-13T01:07:48Z

src/common/debug-requests.ts

@@ -0,0 +1,39 @@
+/********************************************************************************
+ * Copyright (C) 2018 Red Hat, Inc. and others.


Now I'd say that you've rewritten fairly thoroughly, so it likely makes sense to put your company and the current date in here.

src/common/debug-requests.ts

src/common/messaging.ts

src/plugin/adapter-registry/adapter-capabilities.ts

src/plugin/adapter-registry/c-tracker.ts

src/plugin/external-views.ts

src/plugin/memory-provider.ts

- Use short (PascalCase) names for generic arguments - Adapt copyright header in debug-requests - Remove optional 'count' parameter in messaging and memory-provider - Remove caching of session context and simply re-create if necessary - Execute variable queries in parallel instead of sequentially - Fix typo in comment

martin-fleck-at · 2024-03-13T09:45:48Z

@colin-grant-work @jreineckearm I pushed an update that should address the latest concerns.

jreineckearm · 2024-03-13T10:48:05Z

src/plugin/adapter-registry/c-tracker.ts

@@ -42,13 +42,14 @@ export class CTracker extends AdapterVariableTracker {
            return undefined;
        }
        try {
-            const variableAddress = await this.getAddressOfVariable(variable.name, session);
+            const [variableAddress, variableSize] = await Promise.all([
+                variable.memoryReference && hexAddress.test(variable.memoryReference) ? variable.memoryReference : this.getAddressOfVariable(variable.name, session),


Should we consider different integer representations? Not sure if all debug adapters would always return a hex value for an address (although IMO the sensible thing to do).

That is a good point! But I'd rather defer this discussion into a separate issue. The previous behavior also used the hexAddress. I did add a commit that unifies that check but I still think it does not necessarily have to be part of this PR, do you agree?

Fine with me. TBF, I'd expect addresses to be returned as hex values anyway.

Since this isn't standardized, I'd probably prefer a check for any (easily parsed) number format so that 16 is valid in addition to 0x10. That would follow the convention for things like the ReadMemoryResponse and DisassembledInstruction where at least hex and decimal are both considered acceptable formats.

Fantastic, thank you! I added #107 to track this. It shouldn't be too much work but it needs to be discussed first (all formats vs "only" hexadecimal/decimal because they are mentioned in the DAP).

colin-grant-work · 2024-03-13T16:30:28Z

src/plugin/memory-webview-main.ts

+    protected async storeMemory(storeArguments: StoreMemoryArguments): Promise<void> {
+        // Even if we disable the command in VS Code through enablement or when condition, programmatic execution is still possible.
+        // However, we want to fail early in case the user tries to execute a disabled command
+        if (!this.memoryProvider.createContext().canRead) {
+            throw new Error('Cannot read memory, no valid debug session.');
+        }
+        return vscode.commands.executeCommand(StoreCommandType, storeArguments);
+    }
+
+    protected async applyMemory(): Promise<MemoryOptions> {
+        // Even if we disable the command in VS Code through enablement or when condition, programmatic execution is still possible.
+        // However, we want to fail early in case the user tries to execute a disabled command
+        if (!this.memoryProvider.createContext().canWrite) {
+            throw new Error('Cannot write memory, no valid debug session.');
+        }
+        return vscode.commands.executeCommand(ApplyCommandType);
+    }


For a follow-up: since we're sending a context to the view that includes information about the session that that window is associated with, these requests could handle that association and check the relevant session, rather than defaulting to the active session. Re: #97.

This is a very good point and I believe that should be handled as part of #97 as we definitely want to do this for all requests (read, write, store, apply, get variables).

colin-grant-work

I'm happy with the changes. Both of my last comments can be handled as a follow-up, though I'd like it if the address parsing were handled in this PR.

martin-fleck-at · 2024-03-13T19:36:46Z

@colin-grant-work Thank you very much! I'll push an update for the decimal address support in a few minutes!

- Provide decimal address regex and function to extract address - Ensure we can properly serialize bigints when logging data - Ensure we return a proper variable range if we only have the address

martin-fleck-at · 2024-03-13T19:54:00Z

@colin-grant-work If you agree with the latest commit, please feel free to merge this PR.

colin-grant-work reviewed Mar 7, 2024

View reviewed changes

colin-grant-work mentioned this pull request Mar 7, 2024

Introduce lint rule for import order #98

Closed

jreineckearm reviewed Mar 8, 2024

View reviewed changes

martin-fleck-at force-pushed the issues/50 branch 2 times, most recently from 100e9d0 to 9c497d1 Compare March 8, 2024 12:09

martin-fleck-at force-pushed the issues/50 branch 2 times, most recently from c9f3c03 to 733088f Compare March 8, 2024 12:56

jreineckearm reviewed Mar 12, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

jreineckearm reviewed Mar 12, 2024

View reviewed changes

src/plugin/memory-storage.ts Outdated Show resolved Hide resolved

martin-fleck-at force-pushed the issues/50 branch 2 times, most recently from 8cd7612 to e945093 Compare March 12, 2024 13:57

martin-fleck-at added 3 commits March 12, 2024 17:57

PR Feedback: Disable store/apply memory buttons if not possible

53248da

- VS Code always allows to execute a command programmatically -- There is no way to determine whether a command is enabled or not - Forward current session context to memory inspector

martin-fleck-at force-pushed the issues/50 branch from e945093 to 934e95e Compare March 12, 2024 17:20

martin-fleck-at force-pushed the issues/50 branch from 934e95e to 4b0c003 Compare March 12, 2024 17:26

jreineckearm approved these changes Mar 12, 2024

View reviewed changes

colin-grant-work reviewed Mar 13, 2024

View reviewed changes

martin-fleck-at force-pushed the issues/50 branch from ef0e3b2 to 83c51b5 Compare March 13, 2024 09:20

jreineckearm reviewed Mar 13, 2024

View reviewed changes

martin-fleck-at force-pushed the issues/50 branch from 69c5c9d to 8583bb8 Compare March 13, 2024 11:08

Unify usage of hex address conversion

175a330

martin-fleck-at force-pushed the issues/50 branch from 8583bb8 to 175a330 Compare March 13, 2024 11:10

colin-grant-work reviewed Mar 13, 2024

View reviewed changes

colin-grant-work approved these changes Mar 13, 2024

View reviewed changes

martin-fleck-at mentioned this pull request Mar 13, 2024

Support different memory address representations for variables #107

Open

Improve address handling and support decimal addresses

edf986d

- Provide decimal address regex and function to extract address - Ensure we can properly serialize bigints when logging data - Ensure we return a proper variable range if we only have the address

colin-grant-work merged commit bbf3e9d into eclipse-cdt-cloud:main Mar 13, 2024
5 checks passed

martin-fleck-at mentioned this pull request Mar 14, 2024

Use DAP protocol's "memory" event to refresh affected Memory Inspector views #105

Closed

colin-grant-work mentioned this pull request Mar 18, 2024

Memory Inspector context menu enhancements #51

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support storing and applying of memory content #96

Support storing and applying of memory content #96

martin-fleck-at commented Mar 7, 2024 •

edited

colin-grant-work Mar 7, 2024

martin-fleck-at Mar 8, 2024

colin-grant-work Mar 13, 2024

colin-grant-work Mar 7, 2024

martin-fleck-at Mar 8, 2024

jreineckearm Mar 8, 2024

martin-fleck-at Mar 8, 2024

martin-fleck-at commented Mar 8, 2024

jreineckearm commented Mar 12, 2024

martin-fleck-at commented Mar 12, 2024

colin-grant-work commented Mar 12, 2024

martin-fleck-at commented Mar 12, 2024

martin-fleck-at commented Mar 12, 2024

jreineckearm commented Mar 12, 2024

jreineckearm left a comment •

edited

colin-grant-work Mar 13, 2024

martin-fleck-at commented Mar 13, 2024

jreineckearm Mar 13, 2024

martin-fleck-at Mar 13, 2024

jreineckearm Mar 13, 2024

colin-grant-work Mar 13, 2024

martin-fleck-at Mar 13, 2024

colin-grant-work Mar 13, 2024

martin-fleck-at Mar 13, 2024

colin-grant-work left a comment

martin-fleck-at commented Mar 13, 2024

martin-fleck-at commented Mar 13, 2024

		@@ -0,0 +1,39 @@
		/********************************************************************************
		* Copyright (C) 2018 Red Hat, Inc. and others.

-export interface DebugRequestTypes {
-    'evaluate': [DebugProtocol.EvaluateArguments, DebugProtocol.EvaluateResponse['body']]
-    'readMemory': [DebugProtocol.ReadMemoryArguments, DebugProtocol.ReadMemoryResponse['body']]
-    'writeMemory': [DebugProtocol.WriteMemoryArguments, DebugProtocol.WriteMemoryResponse['body']]
-}
+export type ReadMemoryArguments = DebugProtocol.ReadMemoryArguments;
+export type ReadMemoryResult = DebugProtocol.ReadMemoryResponse['body'];
+export type WriteMemoryArguments = DebugProtocol.WriteMemoryArguments;
+export type WriteMemoryResult = DebugProtocol.WriteMemoryResponse['body'];
+export type EvaluateArguments = DebugProtocol.EvaluateArguments;
+export type EvaluateResult = DebugProtocol.EvaluateResponse['body'];
+export interface DebugRequestTypes {
+    'evaluate': [EvaluateArguments, EvaluateResult]
+    'readMemory': [ReadMemoryArguments, ReadMemoryResult]
+    'writeMemory': [WriteMemoryArguments, WriteMemoryResult]
+}

Support storing and applying of memory content #96

Support storing and applying of memory content #96

Conversation

martin-fleck-at commented Mar 7, 2024 • edited

What it does

How to test

Review checklist

Reminder for reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-fleck-at commented Mar 8, 2024

jreineckearm commented Mar 12, 2024

martin-fleck-at commented Mar 12, 2024

colin-grant-work commented Mar 12, 2024

martin-fleck-at commented Mar 12, 2024

martin-fleck-at commented Mar 12, 2024

jreineckearm commented Mar 12, 2024

jreineckearm left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-fleck-at commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-grant-work left a comment

Choose a reason for hiding this comment

martin-fleck-at commented Mar 13, 2024

martin-fleck-at commented Mar 13, 2024

martin-fleck-at commented Mar 7, 2024 •

edited

jreineckearm left a comment •

edited