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

Change WASM direct heap access to use helper functions #61355

Merged
merged 6 commits into from
Nov 10, 2021

Conversation

kg
Copy link
Contributor

@kg kg commented Nov 9, 2021

According to the ECMAScript spec, the left-hand side of an assignment is evaluated before the right-hand side. This means that for the following statement:

Module.HEAPU32[offset] = get_value();

The write will be discarded if the heap grows during the call to get_value. If the offset itself is a property with an accessor or the return value of a function call the problem applies there too. This is because heap growth requires creating a new set of typed arrays pointing at the new larger heap buffer, and the old arrays become 'detached', and all reads/writes to a detached buffer will silently fail.

Since almost any JS expression can secretly be invoking a property getter, the best solution for this is to do roughly what blazor does, and perform memory accesses through functions so that we can be certain the offset and value will be evaluated before the target is evaluated. With this approach, the above becomes:

INTERNAL.setU32(offset, get_value());

@kg kg added this to the 7.0.0 milestone Nov 9, 2021
@kg kg requested review from lewing and pavelsavara November 9, 2021 07:49
@kg kg requested a review from marek-safar as a code owner November 9, 2021 07:49
@ghost
Copy link

ghost commented Nov 9, 2021

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

Issue Details

According to the ECMAScript spec, the left-hand side of an assignment is evaluated before the right-hand side. This means that for the following statement:

Module.HEAPU32[offset] = get_value();

The write will be discarded if the heap grows during the call to get_value. If the offset itself is a property with an accessor or the return value of a function call the problem applies there too.

Since almost any JS expression can secretly be invoking a property getter, the best solution for this is to do roughly what blazor does, and perform memory accesses through functions so that we can be certain the offset and value will be evaluated before the target is evaluated. With this approach, the above becomes:

INTERNAL.setU32(offset, get_value());

Author: kg
Assignees: -
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 7.0.0

…not backwards-compatible to change it or use BigInt
@kg
Copy link
Contributor Author

kg commented Nov 9, 2021

I spent a bit trying to use BigInt to properly implement I64/U64 because Emscripten's setvalue/getvalue for them turn out to be broken, but it seems to not be possible to do that without breaking existing code.

@pavelsavara pavelsavara self-requested a review November 10, 2021 08:21
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.

2 participants