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

Support custom import namespace for functions implemented in JavaScript #20035

Open
toyobayashi opened this issue Aug 15, 2023 · 9 comments
Open

Comments

@toyobayashi
Copy link
Contributor

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: nodejs/node#49037

@sbc100
Copy link
Collaborator

sbc100 commented Aug 15, 2023

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
Copy link
Contributor Author

@sbc100 Currently emnapi is maintaining a modified version of Node-API headers. In this PR nodejs/node#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:

  • 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.

    import init from './node-api-wasm-module-compiled-by-emcc.js'
    
    await init({
       instantiateWasm (imports, successCallback) {
        imports.napi = imports.env
        WebAssembly.instantiate(..., imports).then(({ instance, module }) => {
          successCallback(instance, module)
        })
        return {}
      }
    })
  • Request user manually define NAPI_EXTERN to __attribute__((__import_module__("env"))) on a per-build basis.

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

@sbc100
Copy link
Collaborator

sbc100 commented Aug 15, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

toyobayashi commented Aug 21, 2023

@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?

emscripten/src/preamble.js

Lines 932 to 946 in 097290d

var info = {
#if MINIFY_WASM_IMPORTED_MODULES
'a': wasmImports,
#else // MINIFY_WASM_IMPORTED_MODULES
'env': wasmImports,
'{{{ WASI_MODULE_NAME }}}': wasmImports,
#endif // MINIFY_WASM_IMPORTED_MODULES
#if SPLIT_MODULE
'placeholder': new Proxy({}, splitModuleProxyHandler),
#endif
#if RELOCATABLE
'GOT.mem': new Proxy(wasmImports, GOTHandler),
'GOT.func': new Proxy(wasmImports, GOTHandler),
#endif
};

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
Copy link
Contributor

hoodmane commented Aug 28, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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.

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

3 participants