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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 .sig which 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 addFunction is 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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 addFunction does 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.


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