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.

Sign up for GitHub

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

Jump to bottom

[wasm][debugger] Implement support to modify values. #49557

Merged
merged 13 commits into from
Apr 16, 2021
Merged

[wasm][debugger] Implement support to modify values. #49557

merged 13 commits into from
Apr 16, 2021

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Mar 12, 2021 •
edited
Loading

For simple types like: boolean, integer, long and char.
Tests implemented.

Fixes #42327.

@ghost
Copy link

ghost commented Mar 12, 2021

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

Issue Details

For simple types like: boolean, integer, long and char.
TODO: Write tests
Fixes #42327.

Author: thaystg
Assignees: -
Labels:

area-Debugger-mono
Milestone: -

@srxqds
Copy link
Contributor

srxqds commented Mar 13, 2021

For simple types like: boolean, integer, long and char.
TODO: Write tests
Fixes #42327.

will these Suppot on iOS when merged?

lewing
lewing reviewed Mar 13, 2021
View reviewed changes
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

Just did a quick scan. Will follow up in a bit.

src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
@thaystg @lewing
Apply suggestions from code review
c026bb5 
Co-authored-by: Larry Ewing <lewing@microsoft.com>
@thaystg
Copy link
Member Author

thaystg commented Mar 15, 2021

For simple types like: boolean, integer, long and char.
TODO: Write tests
Fixes #42327.

will these Suppot on iOS when merged?

This will be supported on wasm debugger. :)

@runfoapp runfoapp bot mentioned this pull request Mar 15, 2021
Open
@thaystg thaystg marked this pull request as ready for review March 15, 2021 19:35
@runfoapp runfoapp bot mentioned this pull request Mar 16, 2021
Closed
@radical radical added the arch-wasm WebAssembly architecture label Mar 16, 2021
@ghost
Copy link

ghost commented Mar 16, 2021

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

Issue Details

For simple types like: boolean, integer, long and char.
Tests implemented.

TODO: When we try to set a variable value of an attribute in an object it calls Runtime.callFunctionOn and we don't support it yet.
Fixes #42327.

Author: thaystg
Assignees: -
Labels:

arch-wasm, area-Debugger-mono
Milestone: -

@radical
Copy link
Member

radical commented Mar 16, 2021

TODO: When we try to set a variable value of an attribute in an object it calls Runtime.callFunctionOn and we don't support it yet.

We do support it - https://github.com/dotnet/runtime/blob/main/src/mono/wasm/debugger/BrowserDebugProxy/DevToolsHelper.cs#L207 . But maybe it is missing something that you need?

@thaystg
Copy link
Member Author

thaystg commented Mar 16, 2021

TODO: When we try to set a variable value of an attribute in an object it calls Runtime.callFunctionOn and we don't support it yet.

We do support it - https://github.com/dotnet/runtime/blob/main/src/mono/wasm/debugger/BrowserDebugProxy/DevToolsHelper.cs#L207 . But maybe it is missing something that you need?

I saw it but I didn't investigate why it was not working, it tries to run a function that receives the object as parameter and tries to run something like this: this[attribute] = newValue, do you think this should work?

@radical
Copy link
Member

radical commented Mar 16, 2021

See https://github.com/dotnet/runtime/blob/main/src/mono/wasm/runtime/library_mono.js#L1227-L1249
You will need to implement - https://github.com/dotnet/runtime/blob/main/src/mono/wasm/runtime/library_mono.js#L1237

We construct a proxy object, so you would need a proxy setter there that does the actual value setting for managed code.

@thaystg
Copy link
Member Author

thaystg commented Mar 17, 2021

@radical, done!

@thaystg
Fix compilation
ee7f036
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_fix_42327
b5346a6 
* upstream/main: (568 commits)
  [wasm] Set __DistroRid on Windows to browser-wasm (dotnet#50842)
  [wasm] Fix order of include paths, to have the obj dir first (dotnet#50303)
  [wasm] Fix debug build of AOT cross compiler (dotnet#50418)
  Fix outdated comment (dotnet#50834)
  [wasm][tests] Add properties to allow passing args to xharness (dotnet#50678)
  Vectorized common String.Split() paths (dotnet#38001)
  Fix binplacing symbol files. (dotnet#50819)
  Move type check to after the null ref branch in out marshalling of blittable classes. (dotnet#50735)
  Remove extraneous CMake version requirement. (dotnet#50805)
  [wasm] Remove unncessary condition for EMSDK (dotnet#50810)
  Add loop alignment stats to JitLogCsv (dotnet#50624)
  Resolve ILLink warnings in System.Diagnostics.DiagnosticSource (dotnet#50265)
  Avoid unnecessary closures/delegates in Process (dotnet#50496)
  Fix for field layout verification across version bubble boundary (dotnet#50364)
  JIT: Enable CSE for VectorX.Create (dotnet#50644)
  [main] Update dependencies from mono/linker (dotnet#50779)
  [mono] More domain cleanup (dotnet#50771)
  Race condition in Mock reference tracker runtime with GC. (dotnet#50804)
  Remove IAssemblyName (and various fusion remnants) (dotnet#50755)
  Disable failing test for GCStress. (dotnet#50828)
  ...
This was referenced Apr 8, 2021
Closed
Closed
Closed
radical
radical requested changes Apr 15, 2021
View reviewed changes
src/mono/mono/mini/mini-wasm-debugger.c Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/mono/mini/debugger-engine.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
@radical
Copy link
Member

radical commented Apr 15, 2021

@thaystg thank you for making all the changes, and adding the tests!

@radical
Copy link
Member

radical commented Apr 15, 2021

What happens if you try to set char var with:

  1. the value abc? Shouldn't that fail?
  2. What if abc is another variable? Maybe for char we should require 'a'? What do VS, and chrome do?

@thaystg
Copy link
Member Author

thaystg commented Apr 15, 2021

What happens if you try to set char var with:

  1. the value abc? Shouldn't that fail?
  2. What if abc is another variable? Maybe for char we should require 'a'? What do VS, and chrome do?
  1. Should fail, fixed.
  2. It's required 'a' on VS.

On Js even if I insert a value like this: 'a' when I receive on debug proxy it's "a" so I'm just checking if the strlen is 1 otherwise I will return false.

radical
radical requested changes Apr 15, 2021
View reviewed changes
Copy link
Member

@radical radical left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the changes. Just a few more comments, we are almost done!

Also, I ran all the tests locally, and they pass:

Failed: 0, Passed: 491, Skipped: 0, Total: 491

src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-wasm-debugger.c Show resolved Hide resolved
radical
radical approved these changes Apr 15, 2021
View reviewed changes
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Thanks for your patience! LGTM 👍

@thaystg thaystg merged commit fc53ee7 into dotnet:main Apr 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 16, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lewing lewing lewing left review comments

@pavelsavara pavelsavara pavelsavara left review comments

@radical radical radical approved these changes

@CoffeeFlux CoffeeFlux Awaiting requested review from CoffeeFlux

@lambdageek lambdageek Awaiting requested review from lambdageek lambdageek is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
arch-wasm WebAssembly architecture area-Debugger-mono
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Web Assembly Debugging - Modify values
6 participants
@thaystg @srxqds @radical @lewing @pavelsavara @karelz