Fix dyncall and dynamic linking with -sMAIN_MODULE=1 #16693

hoodmane · 2022-04-10T17:05:50Z

Sometimes side modules need dyncalls with 64 bit types that are missing the dynCall wrappers from the main module. In this case, we can fall back to trying to use BigInt. I don't want to compile with -sWASM_BIGINT BigInt64Array has under a year of support in Safari. BigInt has been supported in Safari since v14.0 so I'm not as worried about this. In any case, this works in strictly more cases than it would before.

We don't compile with WASM_BIGINT so if there is a 64 bit integer in the dynamic call, Emscripten tries to call a legalizer wrapper. These legalizer wrappers are only generated in the main module so sometimes a side module tries to call a wrapper that doesn't exist. This adds an extra handler in that case.

kripken · 2022-04-11T19:50:47Z

src/library.js

@@ -3139,7 +3139,29 @@ LibraryManager.library = {
    // part of thier signature, so we rely the dynCall functions generated by
    // wasm-emscripten-finalize
    if (sig.includes('j')) {
+#if MAIN_MODULE
+      if(Module["dynCall_" + sig]){
+        return dynCallLegacy(sig, ptr, args);


Hmm, reading this I'm confused as to why dynCall_* would only sometimes exist when DYNCALLS. That is, we should have returned from line 3135 above, I think? Is this related to this issue from the description:

"Sometimes side modules need dyncalls with 64 bit types that are missing the dynCall wrappers from the main module."

If so, could we include dyncalls in side modules? cc @sbc100

I will try to come up with a MWE soon. It occurred on this branch:
pyodide/pyodide#2378
with the signature viij but I have seen it before.

could we include dyncalls in side modules? cc @sbc100

Yeah I suspect you can do that, perhaps it's a better solution? BigInt support is much broader than BigInt64Array so I don't mind this. But yeah it would be harder for me to come up with this.

sbc100 · 2022-04-12T17:37:45Z

Can you explain why you don't want to do use -sWASM_BIGINT? It seems like the right solution here.. and if it isn't then perhaps we would want an different option for this limited use of BigInt?

sbc100 · 2022-04-12T17:44:17Z

Can you share the backtrace of there the failing call to dynCall is coming from? Am I correct in assuming its coming from a function generated by createInvokeFunction in response of an invoke_xx import? here:

emscripten/src/library_dylink.js

Lines 251 to 255 in ec1cbf0

    
           $createInvokeFunction: function(sig) { 
        
             return function() { 
        
               var sp = stackSave(); 
        
               try { 
        
                 return dynCall(sig, arguments[0], Array.prototype.slice.call(arguments, 1));

hoodmane · 2022-04-12T17:51:33Z

Yeah: we don't want -sWASM_BIGINT because it doesn't work on Safari 14 and we haven't made up our mind that we are officially not supporting Safari 14.

…

On Tue, Apr 12, 2022, 10:44 Sam Clegg ***@***.***> wrote: Can you share the backtrace of there the failing call to dynCall is coming from? Am I correct in assuming its coming from a function generated by createInvokeFunction in response of an invoke_xx import? here: https://github.com/emscripten-core/emscripten/blob/ec1cbf026077cd42f6247a21f2d9a4fe507cbda3/src/library_dylink.js#L251-L255 — Reply to this email directly, view it on GitHub <#16693 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCVWKUNOSCTLAR66YFXSR3VEWY7ZANCNFSM5TBC4D3Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

sbc100 · 2022-04-12T17:56:48Z

Yeah: we don't want -sWASM_BIGINT because it doesn't work on Safari 14 and we haven't made up our mind that we are officially not supporting Safari 14.

Sorry, can you explain a little more. I'm confused because this patch seems to rely on wasm/BigInt integration (you are passing BigInts as arguments and receiving them as return values to/from wasm functions) which is what I thought that the WASM_BIGINT assumed. Or is WASM_BIGINT assuming something more that just this?

hoodmane · 2022-04-12T18:02:52Z

According to caniuse, Safari 14.0 has BigInt but not BigInt64Array. So we are happy to assume the former is present. -sWASM_BIGINT assumes both.

…

On Tue, Apr 12, 2022, 10:57 Sam Clegg ***@***.***> wrote: Yeah: we don't want -sWASM_BIGINT because it doesn't work on Safari 14 and we haven't made up our mind that we are officially not supporting Safari 14. Sorry, can you explain a little more. I'm confused because this patch seems to rely on wasm/BigInt integration (you are passing BigInts as arguments and receiving them as return values to/from wasm functions) which is what I thought that the WASM_BIGINT assumed. Or is WASM_BIGINT assuming something more that just this? — Reply to this email directly, view it on GitHub <#16693 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCVWKVY7VQY7Z7H6TPFNHTVEW2OZANCNFSM5TBC4D3Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

sbc100 · 2022-04-12T18:06:22Z

According to caniuse, Safari 14.0 has BigInt but not BigInt64Array. So we are happy to assume the former is present. -sWASM_BIGINT assumes both.

In that case can we fix this by making WASM_BIGINT not assume BigInt64Array? or at least making it optional? I seems like BigInt64Array support is unrelated to wasm/bigint integration which is what that setting is designed for.

hoodmane · 2022-04-12T18:11:01Z

In that case can we fix this by making WASM_BIGINT not assume BigInt64Array?

That would be great! Yeah if you just checked if typeof BigInt64Array !== "undefined" and skipped making HEAP64 and HEAPU64 if not I think it would fix the problem for us. Though I haven't grepped the code base just now maybe there are other spots you use it?

kripken · 2022-04-13T17:42:36Z

Grepping for HEAP64 / HEAPU64, it is used in a few places, like embind's extra i64/u64 integration support. That might be optional if the user avoids it. The other place is pthread proxying, where I'm not sure it is as simple to avoid - we use i64s in more places and need to proxy them properly. But in general, we may start to use HEAP64 in more places, so working around it in each of them seems less good.

I think we could do this, though, with a little work. We could add a "BigInt but without BigInt[U]64Array" mode in which a custom acorn optimizer pass is run that rewrites HEAP[U]64 to operations on a smaller size. We have similar things like littleEndianHeap(), asanify(), safeHeap() which rewrite all accesses (over here).

sbc100 · 2022-04-13T18:17:53Z

Grepping for HEAP64 / HEAPU64, it is used in a few places, like embind's extra i64/u64 integration support. That might be optional if the user avoids it. The other place is pthread proxying, where I'm not sure it is as simple to avoid - we use i64s in more places and need to proxy them properly. But in general, we may start to use HEAP64 in more places, so working around it in each of them seems less good.

I think we could do this, though, with a little work. We could add a "BigInt but without BigInt[U]64Array" mode in which a custom acorn optimizer pass is run that rewrites HEAP[U]64 to operations on a smaller size. We have similar things like littleEndianHeap(), asanify(), safeHeap() which rewrite all accesses (over here).

Could we write a BigInt64Array polyfill that kicks in only when targeting older safari versions?

hoodmane · 2022-04-13T18:24:59Z

Could we write a BigInt64Array polyfill that kicks in only when targeting older safari versions?

Yeah that sounds like the best idea. It should be quite simple.

sbc100 · 2022-04-13T18:26:31Z

Could we write a BigInt64Array polyfill that kicks in only when targeting older safari versions?

Yeah that sounds like the best idea. It should be quite simple.

We would probably also want to bump the default safari version to 15 (perhaps at least when bitint is enabled).. so that folks have to explicitly opt into the polyfil.

kripken · 2022-04-14T16:15:02Z

Maybe there is JS magic that I am unaware of, but I'm not sure a polyfill can work. We need to replace HEAP64[x] with a function call. Can a Proxy object do it perhaps? Anyhow, if it's possible then a polyfill sounds best!

sbc100 · 2022-04-14T16:22:32Z

Maybe there is JS magic that I am unaware of, but I'm not sure a polyfill can work. We need to replace HEAP64[x] with a function call. Can a Proxy object do it perhaps? Anyhow, if it's possible then a polyfill sounds best!

Apparently that is possible yes: https://stackoverflow.com/questions/1711357/how-would-you-overload-the-operator-in-javascript

hoodmane · 2022-06-14T15:36:14Z

Closed in favor of #17103.

As discussed in #16693, -s WASM_BIGINT requires both BigInt (present since Safari 14.0) and BigInt64Array (present since Safari 15.0). Thus, users that want to target Safari 14.0 need a polyfill for BigInt64Array, which this adds.

hoodmane · 2022-10-11T08:49:20Z

-sWASM_BIGINT always crashes on startup if BigInt64Array is missing whereas this would only break certain rare side modules that hit this code path if BigInt is missing. But I think we are already assuming that BigInt is present. On Tue, Apr 12, 2022, 11:02 Hood Chatham ***@***.***> wrote:

…

According to caniuse, Safari 14.0 has BigInt but not BigInt64Array. So we are happy to assume the former is present. -sWASM_BIGINT assumes both. On Tue, Apr 12, 2022, 10:57 Sam Clegg ***@***.***> wrote: > Yeah: we don't want -sWASM_BIGINT because it doesn't work on Safari 14 > and we haven't made up our mind that we are officially not supporting > Safari 14. > > Sorry, can you explain a little more. I'm confused because this patch > seems to rely on wasm/BigInt integration (you are passing BigInts as > arguments and receiving them as return values to/from wasm functions) which > is what I thought that the WASM_BIGINT assumed. Or is WASM_BIGINT > assuming something more that just this? > > — > Reply to this email directly, view it on GitHub > <#16693 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACCVWKVY7VQY7Z7H6TPFNHTVEW2OZANCNFSM5TBC4D3Q> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

hoodmane changed the title ~~Fix dyncall and dynamic linking~~ Fix dyncall and dynamic linking with -sMAIN_MODULE=1 Apr 11, 2022

kripken reviewed Apr 11, 2022

View reviewed changes

rth mentioned this pull request Apr 18, 2022

Cryptography v36.0.2 w/ Rust pyodide/pyodide#2378

Merged

hoodmane mentioned this pull request May 30, 2022

Add BigInt64Array shim for Safari 14 #17103

Merged

hoodmane closed this Jun 14, 2022

hoodmane mentioned this pull request Jun 28, 2022

dynCallLegacy: Fill in dynCall_sig with a Wasm adaptor if it is missing #17328

Merged

hoodmane mentioned this pull request Oct 16, 2024

Emscripten: link with -sWASM_BIGINT rust-lang/rust#131736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dyncall and dynamic linking with -sMAIN_MODULE=1 #16693

Fix dyncall and dynamic linking with -sMAIN_MODULE=1 #16693

hoodmane commented Apr 10, 2022

kripken Apr 11, 2022

hoodmane Apr 11, 2022

sbc100 commented Apr 12, 2022

sbc100 commented Apr 12, 2022

hoodmane commented Apr 12, 2022 via email

sbc100 commented Apr 12, 2022

hoodmane commented Apr 12, 2022 via email

sbc100 commented Apr 12, 2022

hoodmane commented Apr 12, 2022

kripken commented Apr 13, 2022

sbc100 commented Apr 13, 2022

hoodmane commented Apr 13, 2022

sbc100 commented Apr 13, 2022

kripken commented Apr 14, 2022

sbc100 commented Apr 14, 2022

hoodmane commented Jun 14, 2022

hoodmane commented Oct 11, 2022 via email

Fix dyncall and dynamic linking with -sMAIN_MODULE=1 #16693

Fix dyncall and dynamic linking with -sMAIN_MODULE=1 #16693

Conversation

hoodmane commented Apr 10, 2022

kripken Apr 11, 2022

Choose a reason for hiding this comment

hoodmane Apr 11, 2022

Choose a reason for hiding this comment

sbc100 commented Apr 12, 2022

sbc100 commented Apr 12, 2022

hoodmane commented Apr 12, 2022 via email

sbc100 commented Apr 12, 2022

hoodmane commented Apr 12, 2022 via email

sbc100 commented Apr 12, 2022

hoodmane commented Apr 12, 2022

kripken commented Apr 13, 2022

sbc100 commented Apr 13, 2022

hoodmane commented Apr 13, 2022

sbc100 commented Apr 13, 2022

kripken commented Apr 14, 2022

sbc100 commented Apr 14, 2022

hoodmane commented Jun 14, 2022

hoodmane commented Oct 11, 2022 via email