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

Add dynCall_sig adaptor for MAIN_MODULE == 2 #17759

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gl84
Copy link
Contributor

@gl84 gl84 commented Aug 30, 2022

Fixes #17707.

Follow up on PR #17328.

  • Create the wasm adaptor also for MAIN_MODULE == 2
  • Remove obsolete assert (triggers for all cases were we create the wrapper)

Follow up on PR emscripten-core#17328.

- Create the wasm adaptor also for MAIN_MODULE == 2
- Remove obsolete assert (triggers for all cases were we create the wrapper)
@kripken
Copy link
Member

kripken commented Sep 1, 2022

cc @hoodmane

My understanding was that MAIN_MODULE=2 was not included for a reason there, but maybe I misunderstood something.

@hoodmane
Copy link
Contributor

hoodmane commented Sep 1, 2022

It was not included because it shouldn't be necessary. In the example:

em++ -g -sDISABLE_EXCEPTION_CATCHING=0 -s MAIN_MODULE=2 main_dlopen.cpp -sDYLINK_DEBUG=1 -o main_dlopen.js -sASSERTIONS=2 -sEXPORTED_FUNCTIONS=[_main] -sNO_AUTOLOAD_DYLIBS libside.wasm

The linker should look inside libside.wasm and see that it calls env.invoke_ji and then generate the appropriate wrappers. It seems like it doesn't do this correctly. So I think this is not the correct fix. OTOH it is an easy fix.

@gl84
Copy link
Contributor Author

gl84 commented Sep 1, 2022

The example is only an easy reproducing case which exports everything (except the dynCalls)...

In reality the MAIN_MODULE has never seen the plugin before loading it. Hence it is hard (or even impossible) to pre-generate all dynCalls at compile time. From my point of view the only thing the main program really expects is that dlopen and dlsym work without side effects...

@hoodmane
Copy link
Contributor

hoodmane commented Sep 1, 2022

In reality the MAIN_MODULE has never seen the plugin before loading it

For that use case you're supposed to use -sMAIN_MODULE=1. For -sMAIN_MODULE=2 you are supposed to build all the side modules first and then include them as arguments when linking the main module. I think it's perfectly reasonable for the linker to check which day calls are used in the side module and include them.

@gl84
Copy link
Contributor Author

gl84 commented Sep 1, 2022

That's a valid point.

If one is interpreting MAIN_MODULE=2 that way something to prevent the elimination of the call to createDyncallWrapper in dynCallLegacy would be super helpful.

@sbc100: Is there a way that doesn't need a new setting?

@sbc100
Copy link
Collaborator

sbc100 commented Sep 2, 2022

Yes I think we should be analyzing the side modules at link time and creating extra dynCalls accordingly.

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.

dlopen - missing dynCall_ji in Module
4 participants