Fix pointer arguments in addFunction + wasm64 #24693
Conversation
Without this fix, any usages of addFunction - e.g. dylink - would add broken functions to the webassembly table, which result in errors like "Cannot mix BigInt and other types, use explicit conversions" when invoked. To fix this, we need to do essentially the opposite of what we do in `dynCall` and convert all pointer arguments to numbers before invoking the actual JS function, and the pointer result to bigint before returning to wasm.
RReverser
force-pushed
the
add-function-p-wasm64
branch
from
e08b2bd to
3d8535f
Compare
| } | ||
| ''') | ||
|
|
||
| self.do_runf('main.c', '') |
There was a problem hiding this comment.
I'm a little surprised we don't have any existing tests of addFunction being used with JS functions with 'p' in their signatures.
I'd like to take a moment to try and figure out why no existing test covered this.
There was a problem hiding this comment.
Okay, as long as it doesn't block the fix too long.
There was a problem hiding this comment.
I suspect it's possible that even if there were any tests, they 1) weren't doing pointer arithmetic and 2) weren't returning a pointer from JS to Wasm, only the other way around.
Without these conditions, e.g. if you have a simple tests that only logs the passed argument, the issues wouldn't trigger.
There was a problem hiding this comment.
I'm just a little suprised we don't run into this issue with our exist dynamic linking tests. The main user or addFunction for JS function with a signature passed are in dynamic linking, firstly in dlsym and secondly (which I imagine occurs way more often) in reportUndefinedSymbols which will fill in missing GOT entries using JS functions. I'm surprised we don't run into this there.
There was a problem hiding this comment.
Oh wait.. I think its because JS libraries functions are modified in place when they are constructed, right? So this change is not necessary for JS library functions at all, only for JS functions that come from elsewhere?
Is that right?
There was a problem hiding this comment.
Ok, fair enough. Given that it's possible to use addFunction from user code, and that it lives in its own libaddfunction.js, I assumed it's part of public API, but it sounds like it's just one of many functions that isn't marked as __internal: true for backward compat / legacy reasons rather than intended to be used by external developers?
If so, I'm happy to also close this and fix just my individual usage of addFunction in the Embind PR where I needed it.
There was a problem hiding this comment.
Yeah, I think its does have some users, but I'd like to discourage its usage if possible. I'd rather see dynamic dispatch mechanisms be built on the JS side instead of by injecting into (modifying the wasm table at runtime). (I think only the dynamic linker should be doing that really, but I've never really articulated that in any official docs anywhere).
There was a problem hiding this comment.
Hmm I'm finding even more usecases for addFunction where it would save quite a bit of JS code as well as make dispatch faster, pretty much win-win.
we would need to make sure this code didn't create extra wrapping for JS library functions (perhaps by removing the 'p' elements from the
.sigwhich we generate at compile time).
So I think I'll spend some more time sorting this out after all.
I'd rather see dynamic dispatch mechanisms be built on the JS side
Pretty much the opposite preference here, the more I play with it - if we can avoid extra JS work and perform "direct" typed calls, why not do that?
I'm tempted to say we shouldn't bother landing this since (I hope) mode usage of
addFunctionis internal and not user-visible.
I also remembered that it's actually been a documented API for invoking JS callbacks from Wasm: https://emscripten.org/docs/porting/connecting_cpp_and_javascript/Interacting-with-code.html#calling-javascript-functions-as-function-pointers-from-c
There was a problem hiding this comment.
Pretty much the opposite preference here, the more I play with it - if we can avoid extra JS work and perform "direct" typed calls, why not do that?
But is there any real performance advantage over having an additional lookup table on the JS side?
Also, remember that addFunction does not currently work with threading, is that a limitation you are OK with?
There was a problem hiding this comment.
But is there any real performance advantage over having an additional lookup table on the JS side?
There is for variadic functions, anywhere where you otherwise have to encode type info and number of arguments out of band.
Also, remember that
addFunctiondoes not currently work with threading, is that a limitation you are OK with?
Yeah that's fine, I'm only lazily storing it in thread-locals.
Without this fix, any usages of
addFunctionwith pointers in signatures would add broken functions to the webassembly table, which would fail with errors like "Cannot mix BigInt and other types, use explicit conversions" when invoked.To fix this, we need to do essentially the opposite of what we do in
dynCalland convert all pointer arguments to numbers before invoking the actual JS function, and the pointer result to bigint before returning to wasm.