-
Notifications
You must be signed in to change notification settings - Fork 27
Memory Inspector: UI and UX Updates and New Features #119
Memory Inspector: UI and UX Updates and New Features #119
Conversation
75d42ca
to
54288bf
Compare
address: number, | ||
} | ||
import { injectable, inject } from 'inversify'; | ||
import { hexStrToUnsignedLong } from '../../common/util'; | ||
|
||
export const MemoryProvider = Symbol('MemoryProvider'); |
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.
@thegecko, this is the interface between the debugger backend and the frontend. The current implementation knows it's interacting with GDB because the CDT-GDB adapter doesn't implement memory reading and writing as prescribed by the debug adapter protocol, but it should be relatively straightforward to swap it out.
It might be desirable to implement something like a MemoryProviderService
that gathers MemoryProvider
implementations and matches a given debug session with the memory provider designed for it. Let us know if that sounds useful, and we'll be happy to add it.
This looks great! Are there plans to merge this PR? This would make it so much easier to adopt the great work you did! |
@JonasHelming, we will plan on merging next week or the following. The one outstanding desideratum is that the memory reading be reimplemented to use the debug adapter protocol's prescribed method, rather than a custom request. That functionality has been implemented for the CDT-GDB adapter here, and once the changes have been confirmed to work for this extension, we will rewrite the code in this PR to use the new method. |
// This functionality is not presently available from the CDT-GDB adapter | ||
// async writeMemory(address: string, content: string): Promise<void> { | ||
// const { currentSession } = this.debugSessionManager; | ||
// if (!currentSession) { | ||
// throw new Error('No active debug session.'); | ||
// } | ||
|
||
// await currentSession.sendCustomRequest('cdt-gdb-adapter/MemoryWrite', { address, content }); | ||
// } |
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.
The write request is now in the DAP, so it should be possible to get it into the adapter.
@colin-grant-work when do you expect to merge? I don not intend to push, it is just for planning our adoption use case. |
@JonasHelming, at the moment, I am waiting on a new release of CDT-GDB to convert the calls to read memory to DAP-compatible requests. It seems the release hasn't yet made it all the way to the VSCode plugin side, which is what this extension consumes. If you'd like to see it merged earlier, I could write a DAP-compatible memory provider, but leave it unbound for now until a DAP-compatible memory request is available from CDT-GDB. |
@colin-grant-work I wanted to mention that this is our own fork of this repo, that @paul-marechal arranged and periodically makes new releases-for. A new version there will not happen on its own, once the new version of the DA is published. Please let Paul know when the start are aligned. |
@paul-marechal Considering that you have since become a committer on the CDT project, maybe we can think about making releases upstream, instead of on our fork. I see there are still no release tags on the upstream repo, so I guess no official releases yet? Alternatively, as I recall, the way we currently consume the DA from the |
https://open-vsx.org/extension/eclipse-cdt/cdt-gdb-vscode ?? |
Good catch - the extension is published on |
At the moment it actually downloads from the GitHub releases: theia-cpp-extensions/packages/cpp-debug/package.json Lines 56 to 58 in 0a5deb4
|
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.
@colin-grant-work I've added one small suggestion before this is merged. Can you please take a look at it?
|
||
dispose(): void { | ||
this.toDispose.dispose(); | ||
} |
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.
It looks like the super
call is missing here and it's not possible to properly dispose of this widget. Can we add this?
} | |
this.super.dispose(); | |
} |
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.
Good catch. I've added a call to super.dispose()
to the method.
54288bf
to
00b6f5d
Compare
Will this be merged soon? I do not want to push, but I need to adapt some project planning based on this, so it would be useful for me to have an ETA. Thank you in advance! :-) |
@JonasHelming, as of yesterday, a new release of CDT-GDB-VSCode is available on Open VSX, so I can now make the changes to use the DAP and then merge this PR. Apologies for the delays! |
@JonasHelming, I have made the necessary adjustments to use the DAP - just need to bring it up to date with current Theia and it will be good to go. I plan to take care of that tomorrow morning. |
sounds great, thank you very much for the update! |
Signed-off-by: Colin Grant <colin.grant@ericsson.com> Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com> Co-authored-by: Colin Grant <colin.grant@ericsson.com> Co-authored-by: Kenneth Marut <kenneth.marut@ericsson.com>
In theory, we could have problems reading addresses as JS numbers, since JS numbers don't have full 64 bits precision. This patch makes use of the 'long' library, which stores 64-bit numbers as two 32-bit halves. Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com> Signed-off-by: Simon Marchi <simon.marchi@ericsson.com> Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
This patch makes it so we hightlight the local variables of the current frame in the memory view. The idea is simple, we get the size and address of all locals of the current frame through GDB (sizeof(foo) and &foo). If the bytes backing these variables are in the scope of the memory view, we highlight them with a different color for each variable. Signed-off-by: Simon Marchi <simon.marchi@ericsson.com> Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com> Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
Signed-off-by: Colin Grant <colin.grant@ericsson.com> Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com> Signed-off-by: Tomas Sisohore <tomas.sisohore@ericsson.com> Co-authored-by: Colin Grant <colin.grant@ericsson.com> Co-authored-by: Kenneth Marut <kenneth.marut@ericsson.com> Co-authored-by: Tomas Sisohore <tomas.sisohore@ericsson.com>
1d38fb8
to
fda9995
Compare
@JonasHelming, @federicobozzini, @thegecko, @kenneth-marut-work, @paul-marechal, I have pushed up some significant changes to this PR.
If you're willing, please give things a run through to ensure that everything is working to your satisfaction, and if it is, I'll merge this ASAP. |
e78acbe
to
7923426
Compare
Signed-off-by: Colin Grant <colin.grant@ericsson.com>
7923426
to
bb9af17
Compare
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.
@colin-grant-work I've added a couple of comments again.
packages/cpp-debug/src/browser/memory-provider/memory-provider.ts
Outdated
Show resolved
Hide resolved
packages/cpp-debug/src/browser/memory-provider/memory-provider.ts
Outdated
Show resolved
Hide resolved
51c8b50
to
0234329
Compare
@federicobozzini, I've pushed up a base-64 patch on this PR, but I propose that we merge it without those changes, since GDB is currently the main (only?) adapter that we here have an interest in, and it's sending its results in hex. I'll make a separate PR for the base 64 changes, and then there's less urgency on the long pipeline from CDT-GDB--> CDT-GDB-VSCode --> here. Does that sound reasonable? cc: @JonasHelming, @thegecko |
0234329
to
8ff9290
Compare
packages/cpp-debug/src/browser/memory-provider/memory-provider.ts
Outdated
Show resolved
Hide resolved
packages/cpp-debug/src/browser/memory-provider/memory-provider.ts
Outdated
Show resolved
Hide resolved
It sounds like a sensible choice to me. I'm OK with it. Also feel free to ignore the other code review if the last commit is not going to be included. |
@federicobozzini, at @paul-marechal's urging, I decided to implement a different solution to the problem along the lines of this idea from the PR description:
I have left the DAP-compliant, base-64-expecting MemoryProvider as the default (and made the changes you suggested, swapping |
Unless there are any objections, I'll merge this on Monday. |
@paul-marechal, would you mind approving this when you have a chance? Thanks! |
3ae5aa4
to
d419934
Compare
d419934
to
bb9f955
Compare
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.
I actually can't test it locally as I don't have the right environment.
Looking at the code it looks good.
Looking at the animated captures, the new widgets looks much better than what we have.
@JonasHelming @thegecko @federicobozzini we plan on deprecating @theia/cpp-debug
and rename this package something like @theia/debug-memory-inspector
. The reasoning is that cpp-debug
used to be about providing C/C++ debugging capabilities, but this is not the case anymore. We'll merge this PR for now, but keep in mind that we need a new package for it with a new more appropriate name, feel free to give suggestions.
What it does
This PR introduces substantial changes to the UI of the memory viewer widget, building on Simon Marchi's work on colorizing variables. It adds useful functionality for inspecting memory during debugging including: dynamic updating of memory, multiple memory views, view comparison (diffing), register viewing.
Key features:
Multiple Views
We have added the ability to open multiple memory views. This allows users to compare different regions of memory or to examine the same memory region of memory at different points in time.
multi-view.mp4
Diff View
In addition to the standard memory viewer, we have added a diffing view that allows users to compare the content of different memory views in-line.
before-after.mp4
Dynamic Updating and Locking
Memory views will now update as users step through code, and changes to the visible memory region will be highlighted. If users would like to opt out of automatic updates - for example to preserve a memory state for comparison to a later state - they can 'lock' the view by clicking a lock icon.
edit-title-and-lock.mp4
highlighting-changes-ascii.mp4
Entry points
On the master branch, in order to examine a particular memory region, users must manually open the memory view and configure its view parameters when stopped at a breakpoint. This PR adds context-menu commands to open a memory view or register view at the location of a variable visible in the Debug Variables widget while also allowing manually address specification.
right-click-entry.mp4
manual-entry-point.mp4
Register View
The Debug Adapter Protocol supports the reporting of register contents as a variable scope, although the CDT-GDB adapter doesn't currently provide registers. We have added a memory view dedicated to showing register contents, since the layout is quite different from the standard memory view.
Setup
git fetch origin pull/119/head:memory-inspector && git checkout memory-inspector
)npx yarn
)electron
, you will need to runnpx yarn rebuild:electron
.npx yarn start:browser
ornpx yarn start:electron
)<repository>/examples/cpp-debug-workspace
)compile example
)a.cpp
and add a break point somewhere inmain
.test-cpp
- it should already be selected in the dropdown.How to test
examples/cpp-debug-workspace
workspace and run the commandTask: Run
and run the compilation task.a.cpp
launch.json
&<what you clicked>
and it should be populated with colorful memory.Toggle Memory View
&<some variable>
to get memory at that location.hex
icon in the Memory Widget Tabbar)Settings
cog wheel at the topenter
should save,escape
should cancel the edit.Go
button in the comparison area.Additional features to observe:
To think about:
Review checklist
Reminder for reviewers