[embind] Use a single invoker mechanism #24577

RReverser · 2025-06-16T13:15:03Z

This is the next step in refactorings I started back in #20383 to merge all Embind generic method callers into a single mechanism. I never got around to landing this PR because I kept trying to split it into even smaller changes / steps, never found a good way to do so due to how entangled they were, got frustrated and left it alone.

However, aside from being a nice-to-have optimisation or cleanup refactoring, it's also an easy way to deal with issues like #24547 which makes it a bit more pressing. Without this PR, we'd have to duplicate the emval_as+emval_as_int64+emval_as_uint64 fix from #13889 for emval_call and emval_call_method as well, but after this PR everything goes through the same centralised implementation where we can deal with all Embind types in a consistent manner.

There are some more things that I'd like to clean up here (including some of the complexity added by this very own PR), but in order to try and keep review scope smaller, I'm going to submit them separately.

Fixes #24547.

sbc100

I can't say I understand all of this but look like a good direction. I leave @brendandahl to do a full review.

sbc100 · 2025-06-16T16:00:08Z

src/lib/libemval.js

 #if !DYNAMIC_EXECUTION
    var argN = new Array(argCount);
-    var invokerFunction = (obj, func, destructorsRef, args) => {
+    var invokerFunction = (handle, methodName, destructorsRef, args) => {


Would it make more sense to call the first argument receiver? Or is handle (the type of the receiver I guess?) more descriptive here?

handle as in generally EM_VAL. If the invoker kind is "method" then it's a handle of the receiver, if it's a "function" or a "constructor" then it's a handle of the function itself, and if it's a cast, then it's just null... A handle seemed like the most generic name.

test/code_size/embind_val_wasm.json

test/embind/test_i64_val.cpp

system/include/emscripten/val.h

RReverser · 2025-06-16T23:44:13Z

The wasm2js failure looks a bit suspicious:

error: undefined symbol: _emscripten_tempret_set (referenced by $setTempRet0, referenced by setTempRet0, referenced by _emval_invoke_i64, referenced by root reference (e.g. compiled C/C++ code))
warning: To disable errors for undefined symbols use -sERROR_ON_UNDEFINED_SYMBOLS=0
warning: __emscripten_tempret_set may need to be added to EXPORTED_FUNCTIONS if it arrives from a system library
Error: Aborting compilation due to previous errors
em++: error: '/root/emsdk/node/22.16.0_64bit/bin/node /root/project/tools/compiler.mjs -' failed (returned 1)

I guess it comes from makeReturn64, but then if the usage is injected automatically, shouldn't the dependency be added automatically as well by jsify? @sbc100

sbc100 · 2025-06-16T23:54:10Z

Yes, _emscripten_tempret_set is listed as a native dependency of $setTempRet.

If you build with EMCC_DEBUG=1 should should see something like /tmp/emscripten_temp/tmpz6ttjgoflibemscripten_js_symbols.so being passed to the linker which is a map of JS dependencies.

However this only handles native->JS dependencies, but what we have here is a case of the JS compiler itself generating calls to a JS function (effectively a JS->JS dependency), but I guess its being injected somehow too late (and the native link has already been done).

If you can share a repro case I can take a look.

RReverser · 2025-06-16T23:55:33Z

If you can share a repro case I can take a look.

I don't have a smaller one, I've only guessed what's happening by searching for keywords from the error message for now.

sbc100 · 2025-06-16T23:58:32Z

Does it fix the issue if you make the condition on line 536 of jsifier.js match the one on 314. It looks like they should be the same condition but they are not.

RReverser · 2025-06-17T00:05:13Z

No, still the same. I even tried making that dep unconditional just in case, but nothing:

.\test\runner wasm2js1.test_embind_unsigned
Running test_core: (1 tests)
test_embind_unsigned (test_core.wasm2js1) ... error: undefined symbol: _emscripten_tempret_set (referenced by $setTempRet0, referenced by setTempRet0, referenced by $ExitStatus, referenced by root reference (e.g. compiled C/C++ code))

Worth noting that setTempRet0 itself is discovered fine, so that condition shouldn't matter - the linker is complaining specifically about _emscripten_tempret_set.

Looks like that one is implemented in assembly, maybe it needs to be somehow exposed to wasm2js conversion?

RReverser · 2025-06-17T00:14:08Z

It's all the weirder that wasm2js0.test_embind_unsigned and wasm2js2.test_embind_unsigned pass fine but wasm2js1.test_embind_unsigned fails with said linker error.

Aren't those digits at the end just optimisation levels?

sbc100 · 2025-06-17T00:49:23Z

yes, you can see the setTempRet0 dependency is supposed to be added on line 530 (deps.push('setTempRet0')). Not that this needs to happen before the native link happens and the end result is that the native dependencies of setTempRet0 should be passed to wasm-ld with --export-if-defined=XX (for each native JS dependency).

RReverser · 2025-06-17T00:54:47Z

Sorry, I still don't understand.

yes, you can see the setTempRet0 dependency is supposed to be added on line 530 (deps.push('setTempRet0')).

But then why even making it unconditional doesn't fix the issue? And why is setTempRet0 the problem if it's added successfully already, just its own native dep isn't? And why does this not happen in wasm2js0 and wasm2js2?

I don't understand the mechanism here at all, so I have more questions than answers 😅

sbc100 · 2025-06-17T01:17:28Z

Sorry, I still don't understand.

yes, you can see the setTempRet0 dependency is supposed to be added on line 530 (deps.push('setTempRet0')).

But then why even making it unconditional doesn't fix the issue? And why is setTempRet0 the problem if it's added successfully already, just its own native dep isn't?

Answering just this part: We process the JS libraries files twice. Once in --symbol-only mode where we extract dependencies. We use this dependency information for figure out which native symbols we might need to --export-if-defined.

I think this issue is that while setTempRet0 is added to the deps here, it never actually ends up in DEFAULT_LIBRARY_FUNCS_TO_INCLUDE out on the python side. I think this is likely we use when searching for which native symbols to explicitly export with --export-if-defined

The code for this is here:

emscripten/tools/link.py

Lines 3104 to 3128 in 542dc42

    
           if (not settings.SIDE_MODULE or settings.ASYNCIFY) and not shared.SKIP_SUBPROCS: 
        
             js_info = get_js_sym_info() 
        
             if not settings.SIDE_MODULE: 
        
               js_syms = js_info['deps'] 
        
               if settings.LINKABLE: 
        
                 for native_deps in js_syms.values(): 
        
                   settings.REQUIRED_EXPORTS += native_deps 
        
               else: 
        
                 def add_js_deps(sym): 
        
                   if sym in js_syms: 
        
                     native_deps = js_syms[sym] 
        
                     if native_deps: 
        
                       settings.REQUIRED_EXPORTS += native_deps 
        
                 for sym in settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE: 
        
                   add_js_deps(sym) 
        
                 for sym in js_info['extraLibraryFuncs']: 
        
                   add_js_deps(sym) 
        
                 for sym in settings.EXPORTED_RUNTIME_METHODS: 
        
                   add_js_deps(shared.demangle_c_symbol_name(sym)) 
        
                 for sym in settings.EXPORTED_FUNCTIONS: 
        
                   add_js_deps(shared.demangle_c_symbol_name(sym)) 
        
             if settings.ASYNCIFY: 
        
               settings.ASYNCIFY_IMPORTS_EXCEPT_JS_LIBS = settings.ASYNCIFY_IMPORTS[:] 
        
               settings.ASYNCIFY_IMPORTS += ['*.' + x for x in js_info['asyncFuncs']]

Specifically I think we are missing a call to add_js_deps for getTempRet0.

And why does this not happen in wasm2js0 and wasm2js2?

I imagine this is because at O0 getTempRet0 and/or its dependencies are exported for other reasons.

RReverser · 2025-06-17T01:57:38Z

I imagine this is because at O0 getTempRet0 and/or its dependencies are exported for other reasons.

That would make sense, but then why does O2 pass? Shouldn't it be more restrictive than O1 basically always?

Could you help me fix this one please? I still don't understand what the fix should look like.

RReverser · 2025-06-17T10:22:08Z

Wait it passed after a rebase, wat? Nothing relevant changed upstream that I can see.

RReverser · 2025-06-17T10:25:10Z

The CI failure other.test_jspi_async_function seems unrelated, likely a bad combination of #24578 + #24550 - I guess they raced on CI. @brendandahl

RReverser · 2025-06-17T16:45:08Z

@brendandahl Can you please fix CI? I don't think I can merge until then (unless someone with force-merge rights wants to do it).

RReverser · 2025-06-17T16:48:54Z

Can you please fix CI?

For #24550 I mean.

sbc100 · 2025-06-17T16:53:53Z

Can you please fix CI?

For #24550 I mean.

#24590

This is the next step in refactorings I started back in emscripten-core#20383 to merge all Embind generic method callers into a single mechanism. I never got around to landing this PR because I kept trying to split it into even smaller changes / steps, never found a good way to do so due to how entangled they were, got frustrated and left it alone. However, aside from being a nice-to-have optimisation or cleanup refactoring, it's also an easy way to deal with issues like emscripten-core#24547 which makes it a bit more pressing. Without this PR, we'd have to duplicate the `emval_as`+`emval_as_int64`+`emval_as_uint64` fix from emscripten-core#13889 for `emval_call` and `emval_call_method` as well, but after this PR everything goes through the same centralised implementation where we can deal with all Embind types in a consistent manner. There are some more things that I'd like to clean up here (including some of the complexity added by this very own PR), but in order to try and keep review scope smaller, I'm going to submit them separately. Fixes emscripten-core#24547.

RReverser · 2025-06-17T19:36:44Z

FAIL [1.119s]: test_fetch_redirect (test_browser.browser64_4gb)

Given how often it flakes lately, it really should use a local server instead of relying on httpbin.org... @sbc100 can you merge manually please? I already did rerun and it takes forever only to run into another flake.

sbc100 · 2025-06-17T20:19:20Z

FAIL [1.119s]: test_fetch_redirect (test_browser.browser64_4gb)

Given how often it flakes lately, it really should use a local server instead of relying on httpbin.org... @sbc100 can you merge manually please? I already did rerun and it takes forever only to run into another flake.

Agreed. Would you open a bug for that?

RReverser · 2025-06-17T20:54:59Z

Thanks, and done: #24597

I'm having real trouble reproducing this UB in isolation, but I ran into issues with garbage values in test_i64_val while making other innocent-looking changes. I think it might explain the really weird bug from emscripten-core#24577 (comment) that magically went away after a rebase despite no related changes being made. Essentially, because I stupidly used `T&&` in a template, and because it's [waves around] C++, it got inferred as `unsigned long long &` instead of the desired `unsigned long long`, so in `.as<T>()` we were quietly returning `unsigned long long&` references to a temporary `unsigned long long` value on function stack. Most of the time it somehow still works and, because it's templated, it's not caught by Clang's "non-const lvalue reference to type" diagnostic, but under unrelated changes and optimisations it can break badly.

I'm having trouble reproducing this UB in isolation, but I ran into issues with garbage values in test_i64_val while making other innocent-looking changes. I think it might even explain the really weird bug from #24577 (comment) that magically went away after a rebase despite no related changes being made upstream. Essentially, because I stupidly used `T&&` in a template, and because it's [waves around] C++, it got inferred as `unsigned long long &` instead of the desired `unsigned long long`, so in `.as<T>()` we were quietly returning an `unsigned long long&` reference to a temporary `unsigned long long` value on function stack. Most of the time it somehow still works and, because it's templated, it's not caught by Clang's "non-const lvalue reference to type" diagnostic, but under unrelated changes and optimisations it can break badly.

RReverser requested review from brendandahl and sbc100 June 16, 2025 13:15

RReverser force-pushed the single-caller branch from fb3ea98 to 8c7bf12 Compare June 16, 2025 13:16

sbc100 reviewed Jun 16, 2025

View reviewed changes

brendandahl requested changes Jun 16, 2025

View reviewed changes

system/include/emscripten/val.h Show resolved Hide resolved

RReverser force-pushed the single-caller branch from 8975da5 to 06ddc64 Compare June 17, 2025 01:01

RReverser force-pushed the single-caller branch from 06ddc64 to 16ff746 Compare June 17, 2025 01:58

RReverser requested a review from brendandahl June 17, 2025 10:26

brendandahl approved these changes Jun 17, 2025

View reviewed changes

RReverser force-pushed the single-caller branch from 16ff746 to 7c09e34 Compare June 17, 2025 17:06

RReverser enabled auto-merge (squash) June 17, 2025 17:06

sbc100 disabled auto-merge June 17, 2025 20:19

sbc100 merged commit d8dda21 into emscripten-core:main Jun 17, 2025
28 of 30 checks passed

RReverser deleted the single-caller branch June 17, 2025 20:54

RReverser mentioned this pull request Jun 18, 2025

[emval] Prevent creating lvalue refs from thin air #24606

Merged

stbergmann mentioned this pull request Jul 28, 2025

[emval] Support dynamically-typed conversion to/from val #24790

Open

DerThorsten mentioned this pull request Sep 12, 2025

"Cannot convert 14 to a BigInt" error introduced in 4.0.11 #25256

Open

juj mentioned this pull request Sep 19, 2025

stringToUTF16 trows "Cannot mix BigInt and other types" for wasm64 #25315

Open

[embind] Use a single invoker mechanism #24577

[embind] Use a single invoker mechanism #24577

Uh oh!

Conversation

RReverser commented Jun 16, 2025

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

RReverser Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RReverser commented Jun 16, 2025

Uh oh!

sbc100 commented Jun 16, 2025

Uh oh!

RReverser commented Jun 16, 2025

Uh oh!

sbc100 commented Jun 16, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

sbc100 commented Jun 17, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

sbc100 commented Jun 17, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

sbc100 commented Jun 17, 2025

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

sbc100 commented Jun 17, 2025

Uh oh!

Uh oh!

RReverser commented Jun 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants