-
Notifications
You must be signed in to change notification settings - Fork 746
Add wasm32 emscripten support #763
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
Conversation
sig needs to be vi here for FFI_TYPE_STRUCT and FFI_TYPE_LONGDOUBLE, noticed this while running the test suite without WASM_BIGINT support.
…ter cast emulation changes dynCall)
The stack pointer apparently needs to be aligned to 16. There were some terrible subtle bugs caused by not respecting this. stackAlloc knows that the stack should be 16 aligned, so we can use stackAlloc(0) to enforce this. This way if alignment requirements change, as long as Emscripten updates stackAlloc to continue to enforce them we should be okay.
When we run the node tests we use node v14 tests (since node v14 is vendored with Emscripten). Node v14 has no Js bigint integration unless the --experimental-wasm-bigint flag is passed. So only the node tests really notice if we get this right. Turns out, it didn't work. We can't call a JavaScript function with 64 bit integer arguments without bigint integration. In ffi_call, we are trying to call a wasm function that takes 64 bit integer arguments. dynCall is designed to do this. We need to go back to tracking the signature when we don't have WASM_BIGINT, and then use dynCall. This works better now that emscripten can dynamically fill in extra dynCall wrappers: emscripten-core/emscripten#17328 On the other hand, for the closures we are not getting a function pointer as a first argument. We need to make our own wasm legalizer adaptor that splits 64 bit integer arguments and then calls the JavaScript trampoline, then the JavaScript trampoline reassembles them, calls the closure, then splits the result (if it's a 64 bit integer) and the adaptor puts it back together.
This fixes the C++ unwinding tests and makes other minor improvements to the Emscripten test shell scripts.
for name in ["cache", "system_libs", "shared"]: | ||
logger = logging.getLogger(name) | ||
logger.setLevel(logging.WARN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbc100 for now it seems like this is a reasonable workaround for emscripten-core/emscripten#18607.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah I guess so. Are you actually patching emcc.py here? Long term seems risky but short term I guess YOLO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YOLO indeed =)
This was the first method I tried that actually worked. I was unable to convince the test system dejagnu that the tests should pass despite these INFO
messages, even though that should be a reasonable thing to ask it to do. Maybe @atgreen can tell me how to do that.
Another approach would be to replace emmake
with my own wrapper script and use a compiler wrapper. But that would require me to duplicate the logic in get_building_env
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are trying to filter out some output for dejagnu, check out libffi-dg-prune in testsuite/lib/libffi.exp. Will that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this works great. I should have just asked! Will make a PR to fix this.
Uses circleci for continuous integration. All tests are currently passing:
https://app.circleci.com/pipelines/github/hoodmane/libffi-emscripten?branch=wasm32-emscripten
Tested against node 14, Chrome 109 and Firefox 109.
I think it needs Emscripten >= 3.1.24 because of the use of
EM_JS_DEPS
although that could be fixed. It doesn't currently work on Emscripten 3.1.31 -- the emitted js files contain a syntax error in that case due to a bad interaction between-sSTRICT_JS
and some other construct.@kleisauke @rth @ryanking13 @tiran