Skip to content
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

Add Debug Context Selector #1

Closed
wants to merge 4 commits into from
Closed

Add Debug Context Selector #1

wants to merge 4 commits into from

Conversation

WyoTwT
Copy link
Owner

@WyoTwT WyoTwT commented Apr 5, 2024

What it does

This PR addresses Issue 67.
In a system with multiple children such as the cdt-gdb-amalgamator, the Memory Inspector needs a way to select the correct context (as the cdt-gdb-vscode Memory Browser does).

This PR has a dependency on cdt-gdb-amalgamator PR 20 which puts the assignment of the Child IDs in the cdt-gdb-amalgamator instead of the Memory Inspector and also uses the readMemory since the data returned is in the correct format for Memory Inspector.

How to test

Run this branch of the vscode-memory-inspector with the mainline cdt-gdb-vscode and cdt-gdb-adapter along with the modified cdt-gdb-amalgamator in PR 20](eclipse-cdt-cloud/cdt-amalgamator#20).

Start the debugger as a Desktop Extension. In the newly launched VSCode, open the cdt-amalgamator/sampleWorkspace folder and place a breakpoint at the first instruction in empty1.c's main and empty2.c's main. Start debugging with Amalgamator Example and you should run to the breakpoints. Open a new Memory Inspector with the Command Palette's Memory: Show Memory Inspector to open a new view. The dropdown should list both proc1 and proc2 listed in the Call Stack.

Review checklist

Reminder for reviewers

src/common/debug-requests.ts Outdated Show resolved Hide resolved
src/common/debug-requests.ts Outdated Show resolved Hide resolved
src/common/messaging.ts Outdated Show resolved Hide resolved
src/common/messaging.ts Outdated Show resolved Hide resolved
src/plugin/memory-provider.ts Outdated Show resolved Hide resolved
src/webview/components/memory-widget.tsx Outdated Show resolved Hide resolved
@WyoTwT WyoTwT force-pushed the tt_add_child_selector branch from eb25f0b to c472173 Compare April 11, 2024 12:50
Copy link
Owner Author

@WyoTwT WyoTwT Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel comfortable with the undefined in the readMemory (line 73 below) but it felt like I was changing too many functions which is what these more surgical changes were supposed to avoid (instead of adding Context to readArguments).

@WyoTwT WyoTwT force-pushed the tt_add_child_selector branch from 1cc2bce to dc5df23 Compare April 30, 2024 21:30
Since the ReadMemory/WriteMemory may be dependent upon the DAPs, move
into the adapter-capabilities.ts file.

Signed-off-by: Thor Thayer <[email protected]>
@WyoTwT WyoTwT force-pushed the tt_add_child_selector branch from dc5df23 to 25ce242 Compare April 30, 2024 21:55
Comment on lines 141 to 143
readMemory?(session: vscode.DebugSession, params: ReadMemoryArguments, context: Context): Promise<ReadMemoryResult>;

writeMemory?(session: vscode.DebugSession, params: WriteMemoryArguments, context: Context): Promise<WriteMemoryResult>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other cases, the context is optional; why is it required here?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops. Good catch. Thanks!


protected showContexts = () => {
if (this.props.contexts.length === 0) {
return false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined would be more natural here.

WyoTwT added 3 commits May 1, 2024 20:18
Populate the Context Dropdown from the different debug Contexts
if available so different DAPs can be addressed.
Add an optional Context to queries so the query can be directed
to the appropriate handler.
Add support for cdt-amalgamator.

Signed-off-by: Thor Thayer <[email protected]>
Match current context id with one of the elements of the
Contexts array.

Signed-off-by: Thor Thayer <[email protected]>
Add new conditional for context menu showing Memory Inspector for a
variable because some DAPs may not support it.

Signed-off-by: Thor Thayer <[email protected]>
@WyoTwT WyoTwT force-pushed the tt_add_child_selector branch from 25ce242 to a80eb95 Compare May 1, 2024 18:20
@WyoTwT WyoTwT changed the title Tt add child selector Add Debug Context Selector May 1, 2024
@WyoTwT WyoTwT marked this pull request as ready for review May 1, 2024 21:33
@WyoTwT
Copy link
Owner Author

WyoTwT commented May 2, 2024

Since this is a local draft PR, I'm closing it.

@WyoTwT WyoTwT closed this May 2, 2024
@WyoTwT
Copy link
Owner Author

WyoTwT commented May 2, 2024

Since this is a local draft PR, I'm closing it. See upstream PR:
eclipse-cdt-cloud#133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants