Skip to content

Fix pointer arguments in addFunction + wasm64 #24693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix pointer arguments in addFunction + wasm64 #24693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/lib/libaddfunction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,30 @@ addToLibrary({
#if ASSERTIONS && !WASM_BIGINT
assert(!sig.includes('j'), 'i64 not permitted in function signatures when WASM_BIGINT is disabled');
#endif

#if MEMORY64
// Find all 'p' in the signature, which indicates a pointer we need to convert to/from bigint.
var pReturn = sig[0] == 'p';
var pArgs = [];
for (var i = 1; i < sig.length; ++i) {
if (sig[i] == 'p') {
pArgs.push(i - 1);
}
}
if (pReturn || pArgs.length) {
var origFunc = func;
func = (...args) => {
for (var i of pArgs) {
// Convert the pointer arguments from bigint to number.
args[i] = Number(args[i]);
}
var ret = origFunc(...args);
// Convert the return value from number to bigint if needed.
return pReturn ? BigInt(ret) : ret;
};
}
#endif

#if WASM_JS_TYPES
// If the type reflection proposal is available, use the new
// "WebAssembly.Function" constructor.
Expand Down
31 changes: 30 additions & 1 deletion test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15458,7 +15458,6 @@ def test_add_js_function(self, wasm2js):
'memory64': (True, False),
'': (False, False),
})
@requires_v8
def test_add_js_function_bigint(self, memory64, wasm_function):
self.set_setting('WASM_BIGINT')

Expand Down Expand Up @@ -15491,6 +15490,36 @@ def test_add_js_function_bigint(self, memory64, wasm_function):

self.do_runf('main.c', '')

@requires_wasm64
def test_add_js_function_pointers_wasm64(self):
self.set_setting('MEMORY64')
self.set_setting('ALLOW_TABLE_GROWTH')

create_file('main.c', r'''
#include <emscripten.h>
#include <assert.h>

EM_JS_DEPS(deps, "$addFunction");

typedef void* (functype)(void*);

int main() {
functype* f = EM_ASM_PTR({
return addFunction((ptr) => {
return ptr + 1;
}, 'pp');
});

void* p1 = (void*)26;
assert(f(p1) == p1 + 1);

void* p2 = (void*)493921253191;
assert(f(p2) == p2 + 1);
}
''')

self.do_runf('main.c', '')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@RReverser RReverser Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@RReverser RReverser Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you trying trying to add non-js-library functions manually using addFunction with 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.


@parameterized({
'': ([],),
'pthread': (['-g', '-pthread', '-Wno-experimental', '-sPROXY_TO_PTHREAD', '-sEXIT_RUNTIME'],),
Expand Down