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.
for example I think EM_JS/EM_ASM has no way to deal with this today and you need to modify your code to handle wasm64
Right, but the way it was discussed in the past gave me an impression it's considered a bug / a limitation of the current system rather than desired outcome? UPD: I think it's also only EM_JS, EM_ASM has a way to determine pointers.
How many folks are calling
addFunctiondirectly? Let along with a the sig argument set. Did you run into this issue because you are doing this?
Yeah. I can work around it manually, but contributing a fix to the addFunction itself seemed like the better choice since it will likely affect other users too when they add wasm64 support.
Doing things like ptr + length is certainly not uncommon, and any such code would break if it ends up in addFunction (eg via dylink) + compiled for wasm64.
There was a problem hiding this comment.
Note that the function is wrapped only if it has p somewhere in its signature.
So if you say JS library functions get p converted to i/j statically, then they should be skipped from the wrapping anyway.
There was a problem hiding this comment.
So which functions are effected then? i.e. how did you come across this bug? Are you trying trying to add non-js-library functions manually using addFunction with manual signature agument specified?
There was a problem hiding this comment.
So if you say JS library functions get
pconverted toi/jstatically, then they should be skipped from the wrapping anyway.
I don't think the current wrapping process modifies that .sig property of the function to remove the p elements. Perhaps if it did that would alleviate that concern about double wrapping. (But we would need to be sure that we don't need to process those p values elsewhere at runtime too).
There was a problem hiding this comment.
Are you trying trying to add non-js-library functions manually using
addFunctionwith manual signature agument specified?
Yeah, or, well, not quite manual - I'm computing signature in Embind (#24611 was related to this). I'm trying to use addFunction for another Embind-related PR and without this fix it would fail on wasm64 due to BigInt vs number incompatibility.
I can definitely fix it one level up, just for my own function, but as I said, I thought fixing it for other addFunction users would be the right choice.
If you strongly feel it's not, I can leave this PR open and make that other PR with just a localized hotfix.
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.