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

[wasm] [debugger] First version of multithreaded debugging #74820

Merged
merged 54 commits into from
Feb 1, 2023

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Aug 30, 2022

cb1fd258-20f9-4648-89eb-78ddbd59a7d9.mp4

Build command:
.\build.cmd mono+libs -os browser /p:MonoWasmBuildVariant=multithread

Run debugger-tests command:
.\dotnet.cmd test src/mono/wasm/debugger/DebuggerTestSuite --filter DebuggerTests.MiscTests.TestDebugUsingMultiThreadedRuntime -e RuntimeConfiguration=Debug -e Configuration=Debug -e DebuggerHost=chrome -e WASM_TESTS_USING_VARIANT=multithreaded -e WasmEnableThreads=true

@ghost
Copy link

ghost commented Aug 30, 2022

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Issue Details
cb1fd258-20f9-4648-89eb-78ddbd59a7d9.mp4
Author: thaystg
Assignees: -
Labels:

area-Debugger-mono

Milestone: -

@thaystg
Copy link
Member Author

thaystg commented Aug 30, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@thaystg thaystg marked this pull request as ready for review September 1, 2022 16:53
@radical
Copy link
Member

radical commented Sep 1, 2022

The Wasm.Build.Tests failures are #74944 .

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

C code and TS code lgtm.

I'd like someone more familiar with the debug proxy to review that part

@radical
Copy link
Member

radical commented Sep 27, 2022

needs to merge main

src/mono/wasm/debugger/BrowserDebugProxy/DevToolsHelper.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/DevToolsHelper.cs Outdated Show resolved Hide resolved
@thaystg
Copy link
Member Author

thaystg commented Jan 30, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@thaystg
Copy link
Member Author

thaystg commented Jan 30, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@thaystg
Copy link
Member Author

thaystg commented Jan 30, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@thaystg
Copy link
Member Author

thaystg commented Jan 30, 2023

/azp run runtime-wasm-non-libtests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@thaystg
Copy link
Member Author

thaystg commented Jan 31, 2023

/azp run runtime-wasm-non-libtests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@thaystg
Copy link
Member Author

thaystg commented Jan 31, 2023

@radical @lambdageek Could you please do a final review so I can merge?

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

C and TypeScript lgtm. I tried to follow BrowserDebugProxy, but I'm not sure if I completely understand it

#
# Evaluate paths
#
- template: /eng/pipelines/common/evaluate-default-paths.yml
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now. But in a follow up we should change this to use these from runtime-extra-platforms-wasm.yml with a new debuggerTestsOnly parameter.

notifications.Remove(what, out _);
return tcs.Task;
}

throw new Exception($"Invalid internal state, waiting for {what} while another wait is already setup");
}
else if (nextNotifications.TryDequeue(out var notification))
Copy link
Member

Choose a reason for hiding this comment

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

What if the next notification in the queue is for a different string what?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll push some changes and it will be throwing an exception if it's a different string what

Comment on lines +217 to +221
await Client.SendCommand(sessionIdNewTarget, "Profiler.enable", null, token);
await Client.SendCommand(sessionIdNewTarget, "Runtime.enable", null, token);
await Client.SendCommand(sessionIdNewTarget, "Debugger.enable", null, token);
await Client.SendCommand(sessionIdNewTarget, "Runtime.runIfWaitingForDebugger", null, token);
await Client.SendCommand(sessionIdNewTarget, "Debugger.setAsyncCallStackDepth", JObject.FromObject(new { maxDepth = 32}), token);
Copy link
Member

Choose a reason for hiding this comment

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

For a follow up PR - the commands here, and in DebuggerTestBase.InitializeAsync should use the same base set of commands. And each case can add any additional ones as needed. Like the ones here seem to be the same.

@thaystg
Copy link
Member Author

thaystg commented Feb 1, 2023

/azp run runtime-wasm-non-libtests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@thaystg
Copy link
Member Author

thaystg commented Feb 1, 2023

/azp run runtime-wasm-non-libtests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@thaystg thaystg merged commit ac18cc9 into dotnet:main Feb 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants