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

Allow taking address of EM_JS functions with dynamic linking #18391

Merged
merged 1 commit into from
Dec 17, 2022

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 16, 2022

Fixes: #18370

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 16, 2022

Still working on a test...

@sbc100 sbc100 requested a review from kripken December 16, 2022 19:01
@sbc100 sbc100 enabled auto-merge (squash) December 16, 2022 19:18
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Nice it's relatively simple to do!

@sbc100 sbc100 disabled auto-merge December 17, 2022 20:31
@sbc100 sbc100 merged commit a742129 into main Dec 17, 2022
@sbc100 sbc100 deleted the main_module_em_js_func_addr branch December 17, 2022 20:31
@kleisauke
Copy link
Collaborator

This PR seems to cause a regression in the wasm-vips project during the final link stage:

building:ERROR: Closure compiler run failed:

building:ERROR: /tmp/emscripten_temp_0cy0oyma/vips.js.pp.jso3.js:4801:105: WARNING - [JSC_MISPLACED_ANNOTATION] Misplaced type annotation. Type annotations are not allowed here. Are you missing parentheses?
  4801|  if (ENVIRONMENT_IS_PTHREAD) return _emscripten_proxy_to_main_thread_js(8, 1, fd, /** @type {!BigInt} */ length);
                                                                                                                 ^^^^^^

/tmp/emscripten_temp_0cy0oyma/vips.js.pp.jso3.js:7690:106: WARNING - [JSC_MISPLACED_ANNOTATION] Misplaced type annotation. Type annotations are not allowed here. Are you missing parentheses?
  7690|  if (ENVIRONMENT_IS_PTHREAD) return _emscripten_proxy_to_main_thread_js(25, 1, fd, /** @type {!BigInt} */ offset, whence, newOffset);
                                                                                                                  ^^^^^^

/tmp/emscripten_temp_0cy0oyma/vips.js.pp.jso3.js:695:21: ERROR - [JSC_UNDEFINED_VARIABLE] variable unbox_small_structs is undeclared
  695|  var rtype_unboxed = unbox_small_structs(HEAPU32[(cif >> 2) + 3]);
                            ^^^^^^^^^^^^^^^^^^^

1 error(s), 2 warning(s)

(those JSC_MISPLACED_ANNOTATION warnings are probably not related)

This might be related to this line:
https://github.com/hoodmane/libffi-emscripten/blob/d0cf67273137f64c73329e805b3c7d94ed40d8fd/src/wasm32/ffi.c#L179

or this one:
https://github.com/hoodmane/libffi-emscripten/blob/d0cf67273137f64c73329e805b3c7d94ed40d8fd/src/wasm32/ffi.c#L433

Let me know if I should provide more information.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 22, 2022

Ah... I guess we don't have tests for EM_JS functions that call other EM_JS functions.

This change make EM_JS functions appear on the JS side with and underscore prefix, just like other native functions. Presumably changing the callsite to _unbox_small_structs would fix the error?

I wonder how common this case is? It would be a shame to have to declare them with both naming styles.

@kleisauke
Copy link
Collaborator

This change make EM_JS functions appear on the JS side with and underscore prefix, just like other native functions.

I see. Building with --disable-modules also seems to fail with:

wasm-ld: error: /home/kleisauke/wasm-vips/build/target/lib/libgobject-2.0.a(gclosure.c.o): undefined symbol: ffi_call
wasm-ld: error: /home/kleisauke/wasm-vips/build/target/lib/libgobject-2.0.a(gclosure.c.o): undefined symbol: ffi_call

(so that probably(?) means that it should reference _ffi_call instead)

For reference, I'm testing this in combination with commit kleisauke/wasm-vips@941de51.

Presumably changing the callsite to _unbox_small_structs would fix the error?

Let me investigate that as possible fix.

I wonder how common this case is?

AFAIK, it only affects Pyodide/Python ctypes and the wasm-vips project.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 22, 2022

This change make EM_JS functions appear on the JS side with and underscore prefix, just like other native functions.

I see. Building with --disable-modules also seems to fail with:

wasm-ld: error: /home/kleisauke/wasm-vips/build/target/lib/libgobject-2.0.a(gclosure.c.o): undefined symbol: ffi_call
wasm-ld: error: /home/kleisauke/wasm-vips/build/target/lib/libgobject-2.0.a(gclosure.c.o): undefined symbol: ffi_call

On the native side the symbols don't need the underscore prefix no. For example the native toolchain (i.e. C/C++ code) sees malloc but on the JS side this is know as _malloc or Module._malloc.

The reason for this is mostly historical.

What is --disable-modules?

@kleisauke
Copy link
Collaborator

What is --disable-modules?

Ah, sorry I should clarify that. The --disable-modules build parameter in wasm-vips will disable the build of dynamic modules (-sSIDE_MODULE=2 / -sMAIN_MODULE=2) and load-time dynamic linking (i.e. when linking with -sAUTOLOAD_DYLIBS=0). When this parameter is passed, modules are instead being statically linked in vips.wasm.

@kleisauke
Copy link
Collaborator

I see. Building with --disable-modules also seems to fail with:

wasm-ld: error: /home/kleisauke/wasm-vips/build/target/lib/libgobject-2.0.a(gclosure.c.o): undefined symbol: ffi_call
wasm-ld: error: /home/kleisauke/wasm-vips/build/target/lib/libgobject-2.0.a(gclosure.c.o): undefined symbol: ffi_call

(so that probably(?) means that it should reference _ffi_call instead)

Sorry, this issue is unrelated to this. I just reproduced the exact same issue on Emscripten 3.1.28 (during the investigation of #18388).

Anyway, I asked in issue hoodmane/libffi-emscripten#16 if there are any other projects that depend on that implementation.

Presumably changing the callsite to _unbox_small_structs would fix the error?

Let me investigate that as possible fix.

FWIW, this is still on my TODO-list.

sbc100 added a commit that referenced this pull request Dec 28, 2022
As part of #18391 (as yet unreleased) I changed the behaviour of EM_JS
functions such that ther were underscore mangled like native functions.
However this change in naming can break existing code and is not needed
for the fix after all.
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 28, 2022

This change make EM_JS functions appear on the JS side with and underscore prefix, just like other native functions.

I see. Building with --disable-modules also seems to fail with:

wasm-ld: error: /home/kleisauke/wasm-vips/build/target/lib/libgobject-2.0.a(gclosure.c.o): undefined symbol: ffi_call
wasm-ld: error: /home/kleisauke/wasm-vips/build/target/lib/libgobject-2.0.a(gclosure.c.o): undefined symbol: ffi_call

(so that probably(?) means that it should reference _ffi_call instead)

For reference, I'm testing this in combination with commit kleisauke/wasm-vips@941de51.

Presumably changing the callsite to _unbox_small_structs would fix the error?

Let me investigate that as possible fix.

I wonder how common this case is?

AFAIK, it only affects Pyodide/Python ctypes and the wasm-vips project.

I decided it would be better not to change this behaviour so I'm planning on reverting the mangling change: #18438

sbc100 added a commit that referenced this pull request Dec 29, 2022
As part of #18391 (as yet unreleased) I changed the behaviour of EM_JS
functions such that they were underscore mangled like native functions.
However this change in naming can break existing code and is not needed
for the fix after all.
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

Successfully merging this pull request may close these issues.

Pointer to EM_JS function won't compile with MAIN_MODULE
3 participants