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

dynCallLegacy: Fill in dynCall_sig with a Wasm adaptor if it is missing #17328

Merged
merged 23 commits into from
Aug 29, 2022

Conversation

hoodmane
Copy link
Contributor

This makes it possible to use a Rust side module without -sWASM_BIGINT.

@hoodmane hoodmane changed the title dynCallLegacy: Fill in dynCall_sig with a custom Wasm adaptor if it is missing dynCallLegacy: Fill in dynCall_sig with a Wasm adaptor if it is missing Jun 28, 2022
@kripken
Copy link
Member

kripken commented Jun 28, 2022

Do we not already have a mechanism for this in dynamic linking @sbc100 ? I thought we did but not sure where.

@hoodmane
Copy link
Contributor Author

Well nobody mentioned such a thing the last time I opened a PR on this:
#16693
but I guess that time the discussion went in the different direction of "why not use -sWASM_BIGINT".

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Can you explain a little more how this problem arises? I assume its where a dyncall is needed for signature that simply doesn't exist in the main module.

In general I'm worried about that mount of code and complexity this could add. I wonder if there are any other solutions, and also if this could be made optional?

I think it only ever needed in the dynamic linking case so at least we could maybe put it behind if MAIN_MODULE?

src/library.js Outdated Show resolved Hide resolved
src/runtime_functions.js Outdated Show resolved Hide resolved
src/library.js Outdated Show resolved Hide resolved
src/library.js Outdated Show resolved Hide resolved
@hoodmane
Copy link
Contributor Author

Can you explain a little more how this problem arises? I assume its where a dyncall is needed for signature that simply doesn't exist in the main module.

Exactly. The side module generates the dyncall but the main module didn't happen. It's often a problem with C++ and particularly Rust side modules that use Emscripten's exception handling.

I think it only ever needed in the dynamic linking case so at least we could maybe put it behind if MAIN_MODULE?

I put it behind MAIN_MODULE == 1 since it shouldn't be needed with MAIN_MODULE=2.

@hoodmane hoodmane closed this Jun 29, 2022
@hoodmane hoodmane reopened this Jun 29, 2022
@kripken
Copy link
Member

kripken commented Jun 29, 2022

Exactly. The side module generates the dyncall but the main module didn't happen. It's often a problem with C++ and particularly Rust side modules that use Emscripten's exception handling.

Makes sense. But it's still bugging me that I thought we had logic to handle this. Didn't we @sbc100 ? Maybe I'm misremembering something related...

Side question, how does Rust use Emscripten exception handling - is that for setjmp/longjmp, or something else?

Another topic, given that we seem to use the wrapper here:

Module['dynCall_' + sig] = createDyncallWrapper(sig)

Do we need the wrapper to be wasm code - could we generate JS code instead? That could be simpler.

@hoodmane
Copy link
Contributor Author

Yeah. The wrapper is converting pairs of 32 bit integers to 64 bit integers and back. We can't do this in JavaScript without BigInt and we are not assuming that we have BigInt. See discussion on #16693.

@kripken
Copy link
Member

kripken commented Jun 30, 2022

@hoodmane I see, thanks for the details.

I understand not wanting to assume BigInt64Array, but can't you assume BigInt? With that, creating a new wasm module would not be necessary IIUC.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 30, 2022

@hoodmane I see, thanks for the details.

I understand not wanting to assume BigInt64Array, but can't you assume BigInt? With that, creating a new wasm module would not be necessary IIUC.

IIRC all this code is gated behind !WASM_BIGINT so it only applies when BigInt is not available.

@kripken
Copy link
Member

kripken commented Jun 30, 2022

Ah, reading back I think I wasn't clear maybe - apologies @sbc100 @hoodmane

IIUC @hoodmane you don't want to use the WASM_BIGINT flag. But that flag is for wasm integration with BigInt. Just assuming BigInt in the JS language is a much lower requirement. In particular, that doesn't assume wasm exists at all (it could work with WASM=0), and also it doesn't assume BigInt64Array etc.

So what I am proposing is that we could depend on BigInt in the JS language even if WASM_BIGINT is not present. (note btw that this was an explicit reason for not calling this flag just BIGINT)

