Open Mintyboi opened 2 weeks ago
Hmm, I guess we're correctly waiting for the secondary module to be loaded, but then we're failing to wait again when the secondary function returns a Promise. @brendandahl, how complicated do you think this would be to fix?
I was thinking it would be as simple as adding suspending wrappers around the imports that are passed to the secondary module. e.g. in __load_secondary_module
wasmExports['%jsPrintSomething'] = new WebAssembly.Suspending(wasmExports['%jsPrintSomething']);
However, that doesn't seem to work and now that I think about it, I'm not sure if JSPI is supposed to work across wasm module calls, or do we need to have JSPI promising/suspending wrappers in between the module calls. Right now we have:
JS->mainPromisingWrapper->mainInPrimaryWasm->fooInSecondaryWasm->sleepSuspendingWrapper->sleepInJs
but do we instead need:
JS->mainPromisingWrapper->mainInPrimaryWasm->fooSuspendingWrapper->fooPromisingWrapper-> fooInSecondaryWasm->sleepSuspendingWrapper->sleepInJs
There should be no need for the wrappers in the wasm-to-wasm cross-module calls, but perhaps this is not working correctly in the current implementation. @fgmccabe, do you know if this is supposed to work correctly right now?
plain (without jspi) wasm-to-wasm cross module calls are working (we have tests for that case).
This: wasmExports['%jsPrintSomething'] = new WebAssembly.Suspending(wasmExports['%jsPrintSomething']);
should cause an exception. Because new WebAssembly.Suspending is expecting a JS function, not a WebAssembly function.
This: JS->mainPromisingWrapper->mainInPrimaryWasm->fooInSecondaryWasm->sleepSuspendingWrapper->sleepInJs
is what we expect to work.
This: wasmExports['%jsPrintSomething'] = new WebAssembly.Suspending(wasmExports['%jsPrintSomething']);
should cause an exception. Because new WebAssembly.Suspending is expecting a JS function, not a WebAssembly function.
I had a bit more time to look into this and this is kind of the issue. wasmExports['%jsPrintSomething']
comes from the main wasm module, but it is the imported JS function that is also exported. If I wrap wasmExports['%jsPrintSomething']
with suspending, I don't get an error, but it doesn't work as expected. However, if I replace the export with the original JS function then it does work as expected e.g. wasmExports['%jsPrintSomething'] = new WebAssembly.Suspending(_jsPrintSomething);
Two options to fix:
1) Change V8 to support doing WebAssembly.Suspending(aWasmExportThatIsActuallyJS);
2) Change emscripten/binaryen to use the original JS import and wrap that. This option seems challening with how things are currently structured.
One thing to clarify above where naming is confusing, wasmExports
is passed into the secondary module as the imports.
There is a third option:
WebAssembly.Suspending((X)=>wasmExport(X))
That will force the function argument to Suspending to be JS. And it would be a local change.
I talked to @brendandahl offline about this to understand the problem more, and I think this situation should not require any additional wrapping at all.
My understanding is that one instance imports new WebAssembly.Suspending(someJSFunc)
, then re-exports it directly as "%someJSFunc"
. The second instance then imports and calls "%someJSFunc"
, which suspends. This should not require any additional wrappers on the boundary between the first instance and the second instance because that is a Wasm-to-Wasm boundary, even though the call is to a directly re-exported JS function rather than a normal Wasm function. It's important to maintain the property that re-exporting an imported function has the same behavior as exporting a trampoline that simply calls the imported functoin.
@brendandahl, can you confirm that something goes wrong when there is no additional wrapping between the two modules? Can you also confirm that the proxy handler that loads the secondary module is not on the stack for this interaction? As we discussed, we need to add more wrappers in the proxy handler, but I'd like to confirm that this is a separate issue.
Part of this is simplicity of specification. The Suspending spec is written to only take JS functions as arguments. This was 'part of the bargain' when the JSPI was simplified to not need an explicit Suspender object.
Requiring an exported wasm function to be recognized as a JS function would add extra corners to the specification for marginal gain.
Using the lambda wrapper is cheap and likely inlineable in most cases.
Neither wrapping of Wasm functions nor using a lambda should be necessary, since there should not need to be any wrappers between the two Wasm modules.
Please include the following in your bug report:
Version of emscripten/emsdk: emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.65 (7f8a05dd4e37cbd7ffde6d624f91fd545f7b52e3) clang version 20.0.0git (https:/github.com/llvm/llvm-project 547917aebd1e79a8929b53f0ddf3b5185ee4df74)
Issue I have a code like this
and the js library as follows:
Build command: emcc main.cpp -g -sMAIN_MODULE=2 -o index.js --js-library=jslibrary.js -sINCLUDE_FULL_LIBRARY=1 -sSPLIT_MODULE=1 -sJSPI -sJSPI_IMPORTS=jsPrintSomething
After doing a split module and patching https://github.com/emscripten-core/emscripten/issues/22487, the Actual result I'm getting is
Expected result:
Reproducer: jspi_wrong_execution.zip