emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.41k stars 3.26k forks source link

In ASYNCIFY=2 build mode function arguments come in as nulls #20490

Open juj opened 9 months ago

juj commented 9 months ago

a.cpp

#include <emscripten.h>
struct options
{
    int x;
};

extern "C" int navigator_gpu_request_adapter_sync(options *opts);

int main()
{
    options opts = { 100 };
    int adapter = navigator_gpu_request_adapter_sync(&opts);
    EM_ASM(console.log(`Got adapter ${0}`), adapter);
}

lib_a.js

mergeInto(LibraryManager.library, {
  navigator_gpu_request_adapter_sync__sig: 'ip',
  navigator_gpu_request_adapter_sync__async: true,
  navigator_gpu_request_adapter_sync: function(opts) {
    console.error(`navigator_gpu_request_adapter_sync: opts: ${opts}`);
    if (!opts) throw 'opts cannot be null';
    var promise = navigator.gpu.requestAdapter().then(a => { return 42; });
    return Asyncify.handleAsync(() => {
      return promise;
    });
  }
});
em++ a.cpp --js-library lib_a.js -sASYNCIFY=1 -sASYNCIFY_IMPORTS=['navigator_gpu_request_adapter_sync'] -o a.html

works as expected, but

em++ a.cpp --js-library lib_a.js -sASYNCIFY=2 -sASYNCIFY_IMPORTS=['navigator_gpu_request_adapter_sync'] -o a.html

doesn't, and instead raises the exception opts cannot be null.

Tested in Chrome Canary 120.0.6076.0 (Official Build) canary (64-bit) on Windows.

I find there's not much documentation on JSPI, but based on the test suite code, it looks like it should work by just replacing -sASYNCIFY=1 with -sASYNCIFY=2 (and adding the __sig directives)?

juj commented 9 months ago

em++ a.cpp --js-library lib_a.js -sASYNCIFY=1 -sASYNCIFY_IMPORTS=['navigator_gpu_request_adapter_sync'] -o a.html works as expected

Actually, looks like ASYNCIFY=1 does not work like I expect it to. What I am seeing is that the function navigator_gpu_request_adapter_sync() ends up being called twice:

image

I wonder if I've got the syntax wrong? Although I see I first implemented ASYNCIFY=1 support into wasm_webgpu two years ago, and back then I did not experience this double call mechanism - and the code has not changed since. That is a bit puzzling..

RReverser commented 9 months ago

@juj The problem is that Asyncify.handleAsync must wrap the entire import. The wrapper is what takes care of imports being called twice - once for actual call before unwind and once for returning result during rewind - and ensures that the JS function inside is actually called only once like a regular JS function.

RReverser commented 9 months ago

Something like this should work as expected (made couple more minor changes along the way):

mergeInto(LibraryManager.library, {
  navigator_gpu_request_adapter_sync__sig: 'ip',
  navigator_gpu_request_adapter_sync__async: true,
  navigator_gpu_request_adapter_sync: (opts) => Asyncify.handleAsync(async () => {
    console.error(`navigator_gpu_request_adapter_sync: opts: ${opts}`);
    if (!opts) throw new Error('opts cannot be null');
    await navigator.gpu.requestAdapter();
    return 42;
  }),
});
juj commented 9 months ago

The problem is that Asyncify.handleAsync must wrap the entire import.

Thanks. This requirement is only for JSPI, and not for ASYNCIFY=1 right?

Would that form of code work on both ASYNCIFY=1 and ASYNCIFY=2?

RReverser commented 9 months ago

This requirement is only for JSPI, and not for ASYNCIFY=1 right?

No, that's for both. In JSPI it matters less actually because it does less work in the wrapper (only runtime keepalive counter tracking), but it's still useful to keep it for both.

RReverser commented 9 months ago

For the sake of another example, this is how we implement internal functions too, e.g.: https://github.com/emscripten-core/emscripten/blob/a8e9dd5ee2fcb87e8cbee7e9a63be78d6a83a474/src/embind/emval.js#L455-L461 or https://github.com/emscripten-core/emscripten/blob/a8e9dd5ee2fcb87e8cbee7e9a63be78d6a83a474/src/library_idbstore.js#L95-L113

juj commented 9 months ago

The problem is that Asyncify.handleAsync must wrap the entire import.

How do I handle parameters that are passed in to the function on the stack?

If I do

  navigator_gpu_request_adapter_sync: function(opts) {
    return Asyncify.handleAsync(() => {
      let someOption = HEAPU32[opts>>2];
      ...
    });
  }

is that first part executed synchronously before yielding back from the function? (so that I know that the caller's stack is still intact?)

juj commented 9 months ago

Something like this should work as expected

I find that with that example, I do still get the error Error: opts cannot be null when building with -sASYNCIFY=2.

juj commented 9 months ago

is that first part executed synchronously before yielding back from the function?

Hmm looks like that is the case.. so all good there.

RReverser commented 9 months ago

How do I handle parameters that are passed in to the function on the stack?

From the PoV of JS function inside, parameters should always just be there and don't need special handling.

They're captured by closure + Wasm itself is suspended so pointers are always valid during the entire lifetime of the async function, both before and after suspending.

juj commented 9 months ago

Alright, I was able to get -sASYNCIFY=1 to work by wrapping the code over the whole function (https://github.com/juj/wasm_webgpu/commit/750df43a0e23071584ad88fcb721dde53482052a). Although there is one issue, I'll open a separate ticket about that.

Still struggling to get -sASYNCIFY=2 to work, I am unable to get function parameters into the asyncified functions.

RReverser commented 9 months ago

Yeah sorry can't help with JSPI, I only played with it briefly and also ran into couple of bugs before.

RReverser commented 9 months ago

Side-note: if you're not limited by very old browsers (which, in case of WebGPU, I can only assume you're not), I'd really recommend to use async-await over .then. Aside from readability, thanks to sequential nature and avoiding closures, it can be better optimised by modern engines and results in shallower and more readable stacktraces in DevTools and errors. https://v8.dev/blog/fast-async

Emscripten already has problems with very deep stacktraces due to usage of .then to simulate async loops (e.g. in proxying and Asyncify itself) but Emscripten is limited by having to support old engines, so can't use native async-await everywhere. But for a project targeting specifically modern browsers anyway it shouldn't be an issue.

juj commented 9 months ago

Thanks, I didn't know that there were any functional differences with these keywords. I thought they were just syntactic sugar. I'll look into migrating the code over.