Practically speaking, caniuse shows BigInt has been in all major browsers for over 2 years, so that seems like something many use cases could assume. We could use BigInt in JS if the user specifies browser versions that are recent enough, so we don't even need a new flag here - but maybe we could set an internal JS_BIGINT marker for convenience.

If so then we can use JS which would be simpler than adding code to make a new wasm module. (However, there might be other reasons for using wasm here, like performance.)

@hoodmane
Copy link
Contributor Author

Pyodide is now using -sWASM_BIGINT and we are pretty happy about it. But I think Rust side modules should work without it.

that flag is for wasm integration with BigInt. Just assuming BigInt in the JS language is a much lower requirement.

Do you know of any reference for browser adoption of wasm bigint integratioon? Caniuse doesn't have it and it doesn't seem that easy to find.

With BigInt creating a new wasm module would not be necessary IIUC.

I think you need the wasm-bigint integration for it to work. The function pointer we want to call is a function that either takes 64 bit arguments or returns a 64 bit result. As far as I understand, we can't call such a thing from JavaScript at all without wasm bigint integration?

@sbc100
Copy link
Collaborator

sbc100 commented Jun 30, 2022

IIUC this change is all about adding dynCalls when wasm bigint integration is mission. Its directly filling a gab that the integration provides, its not about bigint in JS itself.

@kripken
Copy link
Member

kripken commented Jul 1, 2022

@hoodmane

I think you need the wasm-bigint integration for it to work. The function pointer we want to call is a function that either takes 64 bit arguments or returns a 64 bit result. As far as I understand, we can't call such a thing from JavaScript at all without wasm bigint integration?

Thanks! That's the key point I was missing. Ok, this approach sgtm.

@hoodmane
Copy link
Contributor Author

Could someone review this @sbc100 @kripken?

src/library_addfunction.js Outdated Show resolved Hide resolved
src/library_addfunction.js Outdated Show resolved Hide resolved
src/library.js Outdated
$dynCallLegacy__deps: ['$createDyncallWrapper'],
$createDyncallWrapper__deps: ['$generateFuncType', '$uleb128Encode'],
$createDyncallWrapper: function(sig) {
var sections = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really huge and unpleasant function.

I get that we might need it under certain circumstances but maybe we can take some steps to limits its impact. How about moving it to its own file? Maybe: src/library_makeDynCall.js
Is there some way we can have folks opt into it? I'm loath to add a new setting. Perhaps we could ask folks to do -lmakeDynCall or --js-library=makeDynCall? (Let me see if either of those will work).

@hoodmane
Copy link
Contributor Author

I moved it into a different file, we can modify the conditions for inclusion if you have a good idea for how to do that. I also still owe you tests for this function.

@gl84
Copy link
Contributor

gl84 commented Aug 23, 2022

Due to #17707 it seems that the adaptors are needed also for MAIN_MODULE == 2 (at least if dlopen is involved).

@hoodmane
Copy link
Contributor Author

Can this be merged?

@kripken
Copy link
Member

kripken commented Aug 26, 2022

lgtm, and I think @sbc100 hasn't had any further comments, so this should be good to land. See the flake8 error on CI though.

@hoodmane
Copy link
Contributor Author

hoodmane commented Aug 26, 2022

I'm a bit confused about the flake errors about line breaks before binary operators. I thought they reversed this rule:
PyCQA/pycodestyle#502
https://bugs.python.org/issue26763
The opposite rule is correct IMO. Anyways I will fix the flake errors.

@hoodmane
Copy link
Contributor Author

What happened with tempRet0?

@hoodmane
Copy link
Contributor Author

Okay tests pass again =)

@kripken
Copy link
Member

kripken commented Aug 29, 2022

Great!

@kripken kripken merged commit 704218f into emscripten-core:main Aug 29, 2022
gl84 added a commit to gl84/emscripten that referenced this pull request Aug 30, 2022
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)
@derivator
Copy link
Contributor

@hoodmane @kripken I assume this should have fixed rust exception handling in side modules, but I'm still having problems with it.

My Godot engine Rust side module works if I build with -Cpanic=abort, but without it fails with this error:

