Integrate Memory Inspector in Theia repository #11394

colin-grant-work · 2022-07-07T21:35:44Z

What it does

This PR transfers the Memory Inspector functionality from the Theia CPP Extensions to the Theia repository. Relative to the current state of that repository, this PR makes some minor changes necessary to operate with [email protected] and transfers some functionality that is not fully determined by the Debug Adapter Protocol into the MemoryProviderService, where it can be more easily adjusted for adapters with specific known properties.

How to test

With the CDT-GDB adapter, the instructions from this PR still apply
To demonstrate the functionality with another debug adapter: (less convenient than GDB, because it supports fewer reference options)
Create and open a new Rust project or use this one
In your workspace, make sure that the preference lldb.rpcServer is set to null to work around Incorrect conversions from null to undefined in new RPC #11392
Install Rust Analyzer and CodeLLDB
(If using the example project linked above) open main.rs and put a breakpoint on line 12.
Use the code lenses provided by Rust Analyzer to start a debug session from the testson lines 16-25.
When you hit the breakpoint, the terminal in which the program is running will have printed two addresses.
Open the memory inspector from the right panel and input one of the addresses into the address selector and hit Go. The memory location should load, and it should match the data of the struct whose address you entered.

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

JonasHelming · 2022-07-08T18:57:28Z

Just out of interest: Did you consider moving this to CDT.cloud instead of the Theia main repo?

colin-grant-work · 2022-07-12T21:41:20Z

@JonasHelming, I've made this PR for two reasons: I believe that having the code in this repo will make it easier to ensure its compatibility with latest Theia, and I believe that it's advantageous with respect to the features requested in #10841. What would be the benefit of placing it in CDT.cloud?

JonasHelming · 2022-07-13T08:35:54Z

Basically to keep the core Theia project as slim as possible. Most components would benefit from beeing moved to the core project/repo in terms of ensuring compatibility. Are the people maintaining this component the same team that are core Theia developers? Again: I am not opposed to move this to Theia core, I just wanted to raise the question as we had a tendency to try to keep the core repo as small as possible.

One example that you could aim at when hosting it at CDT.cloud would be to ensure compatibility for community releases only.
Again, up to you!

JonasHelming · 2022-07-19T07:10:38Z

Thinking about this again: the memory inspector is not necessarily bound to C/C++, correct? This would be an argument in favor to keep it in Theia.

colin-grant-work · 2022-07-19T14:04:46Z

@JonasHelming, exactly. I've been waiting till I could get it working on at least one non-C(++) debug adapter before trying to move it here, and I was finally able to get it working for Rust. :-)

vince-fugnitto

I added a second commit to localize everything I could find, and cleaned up the code slightly.
I still have some comments however.

vince-fugnitto · 2022-07-20T15:15:01Z

packages/memory-inspector/package.json

+    "@theia/core": "1.27.0",
+    "@theia/debug": "1.27.0",
+    "long": "^4.0.0",
+    "vscode-debugprotocol": "^1.48.0"


Do we need to also update vscode-debugprotocol in @theia/debug and @theia/plugin-ext to follow this version:

theia/packages/debug/package.json

Line 25 in b000f96

"vscode-debugprotocol": "^1.32.0"

theia/packages/plugin-ext/package.json

Line 45 in b000f96

"vscode-debugprotocol": "^1.32.0",

Because they're both caret versions, we're currently getting a high-enough version (1.51) in our yarn.lock, but it's probably best to be explicit.

I've updated the entries in our various package.json. They've deprecated this package name, so I've updated that, as well, with the unfortunate consequence that the new package name isn't part of the 3PP baseline, apparently.

packages/memory-inspector/src/browser/memory-inspector-frontend-contribution.ts

vince-fugnitto · 2022-07-20T15:26:48Z

packages/memory-inspector/src/browser/wrapper-widgets/memory-dockpanel-placeholder-widget.tsx

+    render(): React.ReactNode {
+        return (
+            <div className='t-mv-memory-fetch-error'>
+                Click the
+                {' '}
+                <i className='memory-view-icon toolbar' />
+                {' '}
+                icon to add a new memory view or the
+                {' '}
+                <i className='register-view-icon toolbar' />
+                {' '}
+                icon to add a register view.
+            </div>
+        );
+    }


We can probably clean up this method and localize if possible.

Cleaned up, but not localized. Not quite sure what to do with the fact that it's broken up by the icons - the pieces in between aren't meaningful enough to pass to an automated translation tool. @msujew, any suggestions?

msujew

