Fix pointer arguments in addFunction + wasm64

[RReverser]

Without this fix, any usages of addFunction with 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 dynCall and convert all pointer arguments to numbers before invoking the actual JS function, and the pointer result to bigint before returning to wasm.

[sbc100]

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.

[RReverser]

Okay, as long as it doesn't block the fix too long.

[RReverser]

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.

[sbc100]

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.

[sbc100]

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?

[sbc100]

All library JS functions are modified here:

emscripten/src/jsifier.mjs

Lines 267 to 277 in b490c44

	function handleI64Signatures(symbol, snippet, sig, i53abi) {
	// Handle i64 parameters and return values.
	//
	// When WASM_BIGINT is enabled these arrive as BigInt values which we
	// convert to int53 JS numbers. If necessary, we also convert the return
	// value back into a BigInt.
	//
	// When WASM_BIGINT is not enabled we receive i64 values as a pair of i32
	// numbers which is converted to single int53 number. In necessary, we also
	// split the return value into a pair of i32 numbers.
	return modifyJSFunction(snippet, (args, body, async_, oneliner) => {

So (IIUC) this change is not necessary for JS library functions, in fact its somewhat detrimental in that case.

[RReverser]

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 addFunction directly? 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.

[RReverser]

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.

[sbc100]

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?

[sbc100]

So if you say JS library functions get p converted to i/j statically, 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).