Skip to content

Conversation

Mintyboi
Copy link
Contributor

When accessing functions from the splitted module, there's an error because the wasmtable is using table64. So we would have to access it with a BigInt. This has been updated in other part of the codebase, but we missed this file.

We can use the `toIndexType` utility function to access the wasm table
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I guess this means we don't have any test coverage for wasm64 + module splitting?

Can you try adding @also_with_wasm64 to test_split_module? Can you confirm it doesn't work without this fix?

Otherwise lgtm!

@Mintyboi
Copy link
Contributor Author

I guess this means we don't have any test coverage for wasm64 + module splitting?

This seems to be the case.
Else it would have failed without this fix as well WebAssembly/binaryen#7904

@sbc100
Copy link
Collaborator

sbc100 commented Sep 30, 2025

lgtm with the test update

@sbc100
Copy link
Collaborator

sbc100 commented Oct 7, 2025

Can you update the test ?

@Mintyboi
Copy link
Contributor Author

Mintyboi commented Oct 7, 2025

Can you update the test ?

Sure, do you mean increasing the coverage of the test_split_module test to include wasm64 scenarios?

@Mintyboi
Copy link
Contributor Author

Mintyboi commented Oct 7, 2025

While working on split_module + mem64 in my application, I notice more changes required on top of this PR. I'm working on resolving those issues as well

@sbc100
Copy link
Collaborator

sbc100 commented Oct 7, 2025

Yes, I was hoping you can add @also_with_wasm64 to test_split_module

@Mintyboi
Copy link
Contributor Author

Mintyboi commented Oct 8, 2025

@sbc100 Do you have suggestions on how I can tweak test_split_module.post.js such that it'll work for both wasm32 and wasm64.
e.g.

  • For wasm32, we use __write_profile(0, 0);
  • For wasm64, we use __write_profile(0n, 0);

Could I perhaps create another file? i.e. test_split_module_wasm64.post.js

@sbc100 sbc100 enabled auto-merge (squash) October 8, 2025 17:49
@sbc100
Copy link
Collaborator

sbc100 commented Oct 8, 2025

@sbc100 Do you have suggestions on how I can tweak test_split_module.post.js such that it'll work for both wasm32 and wasm64. e.g.

  • For wasm32, we use __write_profile(0, 0);
  • For wasm64, we use __write_profile(0n, 0);

Could I perhaps create another file? i.e. test_split_module_wasm64.post.js

You can use the toIndexType macro. e.g __write_profile({{{ toIndexType(0) }}}, 0);

@sbc100
Copy link
Collaborator

sbc100 commented Oct 8, 2025

Actually maybe to64 is more appropriate this this case.

@sbc100 sbc100 merged commit 18fa2a3 into emscripten-core:main Oct 8, 2025
33 checks passed
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 8, 2025
This should have been part of emscripten-core#25432 but I guess I landed it too
eagerly.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 8, 2025
This should have been part of emscripten-core#25432 but I guess I landed it too
eagerly.
sbc100 added a commit that referenced this pull request Oct 8, 2025
This should have been part of #25432 but it seems we had a gap in our
wasm64 testing because the `other` test suite is run with the standard
emsdk version of node (v18) which does not support wasm64.
@Mintyboi
Copy link
Contributor Author

Mintyboi commented Oct 9, 2025

__write_profile({{{ toIndexType(0) }}}, 0);

I've tried this actually, but the macro block does not expanded in the final output. Does this file go through the JS preprocessor?
I also tried using only the function as well, but toIndexType is not defined in the output out\test\test_split_module.js file

@Mintyboi
Copy link
Contributor Author

Mintyboi commented Oct 9, 2025

Update: I noticed that you have made a commit to handle this.
Thanks!

One more thing, I looked at the CI logs and it seems like we are not running wasm64 tests
test_split_module_wasm64 (test_other.other) ... skipped 'test requires node >= 24 or d8 (and EMTEST_SKIP_WASM64 is set)'

@Mintyboi Mintyboi deleted the fix-split-module-load branch October 9, 2025 01:48
@sbc100
Copy link
Collaborator

sbc100 commented Oct 9, 2025

Yup, see #25531, which also includes a fix to run the wasm64 other tests

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.

2 participants