I haven't tested it yet, but here are my obligatory i18n comments :)

Also please rebase on latest master to get access to automatic translations for your changes.

packages/memory-inspector/src/browser/diff-widget/memory-diff-options-widget.tsx

packages/memory-inspector/src/browser/diff-widget/memory-diff-select-widget.tsx

msujew · 2022-08-01T14:56:10Z

packages/memory-inspector/src/browser/memory-provider/cdt-gdb-memory-provider.ts

+            while (currentLevel.parent instanceof DebugVariable) {
+                currentLevel = currentLevel.parent;
+            }
+            return currentLevel.parent instanceof DebugScope && currentLevel.parent['raw'].name === 'Local';


Question: Are Local or Registers localized values? I'm feeling queasy about the comparison.

That's a fair observation. This class is intended to be tightly coupled to CDT-GDB adapter, and at the moment, it does not localize any of its text, and adapter returns are mapped fairly directly into the display in the Variables widget, so this should work, but it's certainly not as robust as might be ideal. To my knowledge, there aren't hard conventions about the naming of variable scopes - an adapter could return any labels it wants - so I don't see a very good way around this, at the moment. The DAP does specifically mention both that the name of a scope returned by an adapter should be displayed as is, and that that text may be translated (on the adapter's side, presumably).

packages/memory-inspector/src/browser/memory-widget/memory-table-widget.tsx

packages/memory-inspector/src/browser/register-widget/register-options-widget.tsx

packages/memory-inspector/src/browser/register-widget/register-widget-types.ts

packages/memory-inspector/src/browser/utils/memory-widget-manager.ts

JonasHelming · 2022-08-17T12:10:25Z

@colin-grant-work Is this ready for a re-review?

martin-fleck-at

Hi @colin-grant-work and everyone, thank you for the great work!

I had a look at this PR for an adopter where we customized the memory view to stick on a particular thread. The design of this component is really thought-out and it was easy to do most of the necessary customizations. Still, I thought I'd share some of my observations with you - mostly about making things a bit easier to override. Please note that my comments are just some minor things that should not prevent this PR from being merged.

martin-fleck-at · 2022-08-19T06:54:34Z

packages/memory-inspector/src/browser/utils/memory-widget-manager.ts

+    async createNewMemoryWidget<T extends MemoryWidget>(kind: 'register' | 'memory' = 'memory'): Promise<T> {
+        this.widgetDisplayId = this._availableWidgets.size !== 0 ? this.widgetDisplayId + 1 : 1;
+        const options: MemoryWidgetOptions = { identifier: this.createdWidgetCount += 1, displayId: this.widgetDisplayId };
+        const widgetId = kind === 'memory'


Do I understand that correctly that the only way to plug in a custom memory or widget view is to re-bind the manager and override this method? At least the README seems to suggest so. If that is true, it would be really create if this mapping from kind to widget id could be in a separate overridable/protected method for easier customization.

This is reasonable, but doing it right would require a bit of a refactor - I'll leave this one out for now.

Sure, you can keep the effort as low as possible. I just thought something simple along the lines of:

protected mapKindToWidgetId(kind: 'register' | 'memory' = 'memory') { return kind === 'memory' ? MemoryWidget.ID : RegisterWidget.ID; }

So in case you want the editable memory widget you could simply do:

protected override mapKindToWidgetId(kind: 'register' | 'memory' = 'memory') { return kind === 'memory' ? EditableMemoryWidget.ID : RegisterWidget.ID; }

But we can also keep it as is for now.

packages/memory-inspector/src/browser/memory-widget/memory-options-widget.tsx

packages/memory-inspector/src/browser/editable-widget/memory-editable-table-widget.tsx

martin-fleck-at · 2022-08-19T07:11:02Z

packages/memory-inspector/src/browser/editable-widget/memory-editable-table-widget.tsx

+                console.log('Problem writing memory with arguments', edit, '\n', e);
+            }
+        }
+        this.memoryEditsCompleted.resolve();


I noticed that when I submit edits there is no update on the view by default. We do resolve the edits which means that the next time the memory is changed it will update accordingly and the pending edits are cleared (handleMemoryChange). But this requires a specific user action. At the moment, the pending changes just remain in the UI. Should we trigger an explicit update from here or what is the expected flow after the user submits the edits?

This is a bit tricky because the debug session doesn't emit any events when memory changes - we just get a response. When we implemented this internally, we just had the debug adapter send a StoppedEvent after successfully updating memory, but that was a bit of a workaround, and I didn't include it when implementing the write memory request upstream in the CDT-GDB adapter. I think your suggestion of triggering a refresh cycle is probably the best way to handle it.

