Skip to content
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

Can't return bigint from async function in Asyncify #23441

Open
kainino0x opened this issue Jan 17, 2025 · 9 comments
Open

Can't return bigint from async function in Asyncify #23441

kainino0x opened this issue Jan 17, 2025 · 9 comments

Comments

@kainino0x
Copy link
Collaborator

kainino0x commented Jan 17, 2025

This works in JSPI but is broken in Asyncify. The problem seems to be somewhere in Asyncify.handleAsync.

Version of emscripten/emsdk:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 4.0.1 (89ce854a99238d04116a3d9b5afe241eec90c6c3)
clang version 20.0.0git (https:/github.com/llvm/llvm-project a32e36faf84bd7da3df0c7d50bb9020568128417)
Target: wasm32-unknown-emscripten
Thread model: posix

Failing command line in full:
Repro here: https://github.com/kainino0x/asyncify-bigint-bug
(live here http://kai.graphics/asyncify-bigint-bug/main_asyncify_bigint.html)
Just call make.
The args are in the makefile:

all:

ARGS := -g -sASYNCIFY_IMPORTS=my_async_i64,my_async_u64

# These don't work
main_asyncify_bigint.html: main.cpp Makefile
	em++ main.cpp -o $@ ${ARGS} -sASYNCIFY=1 -sWASM_BIGINT=1
main_asyncify_wasm64.html: main.cpp Makefile
	em++ main.cpp -o $@ ${ARGS} -sASYNCIFY=1 -sMEMORY64=1

# These do work
main_jspi_bigint.html: main.cpp Makefile
	em++ main.cpp -o $@ ${ARGS} -sJSPI=1 -sWASM_BIGINT=1
main_jspi_wasm64.html: main.cpp Makefile
	em++ main.cpp -o $@ ${ARGS} -sJSPI=1 -sMEMORY64=1

Code:

EM_JS(int64_t, my_async_i64, (), {
  return Asyncify.handleAsync(async () => {
    return -123n; // bigint
  });
});

EM_JS(uint64_t, my_async_u64, (), {
  return Asyncify.handleAsync(async () => {
    return -123n; // bigint
  });
});

int main() {
    int64_t i = my_async_i64();
    uint64_t u = my_async_u64();
    printf("got: %lli %llu\n", i, u);
}

Full link command and output with -v appended:

There isn't anything notable in the link output, the problem is a crash:

Uncaught TypeError: Cannot convert 0 to a BigInt
    at main_asyncify_bigint.wasm.__original_main (main_asyncify_bigint.wasm:0x580)
    at main_asyncify_bigint.wasm.main (main_asyncify_bigint.wasm:0x780)
    at ret.<computed> (main_asyncify_bigint.js:1273:24)
    at main_asyncify_bigint.js:718:12
    at callMain (main_asyncify_bigint.js:1832:15)
    at doRun (main_asyncify_bigint.js:1884:24)
    at main_asyncify_bigint.js:1893:7
@kainino0x
Copy link
Collaborator Author

BTW that's the error from Chrome, but Firefox gives approximately the same error.

@kripken
Copy link
Member

kripken commented Jan 17, 2025

The problem here is that Asyncify returns twice: the first time we call the sleeping function, it exits "early", and we just unwind the stack. Then, when we resume, we rewind the stack, and then it exits normally, with its proper return value. The "early" exit's return value is never read - the runtime sees we unwound just to sleep - but here the problem is that we return 0 or such, a number, and VMs trap on that not being a bigint, because the wasm import is defined as returning an i64.

I'm not sure how to fix that. Asyncify is not aware of the signatures of functions. Perhaps we could get that information to it somehow? But these are EM_JSs, which have known issues here #16975

As a workaround, this works, and is basically what we'd need to do if we found a good way to use the type information:

function my_async_i64() {
  return BigInt(Asyncify.handleAsync( .. original code ..));
}

The only addition here is BigInt(), so we return a bigint both in the early return (which doesn't) and the normal one (which already does).

@sbc100 @brendandahl any ideas?

@sbc100
Copy link
Collaborator

sbc100 commented Jan 17, 2025

Hmm, yes, it seems like a variant of #16975. I guess we could add Asyncify.handleAsync64 variant? Or just add the BigInt cast explicitly.

@sbc100
Copy link
Collaborator

sbc100 commented Jan 17, 2025

Perhaps we should be collecting all these issues somewhere so we can one day pettition to relax the bigint coercion rules in JS. There so many issues like this that are caused by this huge inconsistency in JS.

@kripken
Copy link
Member

kripken commented Jan 17, 2025

Yeah, maybe these issues can all link to #16975, and some day we can make the case for not trapping here...

For now maybe we could document this. Do we have a bigint feature page in our docs, I don't remember?

@sbc100
Copy link
Collaborator

sbc100 commented Jan 17, 2025

#16975 is just the tip of the iceberg, the whole wasm64 target relies heavily to inject explicit coercions to avoid exposing users to bigint values. The whole i53 abit thing is only needed because of this JS weirdness.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Jan 17, 2025

FWIW I actually found this issue via --js-library, not EM_JS, but the problem seems to be the same. It was just for an internal function for WebGPU so I worked around it by returning double instead, which should be fine in this case.

I had to work around a bunch of #16975-type things too, but they were all library specific so I could work around them by generating a cast when necessary. Examples: https://dawn-review.googlesource.com/c/dawn/+/222035

globalThis.gpu = {
    NULLPTR: MEMORY64 ? '0n' : '0',
    passAsI64: value => WASM_BIGINT ? `BigInt(${value})` : value,
    passAsPointer: value => MEMORY64 ? `BigInt(${value})` : value,
    convertToPassAsPointer: variable => MEMORY64 ? `${variable} = BigInt(${variable});` : '',

@brendandahl
Copy link
Collaborator

For the EM_JS issue, it's probably be better to use EM_ASYNC_JS. Then we could make the generated JS code aware of the return type and add the conversion to BigInt automatically.

@kainino0x
Copy link
Collaborator Author

That would make for a neater usage pattern since you already know the return type in EM_ASYNC_JS, but I think the implementation would bottom out into handleAsync in a way that would solve it for EM_JS and --js-library too anyway.

copybara-service bot pushed a commit to google/dawn that referenced this issue Jan 21, 2025
- Change emwgpuWaitAny to return `double` as a workaround for
  emscripten-core/emscripten#23441
- Fix a bunch more pointer/i64 values that need to be BigInt in
  WASM_BIGINT/MEMORY64 builds. Found these just by searching for
  '0' or 'stringToUTF8OnStack', probably missed some. I tested a
  few by triggering error conditions but most are untested.
- A few random nits.

Tested with 40 different build configurations:
https://github.com/kainino0x/webgpu-cross-platform-demo/blob/5675031561e4134b2467cd2b7f054c32776001d3/build_all.sh#L60-L78

Bug: 389977397
No-Try: true
Change-Id: I0af67a347351be7002fc25e2157e3767f1e25be3
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/222314
Commit-Queue: Kai Ninomiya <[email protected]>
Reviewed-by: Loko Kung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants