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] improve memory access and marshaling range checks #64845

Merged
merged 13 commits into from
May 20, 2022
Merged

[wasm] improve memory access and marshaling range checks #64845

merged 13 commits into from
May 20, 2022

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Feb 5, 2022 •
edited
Loading

  • capture updateGlobalBufferAndViews
  • remove setI64, getI64
  • add setI52, getI52, getU52, setU52
  • add getI64Big, setI64Big
  • change bind_static_method to use int52 with l signature.
  • inlined JS asserts
  • fixed existing binding for unsigned types
  • implemented range validations for all memory access and interop

@pavelsavara pavelsavara added this to the 7.0.0 milestone Feb 5, 2022
@ghost
Copy link

ghost commented Feb 5, 2022

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

Issue Details
  • capture updateGlobalBufferAndViews
  • improve setI64, getI64
  • add getI64Big, setI64Big
  • fix StringDecoder root and hashtable access
Author: pavelsavara
Assignees: -
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript
Milestone: 7.0.0

@ghost ghost assigned pavelsavara Feb 5, 2022
@pavelsavara pavelsavara requested review from kg and lewing February 5, 2022 11:38
@pavelsavara
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

kg
kg reviewed Feb 5, 2022
View reviewed changes
src/mono/wasm/runtime/memory.ts Outdated Show resolved Hide resolved
@pavelsavara
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

kg
kg reviewed Feb 6, 2022
View reviewed changes
src/mono/wasm/runtime/memory.ts Outdated Show resolved Hide resolved
@lewing
Copy link
Member

lewing commented Mar 25, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara marked this pull request as draft March 31, 2022 17:10
@ghost ghost added the no-recent-activity label Apr 15, 2022
@ghost
Copy link

ghost commented Apr 15, 2022

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented Apr 30, 2022

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

@ghost ghost closed this Apr 30, 2022
@pavelsavara pavelsavara reopened this May 12, 2022
@ghost ghost removed the no-recent-activity label May 12, 2022
@pavelsavara pavelsavara mentioned this pull request May 12, 2022
Merged
@pavelsavara
* new memory accessors setI52, setI64Big, getI52, getI64Big
d119e50 
* removed support for long form automatic marshaler. It has impact to mono_bind_static_method
@kg
Copy link
Contributor

kg commented May 13, 2022

Big fan of the I52 / I64Big split, good idea

@pavelsavara
Copy link
Member Author

pavelsavara commented May 13, 2022 •
edited
Loading

I implemented feedback from @kg about availability of BigInt and speed of BigInt allocation vs arithmetic.

I also had discussion with @lambdageek about 52 integral bits of Number vs Int64.
There are use-cases where 52 bits are useful, for example for Date.value.

We concluded that emscripten's silent usage of only lower 32 bit is bad.

I also realized that silent usage of lower 52bits could be similarly bad.
So I removed support for marshaling long signature form bind_static_method, which did silent 32bits till now.

This is breaking change, but failing fast should be better than silent data corruption on runtime.

@pavelsavara
Copy link
Member Author

pavelsavara commented May 19, 2022 •
edited
Loading

I realized that all the other setXXX are also silently trimming bits and not throwing on overflow.
It's like option C) above.
Should I introduce range check to all of them ?

Or remove the range check from Int52 ?
C1) Store it as the memory bits were double ?
C2) Mask any exponent bits and store it as is ?
F) round it to next int, convert NaN to 0, Number.POSITIVE_INFINITY to long.MaxValue ?

@pavelsavara
Copy link
Member Author

This looks great, but we should make sure the C# -> JS path handles longs correctly (I think we just need to update the unbox primitive stuff to intentionally work in terms of I52 and throw an exception when unboxing if the long is too large)

I think we never had Number -> long conversion on mono_bind_static_method. I added test for it.

@pavelsavara pavelsavara changed the title [wasm] improve I64 memory access [wasm] improve memory access and marshaling range checks May 19, 2022
@pavelsavara
- fixed marshaling of uint32, uint16 and byte to be unsigned
7982e54 
- implemented range check on all set memory operations
- implemented also uint52
- differentiated bool marshaling because it shoud have different validation and message
@pavelsavara
inlined asserts
3e20335
@pavelsavara
Copy link
Member Author

Should I introduce range check to all of them ?

We agreed with @kg to range-check them all.
That forced me to fix broken marshaling of unsigned types and bool.
I also implemented uint52.
And I implemented assert inlining, to make sure it doesn't allocate closure.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara requested a review from kg May 19, 2022 19:18
@pavelsavara pavelsavara requested a review from kg May 20, 2022 09:02
@kg kg merged commit e7886b2 into dotnet:main May 20, 2022
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@SamMonoRT SamMonoRT mentioned this pull request Jun 7, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 22, 2022
@pavelsavara pavelsavara deleted the wasm_resize_and_int64 branch July 14, 2022 20:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kg kg kg approved these changes

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

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

@lambdageek lambdageek Awaiting requested review from lambdageek

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@pavelsavara @lewing @kg