I've modified the code here so that if a write succeeds we refresh the memory, but we don't clear any pending edits.

I really, really like this solution! I tested it and it works and looks great! I think it is a great solution to clear the edits one by one if there is no error. Thank you, Colin!

packages/memory-inspector/src/browser/editable-widget/memory-editable-table-widget.tsx

martin-fleck-at · 2022-08-23T08:50:45Z

@vince-fugnitto @colin-grant-work: Thanks again for the great work! Is there any chance this PR can make it in the upcoming release? If there is any way I can help out, please let me know.

colin-grant-work · 2022-08-23T13:04:42Z

@martin-fleck-at, I'll devote some time to this today to address the outstanding comments.

colin-grant-work · 2022-08-23T21:59:29Z

@vince-fugnitto, @martin-fleck-at, I've addressed most of y'all's comments - since there's some interest in getting this into the release, would you mind taking another look?

martin-fleck-at

Hi Colin, I retested our customization with the new commits and everything still works great and I definitely saved a couple of lines of copied code ;-) I really appreciate the time you are taking for this, so thank you very much!

I found a few minors in the new code that again do not a affect the functionality so if you have some time you could fix them but otherwise I'd rather see this change merged than delayed for the next release.

packages/memory-inspector/src/browser/memory-widget/memory-options-widget.tsx

packages/memory-inspector/src/browser/editable-widget/memory-editable-table-widget.tsx

martin-fleck-at · 2022-08-24T07:53:40Z

packages/memory-inspector/src/browser/editable-widget/memory-editable-table-widget.tsx

+                console.log('Problem writing memory with arguments', edit, '\n', e);
+            }
+        }
+        this.memoryEditsCompleted.resolve();


I really, really like this solution! I tested it and it works and looks great! I think it is a great solution to clear the edits one by one if there is no error. Thank you, Colin!

packages/memory-inspector/package.json

martin-fleck-at · 2022-08-24T08:06:51Z

packages/memory-inspector/src/browser/utils/memory-widget-manager.ts

