emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.63k stars 3.28k forks source link

Support custom import namespace for functions implemented in JavaScript #20035

Open toyobayashi opened 1 year ago

toyobayashi commented 1 year ago

Can I specify a custom import namespace for JavaScript function?

mergeInto(LibraryManager.library, {
  napi_create_int32: function (env, value, result) {
    // ...
  },
  napi_create_int32__sig: 'ipip',
  napi_create_int32__namespace: 'napi' // <-- instead of 'env'
})

compile result

WebAssembly.instantiate(/* ... */, {
  napi: { napi_create_int32 }
})

Refs: https://github.com/nodejs/node/pull/49037

sbc100 commented 1 year ago

At this point we do not have any way to support this, but such a feature does seem potentially useful.

Can you explain why using env doesn't work in your case?

toyobayashi commented 1 year ago

@sbc100 Currently emnapi is maintaining a modified version of Node-API headers. In this PR https://github.com/nodejs/node/pull/49037 I want to make Node.js node-api official header can be used by emnapi, but Node.js intentionally already expands NAPI_EXTERN to __attribute__((__import_module__("napi"))) for all Node-API symbols and refuse to modify it to env.

https://github.com/nodejs/node/blob/9cc7327979a95a76662a5f044118bad1009fa5a8/src/js_native_api.h#L32-L34

For this, there are two options:

Both are breaking changes for users. So I'm wondering if emscripten could provide this feature.

sbc100 commented 1 year ago

Another option might be modify js_native_api.h upstream with an #ifdef __EMSCRIPTEN__ that uses __import_module__("env").

In the long term I can see that this would be useful feature in emscripten, I'm just not sure when we will have time to add such a thing.

toyobayashi commented 1 year ago

Another option might be modify js_native_api.h upstream with an #ifdef EMSCRIPTEN that uses __import_module__("env").

yeah I have done this but Node.js doesn‘t recommend doing this as you can see the comments in that PR so reverted those changes.

it will be really nice to add this feature.

toyobayashi commented 1 year ago

@sbc100 If the custom namespace cannot be implemented in a short time, as an alternative, could you consider moving the info definition outside the createWasm function?

https://github.com/emscripten-core/emscripten/blob/097290ded2746c6ba703d556fd53b23e5aef760a/src/preamble.js#L932-L946

to

// "imports" is the name used in MINIMAL_RUNTIME
var imports = {
  env: wasmImports,
  // ...
};

function createWasm() {
  // ...
}

So that users can add symbol to custom namespace via __postset

mergeInto(LibraryManager.library, {
  napi_create_int32: function (env, value, result) {
    // ...
  },
  napi_create_int32__sig: 'ipip',
  napi_create_int32__postset: '(imports.napi = imports.napi || {}).napi_create_int32 = napi_create_int32;'
})
hoodmane commented 1 year ago

Request user to provide a instantiateWasm hook to emscripten and add napi namespace during this hook call, which isn't what instantiateWasm are supposed to be used for, that force users to write loading logic themselves.

It would be nice to have a hook that just allows modifying imports. I don't have any issue with implementing a custom instantiateWasm function, but the documentation says it doesn't work with dwarf symbols or certain sanitizers. whereas if remapping imports is needed to load the modules then obviously it can't be turned off when dwarf symbols are added.

hoodmane commented 1 year ago

Oh but I guess #20130 is indeed an alternative solution where a hook isn't added. The benefit of the hook is that you could add code to be run after all other setup but before instantiation -- not sure if this is important. I have a patch to add a hook like this in Pyodide:

--- a/src/preamble.js
+++ b/src/preamble.js
@@ -937,6 +941,9 @@ function instantiateAsync(binary, binaryFile, imports, callback) {
 // Create the wasm instance.
 // Receives the wasm imports, returns the exports.
 function createWasm() {
+  if (Module.adjustWasmImports) {
+    Module.adjustWasmImports(wasmImports);
+  }
   // prepare imports
   var info = {
 #if MINIFY_WASM_IMPORTED_MODULES
toyobayashi commented 1 year ago

@hoodmane Unfortunately adding this hook does not satisfy my scenario, because I am the JavaScript library provider, this way still requires my user modify their code. What I want is that I as the library author can specify the import namespace in the JavaScript library file (--js-library).

hoodmane commented 1 year ago

Yeah it sounds like these are pretty different use cases -- I build the main module and have downstream users that make custom dynamically linked libraries, whereas your downstream wants to statically link your library.