Skip to content

ASYNCIFY doesn't work with WASM_BIGINT #17969

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.

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

Closed
sbc100 opened this issue Oct 1, 2022 · 4 comments
Closed

ASYNCIFY doesn't work with WASM_BIGINT #17969

sbc100 opened this issue Oct 1, 2022 · 4 comments

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Oct 1, 2022

When asyncify does its doRewind it calls back into the original entry point without passing any argument at all:

return start();

If the original function takes and i64 argument and WASM_BIGINT is enabled this call will always fail with:

TypeError: Cannot convert undefined to a BigInt

For example the following test currently fails:

  @with_asyncify_and_stack_switching                                                                 
  def test_async_bigint(self):                                                                       
    self.set_setting('WASM_BIGINT')                                                                  
    self.node_args += ['--experimental-wasm-bigint']                                                 
    create_file('main.c',  r'''                                                                      
#include <stdint.h>                                                                                  
#include <stdio.h>                                                                                   
#include <emscripten.h>                                                                              
                                                                                                     
EMSCRIPTEN_KEEPALIVE                                                                                 
void foo(uint64_t arg) {                                                                             
  printf("in foo: %lld\n", arg);                                                                     
  emscripten_sleep(100);                                                                             
  printf("back in foo: %lld\n", arg);                                                                
}                                                                                                    
''')                                                                                                 
                                                                                                     
    create_file('pre.js', 'Module["onRuntimeInitialized"] = () => Module._foo(42n);')                
    self.do_runf('main.c', 'in foo: 42\nback in foo: 42\n', emcc_args=['--pre-js', 'pre.js'])  

Normally this is not a huge deal because maybe not many folks use WASM_BIGINT and have entry points that take i64. However with MEMORY64 its very common because WASM_BIGINT is always enabled and all pointer arguments are i64.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 1, 2022

Even though this issue is not specific to MEMORY64 its currently blocking the implementation of ASYNCIFY with MEMORY64.

@sbc100 sbc100 added the wasm64 label Oct 1, 2022
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 1, 2022

The solution I'm looking at is to record not just the function name but also its signature Asyncify.exportCallStack. I guess we could either store that full original argument... or we could just store the signature which would allow us to create a new all-zero set of arguments.

This is another example of how difficult it is that the wasm bigint integration doesn't cooerce undefined to a zero i64 automatically. I wonder if its worth pushing back on that on the wasm bigint integration spec: WebAssembly/JS-BigInt-integration#12

sbc100 added a commit to WebAssembly/binaryen that referenced this issue Oct 1, 2022
The emscripten side is a little tricky but I've got some tests passing.
Currently blocked on:
emscripten-core/emscripten#17969
@kripken
Copy link
Member

kripken commented Oct 3, 2022

I'm not sure, but storing all the call arguments might be simpler, I suspect. We have wrappers anyhow for all exports, so we could make those exports stash a copy of .arguments together with the export name, or maybe store them both in a new closure that uses the copied arguments etc. Calling with the same arguments again is also kind of logical, in a way - in principle the called code could use them (but Asyncify is careful not to atm).

sbc100 added a commit to WebAssembly/binaryen that referenced this issue Oct 4, 2022
The emscripten side is a little tricky but I've got some tests passing.
Currently blocked on:
emscripten-core/emscripten#17969
sbc100 added a commit to WebAssembly/binaryen that referenced this issue Oct 4, 2022
The emscripten side is a little tricky but I've got some tests passing.
Currently blocked on:
emscripten-core/emscripten#17969
sbc100 added a commit to WebAssembly/binaryen that referenced this issue Oct 4, 2022
The emscripten side is a little tricky but I've got some tests passing.
Currently blocked on:
emscripten-core/emscripten#17969
sbc100 added a commit to WebAssembly/binaryen that referenced this issue Oct 4, 2022
The emscripten side is a little tricky but I've got some tests passing.
Currently blocked on:
emscripten-core/emscripten#17969
sbc100 added a commit to WebAssembly/binaryen that referenced this issue Oct 5, 2022
The emscripten side is a little tricky but I've got some tests passing.
Currently blocked on:
emscripten-core/emscripten#17969
@sbc100 sbc100 added this to the Wasm64 all tests passing milestone Oct 19, 2022
@sbc100
Copy link
Collaborator Author

sbc100 commented Feb 28, 2024

Fixed in #19913

@sbc100 sbc100 closed this as completed Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants