[SPLIT_MODULE] Fix imports into deferred module when loading

[Mintyboi]

Fix an error when attempted to reserve a new memory space in the deferred module when building with MEMORY64 + SPLIT_MODULE.

TypeError: Cannot convert <func ptr> to a BigInt

In a mem64 build, we call applySignatureConversions on malloc() in the wasmExports. applySignatureConversions() wraps the exports and converts the return value to a javascript number. The wasmExports is passed into the deferred module during instantiation so when we call a malloc in the deferred module, we'd be calling the wrapped function, hitting into this error.

Furthermore, we are making a direct call when calling a malloc, so strictly speaking, we should not need to go through the JS trampoline.

[sbc100]

I only see a test change so I guess this PR is WIP?

[Mintyboi]

I only see a test change so I guess this PR is WIP?

Yes, I plan to have the tests fail first and add in the fix later, which should fix the tests as well 🙏

[sbc100]

Sure, we can't actually land the failing test right? So I guess I'll wait to review this?

[sbc100]

(If you are just experimenting then maybe do that with the PR marked as "DRAFT" or you can also do that in your own fork too)

[Mintyboi]

@sbc100 I have reproduced the issue I'm facing in the test suite and some attempted fix that works in resolving this issue.
However these changes does not sit too well with me, can I check if you have any suggestions / alternative approach to resolve the issue where we are calling a wrapper with incorrect types.

[sbc100]

@aheejin @tlively who have been working on module splitting more than I have.

The solution does seem reasonable yes. We so something similar in dynamic linking where we maintain wasmImports as the symbol table we feed to new wasm module This gets populated with raw exports from each module as opposed to wrapped exports.

Can you please wrap all these changes in #if SPLIT_MODULE to avoid bloating the generated code for those not use module splitting?

[aheejin]

Sorry, I'm not sure if I understand the problem and the solution here.

Can you give the command line to run your test? It'd be good if we have a test in one of test_***.py files that actually runs the .c file you added.

Also... I guess I'm missing something, but the solution just looks like renaming wasmExports to wasmRawExports..? Why does this solve the problem?

[sbc100]

Sorry, I'm not sure if I understand the problem and the solution here.

Can you give the command line to run your test? It'd be good if we have a test in one of test_***.py files that actually runs the .c file you added.

Also... I guess I'm missing something, but the solution just looks like renaming wasmExports to wasmRawExports..? Why does this solve the problem?

I think the issue is that if we just use wasmExports this map start life as the raw wasm exports but in some configuration we add JS wrapper functions around these (e.g. in wasm64). However with module splitting (just like dynamic linking) we never want to feed there wrapper function as imports into another wasm module, we want to use the original exports in this case.

The issue only shows up course then wrapper functions are used.

[Mintyboi]

Sorry, I'm not sure if I understand the problem and the solution here.

Can you give the command line to run your test? It'd be good if we have a test in one of test_***.py files that actually runs the .c file you added.

Also... I guess I'm missing something, but the solution just looks like renaming wasmExports to wasmRawExports..? Why does this solve the problem?

Thanks for explaning the issue @sbc100
wasmRawExports is saved before we apply the wrappers in preamble.js‎L#738.
I have update the test_split_module tests that you can execute with python runner.py other.test_split_module. And to reproduce the issue, you can rename wasmRawExports back to wasmExports in splitModuleProxyHandler()

[sbc100]

This line is no longer needed

[aheejin]

Sorry, I'm not sure if I understand the problem and the solution here.
Can you give the command line to run your test? It'd be good if we have a test in one of test_***.py files that actually runs the .c file you added.
Also... I guess I'm missing something, but the solution just looks like renaming wasmExports to wasmRawExports..? Why does this solve the problem?

I think the issue is that if we just use wasmExports this map start life as the raw wasm exports but in some configuration we add JS wrapper functions around these (e.g. in wasm64). However with module splitting (just like dynamic linking) we never want to feed there wrapper function as imports into another wasm module, we want to use the original exports in this case.

The issue only shows up course then wrapper functions are used.

Does this mean SPLIT_MODULE is not going to work with wasm64?

[sbc100]

Sorry, I'm not sure if I understand the problem and the solution here.
Can you give the command line to run your test? It'd be good if we have a test in one of test_***.py files that actually runs the .c file you added.
Also... I guess I'm missing something, but the solution just looks like renaming wasmExports to wasmRawExports..? Why does this solve the problem?

I think the issue is that if we just use wasmExports this map start life as the raw wasm exports but in some configuration we add JS wrapper functions around these (e.g. in wasm64). However with module splitting (just like dynamic linking) we never want to feed there wrapper function as imports into another wasm module, we want to use the original exports in this case.
The issue only shows up course then wrapper functions are used.

Does this mean SPLIT_MODULE is not going to work with wasm64?

the wrappers we generate for wasm64 are for the benefit of JS. They basically turn bigint values into numbers so that JS developers don't have to worry about the "pointers are bigints" problem (at least not in most cases).

The importing function between one wasm module and another we explictly do not want to use these wrappers, and that would actually break, which this PR fixes I believe.

[Mintyboi]

@sbc100 test sanity has been failing but it seems to be unrelated to my changes.
Do you know how I can fix it?

[sbc100]

Yup, i think a rebase would fix but I'm also happy to just land as-is.

Thanks for the contribution!