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] Fix debugging blazor app after ts changes. #60873

Merged
merged 4 commits into from
Nov 1, 2021

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Oct 26, 2021

When running a blazor app, blazor that fills variable MONO.loaded_files. With the latest changes to typescript we were ignoring the content of variable that was set from blazor.

@thaystg thaystg requested a review from pavelsavara October 26, 2021 16:32
@thaystg thaystg marked this pull request as ready for review October 26, 2021 16:32
@thaystg thaystg requested a review from marek-safar as a code owner October 26, 2021 16:32
@ghost
Copy link

ghost commented Oct 26, 2021

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

Issue Details

When running a blazor app, blazor that fills variable MONO.loaded_files. With the latest changes to typescript we were ignoring the content of variable that was set from blazor.

Author: thaystg
Assignees: -
Labels:

area-Debugger-mono

Milestone: -

@thaystg thaystg requested review from radical and lewing October 26, 2021 17:03
@pavelsavara
Copy link
Member

Let's get rid of runtimeHelpers.loaded_files and use MONO.loaded_files everywhere.
I introduced runtimeHelpers.loaded_files during refactoring to decrease coupling, but it's not good change.

@lewing
Copy link
Member

lewing commented Oct 26, 2021

We can and should reexamine the file loading across everything at some point but using MONO.loaded_files makes sense for now.

@pavelsavara
Copy link
Member

pavelsavara commented Oct 26, 2021

  • please initialize MONO.loaded_files to [] in exports.ts
  • please also delete exports.ts line 79 loaded_files: runtimeHelpers.loaded_files,

ilonatommy added a commit to ilonatommy/runtime that referenced this pull request Oct 27, 2021
@radical radical added the arch-wasm WebAssembly architecture label Oct 27, 2021
@ghost
Copy link

ghost commented Oct 27, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

When running a blazor app, blazor that fills variable MONO.loaded_files. With the latest changes to typescript we were ignoring the content of variable that was set from blazor.

Author: thaystg
Assignees: -
Labels:

arch-wasm, area-Debugger-mono

Milestone: -

@radical
Copy link
Member

radical commented Oct 27, 2021

[13:45:15] fail: [out of order message from the browser]: http://127.0.0.1:43471/dotnet.js 0:38312 Uncaught TypeError: Cannot read property 'trim' of undefined

Seeing this in https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-60873-merge-5cfd3d45ea58416da1/Common.Tests/1/console.b40067f3.log?sv=2019-07-07&se=2021-11-16T13%3A26%3A33Z&sr=c&sp=rl&sig=MB0zNvUuSoiguFObH90liBpMLoC281X659YLQoAEqVE%3D, and other tests.

@thaystg
Copy link
Member Author

thaystg commented Oct 27, 2021

@lewing, can we merge? I think that the errors on CI are unrelated to my change.

@radical
Copy link
Member

radical commented Oct 27, 2021

RuntimeTests: console.error: error in mono_load_runtime_and_bcl_args: TypeError: Cannot read property 'trim' of undefined

Wasm.Build.Tests also

@thaystg
Copy link
Member Author

thaystg commented Oct 27, 2021

thanks @radical I'll investigate then!

@pavelsavara
Copy link
Member

pavelsavara commented Oct 27, 2021

trim would be in mono_method_resolve or bindings_lazy_init or maybe sourcePrefix.trim() in startup

@thaystg
Copy link
Member Author

thaystg commented Oct 28, 2021

@pavelsavara , I know that you told me to not import MONO from exports, but if I don't import I get this trim error and after this (if I workaround) I get another error when trying to use MONO.loaded_files, MONO is empty when I try to use on finalize_startup.

@lewing lewing requested a review from pavelsavara October 28, 2021 22:15
@pavelsavara
Copy link
Member

There are 2 objects perceived as MONO.

  • Legacy one comes from library-dotnet.js $MONO and surfaces as 'mono' in export_to_emscripten and also as MONO in ./modules and also as globalThis.MONO
  • The new export MONO in exports.ts, which members are merged into one above
  • There was third place runtimeHelpers where loaded_files lived before this PR.

I pushed change in which

  • we initilize loaded_files to [] and then it's merged into legacy 'mono' during export_to_emscripten
  • it's still gone from runtimeHelpers

Let's see if that works in CI.

…iles

- disable too agresive Terser minification
Co-authored-by: Thays <thaystg@gmail.com>
@pavelsavara pavelsavara force-pushed the thays_fix_blazor_after_ts_changes branch from 1be94aa to b25cc33 Compare October 29, 2021 14:34
@pavelsavara
Copy link
Member

For anybody interested, the undefined trim issue was that the Release minification was not playing well between Terser and emcc tools.

@thaystg thaystg added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 29, 2021
@pavelsavara
Copy link
Member

Release version of the runtime can't be debugged at the moment. It's also caused by the Terser clash.

@lewing
Copy link
Member

lewing commented Oct 29, 2021

The Release runtime is what we ship so what is the plan?

@thaystg
Copy link
Member Author

thaystg commented Oct 29, 2021

Pavel and I investigated it all the morning for me, and he will continue working on it on monday. We will find a solution.

pavelsavara and others added 2 commits October 29, 2021 21:27
…this case:

                            case "mono_wasm_fire_debugger_agent_message":
                            case "_mono_wasm_fire_debugger_agent_message":
@thaystg
Copy link
Member Author

thaystg commented Oct 29, 2021

I pushed another commit in this PR.
We also need to keep function names otherwise it will never enter in this case on MonoProxy.cs:

                            case "mono_wasm_fire_debugger_agent_message":
                            case "_mono_wasm_fire_debugger_agent_message":

Using this PR the debugger tests are working using runtime on release and also a Blazor app can be debugged using runtime on release.

@pavelsavara
Copy link
Member

pavelsavara commented Oct 31, 2021

I pushed another commit in this PR. We also need to keep function names otherwise it will never enter in this case on MonoProxy.cs:

                            case "mono_wasm_fire_debugger_agent_message":
                            case "_mono_wasm_fire_debugger_agent_message":

Using this PR the debugger tests are working using runtime on release and also a Blazor app can be debugged using runtime on release.

That should be instead exported under INTERNAL, which would stay un-mangled anyway. All other non API methods should be mangled

I was wrong, this is stack walk and so the function name needs to stay un-mangled.

@thaystg

@thaystg
Copy link
Member Author

thaystg commented Nov 1, 2021

As far as I understood you will try to find another solution for the other things, so I think you can do this change to internal together, once you do I can run the debugger-tests again and test the blazor app again.

@pavelsavara pavelsavara merged commit a5eacee into dotnet:main Nov 1, 2021
ilonatommy added a commit that referenced this pull request Nov 1, 2021
* Fix for #60340

* Fixes element access for a constant index.

* Undo changes made based on #60873, that were not intended to be commited.

* Removed whitespaces.

* Fixed element access by local variables, added tests for element access by object member variables.

* Element access by indexing with simple object members is fixed.

* Nested element access is fixed, e.g. a[a[a[0]]].

* Added tests for nested element access. Reverted an unintentional change in EvaluateSimpleMethodCallsCheckChangedValue test.

* Change test name to enable running test batch on calling ".EvaluateExpressionsWithElementAccess" prefix.

* Fix ElementAccessByMemberVariables that was failing after previous changes.

* Removed unused namespace import. Regexes are not needed in the new approach.

* Added implementatio of evaluation for multidimentional indexing.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Debugger-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants