-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[wasm][debugger] Reuse debugger-agent on wasm debugger #52300
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] Reuse debugger-agent on wasm debugger #52300
Conversation
…ot of code that does the same thing on mini-wasm-debugger.
|
Tagging subscribers to this area: @thaystg Issue DetailsPROTOTYPE!!!
|
lewing
left a comment
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.
Sharing this code will be a great step forward and the prototype is coming along nicely. Nice work.
make -C src/mono/wasm/ run-debugger-tests TEST_FILTER=DebuggerTests.SteppingTests.InspectValueTypeMethodArgsWhileStepping is working without use_cfo.
Failed: 271, Passed: 220
radical
left a comment
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.
Just posting some comments on the bits that I was able to look at (very little!).
| for (i = strlen (lookup_name) - 1; i >= 0; --i) { | ||
| if (lookup_name [i] == '.') { | ||
| lookup_name [i] = 0; | ||
| break; | ||
| } | ||
| } |
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.
can we use strrchr here?
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 copied this code from mini-wasm-debugger. Probably yes.
src/mono/mono/mini/debugger-agent.c
Outdated
| buffer_add_int(buf, -1); | ||
| break; | ||
| } | ||
| mono_assembly_name_free_internal (aname); |
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.
this should be done in the error cases too.
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.
fixed.
| MonoTypeNameFormat format = MONO_TYPE_NAME_FORMAT_FULL_NAME; | ||
| if (CHECK_PROTOCOL_VERSION(2, 61)) | ||
| format = (MonoTypeNameFormat) decode_int (p, &p, end); |
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.
Would it make sense to make this a macro, or function, so the protocol version stuff doesn't have to be here?
src/mono/mono/mini/debugger-agent.c
Outdated
| } | ||
|
|
||
| ErrorCode | ||
| mono_process_dbg_packet (int id, CommandSet command_set, int command, gboolean *no_reply, guint8 *p, guint8 *end, Buffer *buf) |
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.
Maybe rename p to something descriptive
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.
done.
| /* Enums are sent as a normal vtype */ | ||
| if (is_enum) | ||
| return ERR_NOT_IMPLEMENTED; | ||
| if (CHECK_PROTOCOL_VERSION(2, 61)) | ||
| decode_byte (buf, &buf, limit); |
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.
What happens in versions other than 2.61 now?
Implementing handling error on debugger-agent.
Failed: 248, Passed: 243
Failed: 243, Passed: 248
Failed: 226, Passed: 265
Failed: 192, Passed: 299
Failed: 184, Passed: 307
…oints tests are passing. Failed: 172, Passed: 319
Failed: 156, Passed: 335
Failed: 148, Passed: 343
Failed: 99, Passed: 392
Failed: 53, Passed: 438
Failed: 31, Passed: 460
Failed: 30, Passed: 461
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.
This is looking great but I have some style questions while I keep reviewing. There are a few small formatting things to fix as well but I stopped pointing them out individually.
Co-authored-by: Larry Ewing <lewing@microsoft.com>
lewing
left a comment
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.
More style thoughts, but feel free to ignore
src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Larry Ewing <lewing@microsoft.com>
Co-authored-by: Larry Ewing <lewing@microsoft.com>
Co-authored-by: Larry Ewing <lewing@microsoft.com>
|
The amount of this error in the Helix logs is worrying: |
|
Thanks @filipnavara I'll take a look! :) |
lewing
left a comment
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.
Let's do a lot of manual testing now.
cc @radical @pavelsavara @radekdoulik @kg @Daniel-Genkin-MS
|
I just did some testing on Windows and found the following behaviours. Although I'm not sure if they are directly applicable to this PR. Although other than that, this works really well and is really cool!!
|
|
@Daniel-Genkin thank you for the testing
This needs to have a test added and needs to be fixed. I'm not sure I consider it blocking for landing this pr, especially if it only diagnostics and doesn't surface to the UI.
This sounds like #54040 |
This also happens in the version without my changes, we need to investigate if debugger proxy is returning this error or the browser is returning this error. |
This one I think that always happens without my changes, but probably we should also try to fix. |
|
@Daniel-Genkin thanks for testing!!! :) |
|
Please, if anyone find anything, please comment here and I will fix and change what is suggested in another PR. |
This will remove a lot of code that does the same thing on mini-wasm-debugger.
The code for debugger before this PR was shared between mini-wasm-debugger, library_mono.js and the BrowserDebugProxy. With this new approach the important code for the debugger are mainly on debugger-agent and BrowserDebugProxy.
This PR affects the size of dotnet.wasm and dotnet.js as is described below.