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

[jspi] Fix TSD generation when using asyncify library functions. #23419

Merged
merged 1 commit into from
Jan 16, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
[jspi] Fix TSD generation when using asyncify library functions.
Instead of disabling JSPI during TSD generation just skip instrumenting
the wasm exports and imports so JSPI isn't needed.

Fixes #23418
brendandahl committed Jan 15, 2025
commit e0a22ec059f21c22bc7569ffa84c65a74f6448ae
8 changes: 8 additions & 0 deletions src/library_async.js
Original file line number Diff line number Diff line change
@@ -38,6 +38,10 @@ addToLibrary({
rewindArguments: {},
#endif
instrumentWasmImports(imports) {
#if EMBIND_GEN_MODE
// Instrumenting is not needed when generating code.
return imports;
#endif
#if ASYNCIFY_DEBUG
dbg('asyncify instrumenting imports');
#endif
@@ -103,6 +107,10 @@ addToLibrary({
},
#endif
instrumentWasmExports(exports) {
#if EMBIND_GEN_MODE
// Instrumenting is not needed when generating code.
return exports;
#endif
#if ASYNCIFY_DEBUG
dbg('asyncify instrumenting exports');
#endif
4 changes: 4 additions & 0 deletions src/settings_internal.js
Original file line number Diff line number Diff line change
@@ -92,6 +92,10 @@ var EMBIND = false;
// Whether a TypeScript definition file has been requested.
var EMIT_TSD = false;

// This will be true during the generation of code in run_embind_gen. Helpful
// for detecting if either TSD file or embind AOT JS generation is running.
var EMBIND_GEN_MODE = false;

// Whether the main() function reads the argc/argv parameters.
var MAIN_READS_PARAMS = true;

5 changes: 4 additions & 1 deletion test/other/embind_tsgen_jspi.cpp
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
#include <emscripten/bind.h>
#include <emscripten.h>

using namespace emscripten;

void sleep() {}
void sleep() {
emscripten_sleep(0);
}

EMSCRIPTEN_BINDINGS(Test) {
function("sleep", &sleep, async());
15 changes: 1 addition & 14 deletions test/other/embind_tsgen_jspi.d.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,9 @@
// TypeScript bindings for emscripten-generated code. Automatically generated at compile time.
declare namespace RuntimeExports {
let HEAPF32: any;
let HEAPF64: any;
let HEAP_DATA_VIEW: any;
let HEAP8: any;
let HEAPU8: any;
let HEAP16: any;
let HEAPU16: any;
let HEAP32: any;
let HEAPU32: any;
let HEAP64: any;
let HEAPU64: any;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is a side effect of adding -sSTRICT? Is that related to this change somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, strict will remove those definitions. The reporters issue only happened in strict mode, since it turns on erroring on undefined symbols.

interface WasmModule {
}

interface EmbindModule {
sleep(): Promise<void>;
}

export type MainModule = WasmModule & typeof RuntimeExports & EmbindModule;
export type MainModule = WasmModule & EmbindModule;
2 changes: 1 addition & 1 deletion test/test_other.py
Original file line number Diff line number Diff line change
@@ -3530,7 +3530,7 @@ def test_embind_tsgen_memory64(self):
@requires_jspi
def test_embind_tsgen_jspi(self):
self.run_process([EMXX, test_file('other/embind_tsgen_jspi.cpp'),
'-lembind', '--emit-tsd', 'embind_tsgen_jspi.d.ts', '-sJSPI'] +
'-lembind', '--emit-tsd', 'embind_tsgen_jspi.d.ts', '-sJSPI', '-sSTRICT', '--no-entry'] +
self.get_emcc_args())
self.assertFileContents(test_file('other/embind_tsgen_jspi.d.ts'), read_file('embind_tsgen_jspi.d.ts'))

6 changes: 1 addition & 5 deletions tools/link.py
Original file line number Diff line number Diff line change
@@ -1964,6 +1964,7 @@ def run_embind_gen(wasm_target, js_syms, extra_settings, linker_inputs):
# Save settings so they can be restored after TS generation.
original_settings = settings.backup()
settings.attrs.update(extra_settings)
settings.EMBIND_GEN_MODE = True

if settings.MAIN_MODULE and linker_inputs:
# Copy libraries to the temp directory so they can be used when running
@@ -2001,11 +2002,6 @@ def run_embind_gen(wasm_target, js_syms, extra_settings, linker_inputs):
setup_environment_settings()
# Use a separate Wasm file so the JS does not need to be modified after emscripten.emscript.
settings.SINGLE_FILE = False
if settings.ASYNCIFY == 2:
# JSPI is not needed to generate the definitions.
# TODO: when the emsdk node version supports JSPI, it probably makes more sense
# to enable it in node than disabling the setting here.
settings.ASYNCIFY = 0
# Embind may be included multiple times, de-duplicate the list first.
settings.JS_LIBRARIES = dedup_list(settings.JS_LIBRARIES)
# Replace embind with the TypeScript generation version.