+    async createNewMemoryWidget<T extends MemoryWidget>(kind: 'register' | 'memory' = 'memory'): Promise<T> {
+        this.widgetDisplayId = this._availableWidgets.size !== 0 ? this.widgetDisplayId + 1 : 1;
+        const options: MemoryWidgetOptions = { identifier: this.createdWidgetCount += 1, displayId: this.widgetDisplayId };
+        const widgetId = kind === 'memory'


Sure, you can keep the effort as low as possible. I just thought something simple along the lines of:

protected mapKindToWidgetId(kind: 'register' | 'memory' = 'memory') { return kind === 'memory' ? MemoryWidget.ID : RegisterWidget.ID; }

So in case you want the editable memory widget you could simply do:

protected override mapKindToWidgetId(kind: 'register' | 'memory' = 'memory') { return kind === 'memory' ? EditableMemoryWidget.ID : RegisterWidget.ID; }

But we can also keep it as is for now.

packages/memory-inspector/src/browser/editable-widget/memory-editable-table-widget.tsx

packages/memory-inspector/src/browser/style/index.css

packages/memory-inspector/src/browser/editable-widget/memory-editable-table-widget.tsx

colin-grant-work · 2022-08-24T15:33:56Z

@martin-fleck-at, I've pushed commits that I hope should address your concerns and requests.

martin-fleck-at

Oh wow, everything has been done, thank you very much, Colin! I really appreciate it!

I re-tested the latest version with our customizations and everything works and looks great. It is really an awesome feature, thank you to everyone involved.

martin-fleck-at · 2022-08-25T07:20:24Z

@vince-fugnitto Do you want to have another look at it or do you think we can merge this?

Signed-off-by: vince-fugnitto <[email protected]>

colin-grant-work added debug issues that related to debug functionality proposal feature proposals (potential future features) labels Jul 7, 2022

colin-grant-work marked this pull request as draft July 7, 2022 22:32

colin-grant-work force-pushed the feature/memory-inspector branch from ab0ec7f to b6db786 Compare July 12, 2022 21:56

colin-grant-work marked this pull request as ready for review July 12, 2022 23:27

vince-fugnitto reviewed Jul 20, 2022

View reviewed changes

msujew reviewed Aug 1, 2022

View reviewed changes

colin-grant-work force-pushed the feature/memory-inspector branch from 167ec32 to d8f1b57 Compare August 4, 2022 16:31

vince-fugnitto self-requested a review August 17, 2022 14:08

martin-fleck-at reviewed Aug 19, 2022

View reviewed changes

colin-grant-work force-pushed the feature/memory-inspector branch 2 times, most recently from ae1d582 to a3785f8 Compare August 23, 2022 21:04

martin-fleck-at approved these changes Aug 24, 2022

View reviewed changes

martin-fleck-at reviewed Aug 24, 2022

View reviewed changes

packages/memory-inspector/src/browser/editable-widget/memory-editable-table-widget.tsx Outdated Show resolved Hide resolved

martin-fleck-at approved these changes Aug 25, 2022

View reviewed changes

colin-grant-work and others added 4 commits August 25, 2022 08:15

Integrate Memory Inspector in Theia repository

50883fc

memory-inspector: cleanup and nls

2226536

Signed-off-by: vince-fugnitto <[email protected]>

MS comments

eb56b76

Update DebugProtocol and removed deprecated ReactNodeArray

e4dd0f4

colin-grant-work added 10 commits August 25, 2022 08:15

Update colors

3ecfa8f

private -> protected

1f3d3ec

extract various bits of rendering

1c47f28

remove many unnecessary lint disable comments

571116c

Show write errors

3b2e21d

Style error container

9c478ba

Style adjustments

9225e37

Style and more flexible widget creation

ae3076e

Yarn lock

373dc41

merge noselect declarations

64c8047

colin-grant-work force-pushed the feature/memory-inspector branch from b355a0d to 64c8047 Compare August 25, 2022 14:15

colin-grant-work merged commit 9331b9b into eclipse-theia:master Aug 25, 2022

colin-grant-work deleted the feature/memory-inspector branch August 25, 2022 14:38

colin-grant-work added this to the 1.29.0 milestone Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Memory Inspector in Theia repository #11394

Integrate Memory Inspector in Theia repository #11394

colin-grant-work commented Jul 7, 2022

JonasHelming commented Jul 8, 2022

colin-grant-work commented Jul 12, 2022

JonasHelming commented Jul 13, 2022

JonasHelming commented Jul 19, 2022

colin-grant-work commented Jul 19, 2022

vince-fugnitto left a comment

vince-fugnitto Jul 20, 2022

colin-grant-work Jul 20, 2022

colin-grant-work Aug 23, 2022

vince-fugnitto Jul 20, 2022

colin-grant-work Aug 23, 2022

msujew left a comment

msujew Aug 1, 2022

colin-grant-work Aug 4, 2022 •

edited

Loading

JonasHelming commented Aug 17, 2022

martin-fleck-at left a comment

martin-fleck-at Aug 19, 2022

colin-grant-work Aug 23, 2022

martin-fleck-at Aug 24, 2022 •

edited

Loading

martin-fleck-at Aug 19, 2022

colin-grant-work Aug 23, 2022

colin-grant-work Aug 23, 2022

martin-fleck-at Aug 24, 2022

martin-fleck-at commented Aug 23, 2022

colin-grant-work commented Aug 23, 2022

colin-grant-work commented Aug 23, 2022

martin-fleck-at left a comment

martin-fleck-at Aug 24, 2022

martin-fleck-at Aug 24, 2022 •

edited

Loading

colin-grant-work commented Aug 24, 2022

martin-fleck-at left a comment

martin-fleck-at commented Aug 25, 2022

Integrate Memory Inspector in Theia repository #11394

Integrate Memory Inspector in Theia repository #11394

Conversation

colin-grant-work commented Jul 7, 2022

What it does

How to test

Review checklist

Reminder for reviewers

JonasHelming commented Jul 8, 2022

colin-grant-work commented Jul 12, 2022

JonasHelming commented Jul 13, 2022

JonasHelming commented Jul 19, 2022

colin-grant-work commented Jul 19, 2022

vince-fugnitto left a 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

msujew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-grant-work Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

JonasHelming commented Aug 17, 2022

martin-fleck-at left a 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 Aug 24, 2022 • edited Loading

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 Aug 23, 2022

colin-grant-work commented Aug 23, 2022

colin-grant-work commented Aug 23, 2022

martin-fleck-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-fleck-at Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

colin-grant-work commented Aug 24, 2022

martin-fleck-at left a comment

Choose a reason for hiding this comment

martin-fleck-at commented Aug 25, 2022

colin-grant-work Aug 4, 2022 •

edited

Loading

martin-fleck-at Aug 24, 2022 •

edited

Loading

martin-fleck-at Aug 24, 2022 •

edited

Loading