CompileError: WebAssembly.Module(): Compiling function #1 failed: invalid local index: 6 @+79
at createDyncallWrapper (MyGodotApp.js:formatted:839:26)
at dynCallLegacy (MyGodotApp.js:formatted:852:44)
at dynCall (MyGodotApp.js:formatted:864:24)
at MyGodotApp.js:formatted:874:28
at stubs. (MyGodotApp.js:formatted:1118:49)
at 013f806e:0x2e84ee
at 013f806e:0x246673
at dynCall (MyGodotApp.js:formatted:866:46)
at MyGodotApp.js:formatted:874:28
at stubs. (MyGodotApp.js:formatted:1118:49)

Does something still need to change on the rust side for this to work?

I'm using

rustc 1.65.0-nightly (8c6ce6b91 2022-09-02)
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.21-git (3ce1f72)
clang version 16.0.0 (https://github.com/llvm/llvm-project bd956b7db398e4ec1a73a3b344a304d6c6630681)

Apologies if this is the wrong place to ask.

@hoodmane
Copy link
Contributor Author

hoodmane commented Sep 4, 2022

Could you modify createDyncallWrapper to log its arguments and tell us what the failing invocation is?

@derivator
Copy link
Contributor

sig is viijj

@hoodmane
Copy link
Contributor Author

hoodmane commented Sep 4, 2022

Okay I'll try to look into it if I get a chance, though I'm pretty busy this week.

@hoodmane
Copy link
Contributor Author

hoodmane commented Sep 4, 2022

Reproduced the problem.

@hoodmane
Copy link
Contributor Author

hoodmane commented Sep 4, 2022

Fix here:
#17798

hoodmane added a commit to hoodmane/libffi-emscripten that referenced this pull request Jan 27, 2023
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.
hoodmane added a commit to hoodmane/libffi-emscripten that referenced this pull request Jan 27, 2023
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.
atgreen pushed a commit to libffi/libffi that referenced this pull request Feb 2, 2023
* added build script

* Apply libffi-emscripten patch

* Some changes to wasm32/ffi.c

* Remove exit(0); from test suites

* Fix LONGDOUBLE argument type

* Use more macros in ffi.c

* Use switch statements instead of if chains

* Implemented struct args

* Finish struct implementation

* Partially working closures

* Got closures working (most of closures test suite passes)

* Revert changes to test suite

* Update .gitignore

* Apply code formatter

* Use stackSave and stackRestore rather than directly adjusting stack pointer

* Add missing break

* Fix visibility of ffi_closure_alloc and ffi_closure_free

* Fix FFI_TYPE_STRUCT and FFI_TYPE_LONGDOUBLE when WASM_BIGINT is not used
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.

* Always use dynCall rather than direct wasmTable lookup (function pointer cast emulation changes dynCall)

* Prevent closures.c from duplicating symbols

* Try to set up CI

* Add test with bigint

* Make test methods static

* Remove BigInt shorthand because it messes up terser

* Add selenium tests

* Update tests a bit to try to make CI work

* WASM_BIGINT is a linker flag not a compile flag

* Finish getting CI working (#1)

* update gitignore

* Avoid adding "use strict;" to generated JS

This should be controlled by -s STRICT_JS in Emscripten.

* Make JavaScript ES5 compliant

* Remove redundant EXPORTED_RUNTIME_METHODS settings

* Fix definition of DEREF_I16

* Avoid marshalling FFI_TYPE_LONGDOUBLE when WASM_BIGINT is not used

* Add missing FFI_TYPE_STRUCT signature

* Improve test scripts

* Remove redundant EXPORTED_RUNTIME_METHODS settings

* Add missing EOL

* Add struct unpacking tests

* Update ci config to try to actually use WASM_BIGINT

* Revert "Avoid marshalling FFI_TYPE_LONGDOUBLE when WASM_BIGINT is not used"

This reverts commit 61bd5a3.

* Fix single_entry_structs tests

* Fix return from closure call

* Fix 64 bit return from closures

* only allocate as much space on stack for return pointer as needed

* Revert "only allocate as much space on stack for return pointer as needed"

This reverts commit e54a30f.

* xfail two tests

* Fix err_bad_abi test

* Remove test logging junk

* Try to set up long double marshalling for closures

* xfail err_bad_abi

* Fix reference errors in previous commit

* Add missing argument pointer assignment

* Fix signature of function pointer in cls_dbls_struct

* Fix longdouble argument

* Try some changes to bigint handling

* Fix BigInt handling

* Fix cls_longdouble test

* Fix long double closure arg with no WASM_BIGINT

* Use EM_JS to factor out js helpers

* Support for varargs closure calls

* Fix varargs calls

* Fix err_bad_abi test

* Fix typo in previous commit

* Add more assertions to closures test suite

* Fix some asserts

* Add assertions to a few more tests

* Fix some tests

* Fix more floating point assertions

* Update more tests

* Var args for ffi_call

* Don't do node tests

* Macro for allocating on stack

* Add some comments, simplify struct handling

* Try again to fix varargs calls, add comments

* Consolidate WASM_BIGINT conditionals into LOAD_U64 and STORE_U64 macros

* A bit of cleanup

* Fix another typo

* Some fixes to the testsuite

* Another testsuite fix

* Fix varags with closures?

* Another attempt at getting closure varargs to work

* sig is initialized later

* Allow libffi.closures tests to be run

* Improve build script

* Remove redundant semicolons

* Fix a few libffi.closures test failures

* Cleanup

* Legacy dynCall API is no longer used

* Fix FFI_TYPE_LONGDOUBLE offset

* xfail 2 tests for WASM

- closure_loc_fn0; not applicable -- codeloc doesn't point to closure.
- huge_struct; function signature too long.

* Revert some redundant dg-output/printf statements

Helps Node.

* Revert "Don't do node tests"

This reverts commit a341ef4.

* Fix assertions in cls_24byte

* More tiny formating fixes to test suite

* Revert "Revert "Don't do node tests""

This reverts commit 7722e68.

* Fix 64 bit returns when WASM_BIGINT is absent

* Fix print statement in cls_24byte

* Add CALL_FUNC_PTR macro to allow pyodide to define custom calling behavior to handle fpcast

* Update single_entry_structs tests

* More explanations

* Fix compile error in last commit

* Add more support for pyodide fpcast emulation, update CI to try to test it

* Clone via https

* Fix path to pyodide emsdk_env

* Add asserts to the rest of the test suite

* Fix test compile errors

* Fix some tests

* Fix cls_ulonglong

* Fix alignment of <4 byte args

* fix cls_ulonglong again

* Use snprintf instead of sprintf

* Should assert than strncmp returned 0

* Fix va_struct1 and va_struct3

* Change double and long double tests

These tests are failing because of a strange bug with prinft and doubles, but I am not convinced
it necessarily has anything to do with libffi. This version casts the double to int before printing it and avoids the issue

* Enable node tests

* Revert "Change double and long double tests"

This reverts commit 8f3ff89.

* Fix PYODIDE_FPCAST flag

* add conftest.py back in

* Fix emcc error: setting `EXPORTED_FUNCTIONS` expects `<class 'list'>` but got `<class 'str'>`

See discussion on pyodide/pyodide#1596

* Remove test.html

* Remove duplicate test file

* More changes from upstream

* Fix some whitespace

* Add some basic debug logging statements

* Reapply libffi.exp changes

* Don't build docs (#7)

Works around build issue makeinfo: command not found.

* Update long double alignment

Emscripten 2.0.26 reduces the aligmnet of long double to 8. Quoting
from `ChangeLog.md`:

> The alignment of `long double`, which is a 128-bit floating-point
> value implemented in software, is reduced from 16 to 8. The lower
> alignment allows `max_align_t` to properly match the alignment we
> use for malloc, which is 8 (raising malloc's alignment to achieve
> correctness the other way would come with a performance regression).
> (#10072)

* Update long double alignment

Emscripten 2.0.26 reduces the aligmnet of long double to 8. Quoting
from `ChangeLog.md`:

> The alignment of `long double`, which is a 128-bit floating-point
> value implemented in software, is reduced from 16 to 8. The lower
> alignment allows `max_align_t` to properly match the alignment we
> use for malloc, which is 8 (raising malloc's alignment to achieve
> correctness the other way would come with a performance regression).
> (#10072)

* Improve error handling a bit (#8)

* Fix handling of signed arguments to ffi_call (#11)

* Fix struct argument handling in ffi_call (#10)

* Remove fpcast emulation tests

* Align the stack to MAX_ALIGN before making call (#12)

* Increase MAX_ARGS

* Cleanup (#14)

* Fix Closure compiler error with -sASSERTIONS=1 (#15)

* Remove function pointer cast emulation (#13)

This reverts commit 593b402 and cbc54da, as it's no longer needed
after PR pyodide/pyodide#2019.

* Prefer the `__EMSCRIPTEN__` definition over `EMSCRIPTEN` (#18)

"The preprocessor define EMSCRIPTEN is deprecated. Don't pass it to code
in strict mode. Code should use the define __EMSCRIPTEN__ instead."
https://github.com/emscripten-core/emscripten/blob/84a634167a1cd9e8c47d37a559688153a4ceace6/emcc.py#L887-L890

* Install autoconf 2.71

* Try again with installing autoconf 2.71

* Fix compatibility with Emscripten 3.1.28

* CI: remove use of `EM_CONFIG` env

See commit:
emscripten-core/emsdk@3d87d5e

* Fix cls_multi_schar: cast rest_call to signed char

* Remove test xfails (#17)

* Fix long double when used as a varargs argument

* Enable unwindtest and fix it

* Add EM_JS_DEPS

* Also require convertJsFunctionToWasm

* Run tests very very verbose

* Echo the .emscripten file

* Remove --experimental-wasm-bigint insertion

* Build with assertions

* Move verbosity flags back out of LDFLAGS

* Remove debug print statement

* Use up to date pyodide docker image

* Explicitly cast res_call to fix test failure

* Put back name of main function in cls_longdouble_va.c

* Fix alignment

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.

* Fix handling of systems with no Js bigint integration

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.

* Improvements to emscripten test shell scripts (#21)

This fixes the C++ unwinding tests and makes other minor improvements
to the Emscripten test shell scripts.

* Rename the test folder and move test files into emscripten test folder

* Use docker image that has autoconf-2.71

* Cleanup

* Pin emscripten 3.1.30

* Fix build.sh path

* Rearrange ci pipeline

* Fix bpo_38748 test

* Cleanup

* Improvements to comments, add static asserts, and update copyright

* Use `*_js` instead of `*_helper` for EM_JS functions (#22)

* Minor code simplification

* Xfail first dejagnu test to work around emscripten cache messages

See emscripten-core/emscripten#18607

* Remove unneeded xfails

* Shorten conftest.py by using pytest-pyodide

* Apply formatters and linters to emscripten directory

* Fix Emscripten xfail hack

* Fix build-tests script

* Patch emscripten to quiet info messages

* Clean up compiler flags in scripts and remove some settings from circleci config

* Rename emscripten quiet script

* Add missing export

* Don't remove go.exp

* Add reference to emscripten logging issue

---------

Co-authored-by: Kleis Auke Wolthuizen <info@kleisauke.nl>
Co-authored-by: Kleis Auke Wolthuizen <github@kleisauke.nl>
Co-authored-by: Christian Heimes <christian@python.org>
Assioftware added a commit to Assioftware/libffi that referenced this pull request Feb 5, 2023
* added build script

* Apply libffi-emscripten patch

* Some changes to wasm32/ffi.c

* Remove exit(0); from test suites

* Fix LONGDOUBLE argument type

* Use more macros in ffi.c

* Use switch statements instead of if chains

* Implemented struct args

* Finish struct implementation

* Partially working closures

* Got closures working (most of closures test suite passes)

* Revert changes to test suite

* Update .gitignore

* Apply code formatter

* Use stackSave and stackRestore rather than directly adjusting stack pointer

* Add missing break

* Fix visibility of ffi_closure_alloc and ffi_closure_free

* Fix FFI_TYPE_STRUCT and FFI_TYPE_LONGDOUBLE when WASM_BIGINT is not used
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.

* Always use dynCall rather than direct wasmTable lookup (function pointer cast emulation changes dynCall)

* Prevent closures.c from duplicating symbols

* Try to set up CI

* Add test with bigint

* Make test methods static

* Remove BigInt shorthand because it messes up terser

* Add selenium tests

* Update tests a bit to try to make CI work

* WASM_BIGINT is a linker flag not a compile flag

* Finish getting CI working (#1)

* update gitignore

* Avoid adding "use strict;" to generated JS

This should be controlled by -s STRICT_JS in Emscripten.

* Make JavaScript ES5 compliant

* Remove redundant EXPORTED_RUNTIME_METHODS settings

* Fix definition of DEREF_I16

* Avoid marshalling FFI_TYPE_LONGDOUBLE when WASM_BIGINT is not used

* Add missing FFI_TYPE_STRUCT signature

* Improve test scripts

* Remove redundant EXPORTED_RUNTIME_METHODS settings

* Add missing EOL

* Add struct unpacking tests

* Update ci config to try to actually use WASM_BIGINT

* Revert "Avoid marshalling FFI_TYPE_LONGDOUBLE when WASM_BIGINT is not used"

This reverts commit 61bd5a3e20891623715604581b6e872ab3dfab80.

* Fix single_entry_structs tests

* Fix return from closure call

* Fix 64 bit return from closures

* only allocate as much space on stack for return pointer as needed

* Revert "only allocate as much space on stack for return pointer as needed"

This reverts commit e54a30faea3803e7ac33eed191bde9e573850fc1.

* xfail two tests

* Fix err_bad_abi test

* Remove test logging junk

* Try to set up long double marshalling for closures

* xfail err_bad_abi

* Fix reference errors in previous commit

* Add missing argument pointer assignment

* Fix signature of function pointer in cls_dbls_struct

* Fix longdouble argument

* Try some changes to bigint handling

* Fix BigInt handling

* Fix cls_longdouble test

* Fix long double closure arg with no WASM_BIGINT

* Use EM_JS to factor out js helpers

* Support for varargs closure calls

* Fix varargs calls

* Fix err_bad_abi test

* Fix typo in previous commit

* Add more assertions to closures test suite

* Fix some asserts

* Add assertions to a few more tests

* Fix some tests

* Fix more floating point assertions

* Update more tests

* Var args for ffi_call

* Don't do node tests

* Macro for allocating on stack

* Add some comments, simplify struct handling

* Try again to fix varargs calls, add comments

* Consolidate WASM_BIGINT conditionals into LOAD_U64 and STORE_U64 macros

* A bit of cleanup

* Fix another typo

* Some fixes to the testsuite

* Another testsuite fix

* Fix varags with closures?

* Another attempt at getting closure varargs to work

* sig is initialized later

* Allow libffi.closures tests to be run

* Improve build script

* Remove redundant semicolons

* Fix a few libffi.closures test failures

* Cleanup

* Legacy dynCall API is no longer used

* Fix FFI_TYPE_LONGDOUBLE offset

* xfail 2 tests for WASM

- closure_loc_fn0; not applicable -- codeloc doesn't point to closure.
- huge_struct; function signature too long.

* Revert some redundant dg-output/printf statements

Helps Node.

* Revert "Don't do node tests"

This reverts commit a341ef4b.

* Fix assertions in cls_24byte

* More tiny formating fixes to test suite

* Revert "Revert "Don't do node tests""

This reverts commit 7722e685ea04e2420e042886816d8c4dd31f5dcb.

* Fix 64 bit returns when WASM_BIGINT is absent

* Fix print statement in cls_24byte

* Add CALL_FUNC_PTR macro to allow pyodide to define custom calling behavior to handle fpcast

* Update single_entry_structs tests

* More explanations

* Fix compile error in last commit

* Add more support for pyodide fpcast emulation, update CI to try to test it

* Clone via https

* Fix path to pyodide emsdk_env

* Add asserts to the rest of the test suite

* Fix test compile errors

* Fix some tests

* Fix cls_ulonglong

* Fix alignment of <4 byte args

* fix cls_ulonglong again

* Use snprintf instead of sprintf

* Should assert than strncmp returned 0

* Fix va_struct1 and va_struct3

* Change double and long double tests

These tests are failing because of a strange bug with prinft and doubles, but I am not convinced
it necessarily has anything to do with libffi. This version casts the double to int before printing it and avoids the issue

* Enable node tests

* Revert "Change double and long double tests"

This reverts commit 8f3ff89c6577dc99564181cd9974f2f1ba21f1e9.

* Fix PYODIDE_FPCAST flag

* add conftest.py back in

* Fix emcc error: setting `EXPORTED_FUNCTIONS` expects `<class 'list'>` but got `<class 'str'>`

See discussion on pyodide/pyodide#1596

* Remove test.html

* Remove duplicate test file

* More changes from upstream

* Fix some whitespace

* Add some basic debug logging statements

* Reapply libffi.exp changes

* Don't build docs (#7)

Works around build issue makeinfo: command not found.

* Update long double alignment

Emscripten 2.0.26 reduces the aligmnet of long double to 8. Quoting
from `ChangeLog.md`:

> The alignment of `long double`, which is a 128-bit floating-point
> value implemented in software, is reduced from 16 to 8. The lower
> alignment allows `max_align_t` to properly match the alignment we
> use for malloc, which is 8 (raising malloc's alignment to achieve
> correctness the other way would come with a performance regression).
> (#10072)

* Update long double alignment

Emscripten 2.0.26 reduces the aligmnet of long double to 8. Quoting
from `ChangeLog.md`:

> The alignment of `long double`, which is a 128-bit floating-point
> value implemented in software, is reduced from 16 to 8. The lower
> alignment allows `max_align_t` to properly match the alignment we
> use for malloc, which is 8 (raising malloc's alignment to achieve
> correctness the other way would come with a performance regression).
> (#10072)

* Improve error handling a bit (#8)

* Fix handling of signed arguments to ffi_call (#11)

* Fix struct argument handling in ffi_call (#10)

* Remove fpcast emulation tests

* Align the stack to MAX_ALIGN before making call (#12)

* Increase MAX_ARGS

* Cleanup (#14)

* Fix Closure compiler error with -sASSERTIONS=1 (#15)

* Remove function pointer cast emulation (#13)

This reverts commit 593b402 and cbc54da, as it's no longer needed
after PR pyodide/pyodide#2019.

* Prefer the `__EMSCRIPTEN__` definition over `EMSCRIPTEN` (#18)

"The preprocessor define EMSCRIPTEN is deprecated. Don't pass it to code
in strict mode. Code should use the define __EMSCRIPTEN__ instead."
https://github.com/emscripten-core/emscripten/blob/84a634167a1cd9e8c47d37a559688153a4ceace6/emcc.py#L887-L890

* Install autoconf 2.71

* Try again with installing autoconf 2.71

* Fix compatibility with Emscripten 3.1.28

* CI: remove use of `EM_CONFIG` env

See commit:
emscripten-core/emsdk@3d87d5e

* Fix cls_multi_schar: cast rest_call to signed char

* Remove test xfails (#17)

* Fix long double when used as a varargs argument

* Enable unwindtest and fix it

* Add EM_JS_DEPS

* Also require convertJsFunctionToWasm

* Run tests very very verbose

* Echo the .emscripten file

* Remove --experimental-wasm-bigint insertion

* Build with assertions

* Move verbosity flags back out of LDFLAGS

* Remove debug print statement

* Use up to date pyodide docker image

* Explicitly cast res_call to fix test failure

* Put back name of main function in cls_longdouble_va.c

* Fix alignment

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.

* Fix handling of systems with no Js bigint integration

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.

* Improvements to emscripten test shell scripts (#21)

This fixes the C++ unwinding tests and makes other minor improvements
to the Emscripten test shell scripts.

* Rename the test folder and move test files into emscripten test folder

* Use docker image that has autoconf-2.71

* Cleanup

* Pin emscripten 3.1.30

* Fix build.sh path

* Rearrange ci pipeline

* Fix bpo_38748 test

* Cleanup

* Improvements to comments, add static asserts, and update copyright

* Use `*_js` instead of `*_helper` for EM_JS functions (#22)

* Minor code simplification

* Xfail first dejagnu test to work around emscripten cache messages

See emscripten-core/emscripten#18607

* Remove unneeded xfails

* Shorten conftest.py by using pytest-pyodide

* Apply formatters and linters to emscripten directory

* Fix Emscripten xfail hack

* Fix build-tests script

* Patch emscripten to quiet info messages

* Clean up compiler flags in scripts and remove some settings from circleci config

* Rename emscripten quiet script

* Add missing export

* Don't remove go.exp

* Add reference to emscripten logging issue

---------

Co-authored-by: Kleis Auke Wolthuizen <info@kleisauke.nl>
Co-authored-by: Kleis Auke Wolthuizen <github@kleisauke.nl>
Co-authored-by: Christian Heimes <christian@python.org>
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.

None yet

5 participants