Handle wildcards in ASYNCIFY_EXPORTS when intrumenting exports

[mboc-qt]

Binaryen already handles wildcards sent to it via jspi-exports, which is derived from ASYNCIFY_EXPORTS. Sync that with instrumentWasmExports, which (until now) has been ignoring the wildcards.

[mboc-qt]

I couldn't find a suitable test for this. Should I write a new one? Where should that be, test_other, test_browser, somewhere else?

[kripken]

For a test, maybe in browser, and near the one modified in #19280?

[mboc-qt]

Test created

[mboc-qt]

Tests flaked I think... ?

[sbc100]

I don't think you need the outer new RegExp.. you can just use / at the beginning and end, no? Like in the other locations where we do this?

[sbc100]

This seems rather hacky.. i would hope we could find a better way? But its hard for me to say without debugging the the code why this would be needed?

[mikolaj-boc]

Test replaced

[sbc100]

Does this need to be a browser test? Can it go in test_other.py or test_core.py? @brendandahl WDYT? Does this require a completely new test or can we modify an existing one?

[brendandahl]

test_other.py is probably more appropriate (see test_embind_jspi for an example).

[mboc-qt]

Tried moving it there, but see my other comment below - no easy way to check if a function has been instrumented or not.

[mikolaj-boc]

Moved to test_other with help from @brendandahl

[sbc100]

Maybe just make this file a .c file and then you don't need the extern "C" here.

Also the indention here looks wrong. 2 space indents everywhere is the convention I think.

[mikolaj-boc]

Done

[sbc100]

Can you use btest_exit rather than btest? Then you don't need the REPORT_RESULT macro at all. Its preferable in most cases.

[mikolaj-boc]

Done

[brendandahl]

I think the test can be simplified a bit, we have other tests for more of the functionality. The main thing we need to test is if the exports get instrumented. How about:

1. export three c functions
2. have the wild card match 2
3. in an EM_ASYNC_JScall the 2 that should match and assert that they return promises. Also, call the third and assert it returns a value.

[mboc-qt]

It turns out that instrumenting functions does not necessarily make them return promises.
Taking a look at instrumentWasmExports in library_async.js, instrumenting just changes the function to one that calls Asyncify.exportCallStack.push/pop (ASYNCIFY=1) or wraps it with one that has o promising parameter (ASYNCIFY=2) - so there is no easy way to check by simply verifying if the return value is a promise.
Also, if we wanted to check if the promising parameter has been added, there's also no easy way of doing this in a test, because we have no access to the original unwrapped WASM function, since it is wrapped in a js function in instrumentWasmExports.
We could check that in case of a MAIN_MODULE, because then we have access to export.orig (see ret[x].orig = original; in library_async.js), but I don't know if that would be a correct approach for a unittest.

[brendandahl]

We're only testing ASYNCIFY=2 so the instrumented function should always return a promise. I was thinking something like:

#include <emscripten.h>

EM_ASYNC_JS(int, test, (), {
  console.log(Module._async1() instanceof Promise);
  console.log(Module._async2() instanceof Promise);
  console.log(!(Module._sync() instanceof Promise));
});

EMSCRIPTEN_KEEPALIVE int async1() {
  return 99;
}

EMSCRIPTEN_KEEPALIVE int async2() {
  return 99;
}

EMSCRIPTEN_KEEPALIVE int sync() {
  return 99;
}

int main() {
  test();
}

[mboc-qt]

That's what I did - but the problem is that instrumenting alone does not make the function return a promise automatically.
See library_async.js' Asyncify.instrumentWasmImports:
the only thing it does is add the first parameter of 'externref' and adding the suspending: 'first' parameter to WebAssembly.Function. The return type from the call to sigToWasmTypes is left intact.

[brendandahl]

Are you seeing the code not actually return a promise? There were older versions of v8 where it didn't always return promise, but newer versions should always return a promise. The return type will be what the promise resolves with.

[brendandahl]

I don't think we need to test the instrumentation itself, just that it has been instrumented. If it wasn't instrumented it will not return a promise.

[mikolaj-boc]

That's what I actually wanted to say; instrumenting does not make it return a promise, and adding emscripten_sleep(0); will always make it return a promise, regardless of instrumentation.
Since all we want here is to check if functions were instrumented, and a promise being returned is not a result of instrumentation, I cannot think of any viable way of testing this.

[brendandahl]

Here's what I had in mind. This test passes with the PR and fails without the PR.

diff --git a/test/other/jspi_wildcard.c b/test/other/jspi_wildcard.c
new file mode 100644
index 000000000..e4e07afa4
--- /dev/null
+++ b/test/other/jspi_wildcard.c
@@ -0,0 +1,31 @@
+#include <emscripten.h>
+#include <stdio.h>
+
+EM_ASYNC_JS(int, test, (), {
+  let promise1 = Module._async1();
+  assert(promise1 instanceof Promise);
+  await promise1;
+  let promise2 = Module._async2();
+  assert(promise2 instanceof Promise);
+  await promise2;
+  assert(!(Module._sync() instanceof Promise));
+});
+
+EMSCRIPTEN_KEEPALIVE int async1() {
+  emscripten_sleep(0);
+  return 99;
+}
+
+EMSCRIPTEN_KEEPALIVE int async2() {
+  emscripten_sleep(0);
+  return 99;
+}
+
+EMSCRIPTEN_KEEPALIVE int sync() {
+  return 99;
+}
+
+int main() {
+  test();
+  printf("done\n");
+}
diff --git a/test/test_other.py b/test/test_other.py
index d8e1916d2..6853a0bdf 100644
--- a/test/test_other.py
+++ b/test/test_other.py
@@ -2868,6 +2868,13 @@ int f() {
     self.assertContained('Constructed from JS destructed', output)
     self.assertNotContained('Foo* destructed', output)
 
+  def test_jspi_wildcard(self):
+    self.require_v8()
+    self.v8_args.append('--experimental-wasm-stack-switching')
+    self.emcc_args += ['-g', '-sASYNCIFY=2', '-sASYNCIFY_EXPORTS=async*', '-Wno-experimental']
+
+    self.do_runf(test_file('other/jspi_wildcard.c'), 'done')
+
   def test_emconfig(self):
     output = self.run_process([emconfig, 'LLVM_ROOT'], stdout=PIPE).stdout.strip()
     self.assertEqual(output, config.LLVM_ROOT)

[mikolaj-boc]

Thanks, yes - it follows the same logic as my first test. As long as we assume that we are okay with not asserting on the exact error message:

caught (in promise) RuntimeError: invalid suspender object for suspend
at emscripten_sleep (main.wasm:0x4aa1)
at async1 (test_jspi_wildcard.c:20:3)
at export$async1 (main.wasm:0x4a7c)
at ret. (main.js:1404:35)
at Object._async1 (main.js:730:22)
at main.js:1017:96
at main.js:1424:11
at main.js:1415:11

then I'm okay with that.

[mikolaj-boc]

(test pushed, only slightly changed)