Skip to content

[Wasm64] Fix creation of 64-bit memories from JS #20111

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

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Aug 23, 2023

This enables quite a few tests that were previously disabled.

The extra check for navigator.userAgent is because node added support for the navigator object: nodejs/node#47769.

This change is also part of #19959, which could land instead.

@sbc100 sbc100 requested review from kripken and dschuff August 23, 2023 22:38
@sbc100 sbc100 enabled auto-merge (squash) August 23, 2023 23:00
@dschuff
Copy link
Member

dschuff commented Aug 23, 2023

This also means that once this lands, the memory64 feature will only work with Chrome builds that have the new JS API support, right?

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 23, 2023

This also means that once this lands, the memory64 feature will only work with Chrome builds that have the new JS API support, right?

No.. extra attributes seem to just get ignored so it should have no effect. Even it if did it would only effect those who use IMPORTED_MEMORY (directly or indirectly).

@dschuff
Copy link
Member

dschuff commented Aug 23, 2023

So IIUC the thing that this actually makes work is the use cases for IMPORTED_MEMORY (of which the most important is threads... and #19959 is the rest of the threads fixes).

Copy link
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

This is good to split out into a smaller change though.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 23, 2023

So IIUC the thing that this actually makes work is the use cases for IMPORTED_MEMORY (of which the most important is threads... and #19959 is the rest of the threads fixes).

Yes that sounds right. Hopefully @dakenf is OK with this change landing even though it overlaps with #19959?

This enables quite a few tests that were previously disabled.

The extra check for `navigator.userAgent` is because node added support
for the navigator object: nodejs/node#47769.

This change is also part of #19959, which could land instead.
@sbc100 sbc100 force-pushed the wasm64_4gb_external_memory branch from 020b0f4 to 5190960 Compare August 23, 2023 23:55
@dakenf
Copy link
Contributor

dakenf commented Aug 24, 2023

Sure, I'll need to update that one anyway because some tests fail

@sbc100 sbc100 merged commit 0c35c4a into main Aug 24, 2023
@sbc100 sbc100 deleted the wasm64_4gb_external_memory branch August 24, 2023 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants