Fix compatibility with Emscripten 3.1.28 #20

kleisauke · 2022-12-25T11:57:24Z

Also, take advantage of EM_JS_DEPS while we are here (commit f0e8a0e).

Tested with:

$ sed -i'.old' "/^NODE_JS = /s/emsdk.*/\[&, '--experimental-wasm-bigint'\]/" $EMSDK/.emscripten
$ autoreconf -fiv
$ emconfigure ./configure --host=wasm32-unknown-linux --enable-static --disable-shared --disable-builddir --disable-multi-os-directory --disable-raw-api --disable-docs CFLAGS="-DWASM_BIGINT"
$ make
$ EMMAKEN_JUST_CONFIGURE=1 emmake make check RUNTESTFLAGS="LDFLAGS_FOR_TARGET='-sEXPORTED_FUNCTIONS=_main,_malloc,_free -sALLOW_TABLE_GROWTH -sWASM_BIGINT'"
...
		=== libffi Summary ===

# of expected passes		1388
# of unexpected failures	70
# of unexpected successes	2
# of expected failures		2
# of unresolved testcases	2
# of unsupported tests		28

Context: emscripten-core/emscripten#18391 (comment).

/cc @sbc100

sbc100 · 2022-12-25T12:17:31Z

build.sh

@@ -38,6 +38,8 @@ export LDFLAGS="-L$TARGET/lib -O3 -sEXPORTED_FUNCTIONS=_main,_malloc,_free -sALL
 if [ "$WASM_BIGINT" = "true" ]; then
  export LDFLAGS+=" -sWASM_BIGINT"
 fi
+# Ensure the `ffi_call` symbol can be found at link time
+export LDFLAGS+=" -sLLD_REPORT_UNDEFINED=0"


This is an unfortunate thing to have to add.

Why is this symbol defined? Is it and EM_JS symbol? Do you know where the undefined reference coming from?

Good question. It is defined here:

libffi-emscripten/src/wasm32/ffi.c

Line 173 in 8bcb426

ffi_call, (ffi_cif * cif, ffi_fp fn, void *rvalue, void **avalue),

(i.e., as an EM_JS symbol)

Do you know where the undefined reference coming from?

I'm not sure, let me investigate that. Perhaps defining an ffi_call stub in native code fixes this.

... relevant header declaration:

libffi-emscripten/include/ffi.h.in

Lines 482 to 486 in 8bcb426

FFI_API

void ffi_call(ffi_cif *cif,

void (*fn)(void),

void *rvalue,

void **avalue);

I think I see what is happening. EM_JS functions are functions that are defined in native code. We they kind of need to be undefined when wasm-ld runs, so it knows to generate imports for these symbols.

When an EM_JS functions is used within the same file where it is declared this is not an issues since the macro declares the symbol with an import_name attribute, which tells the linker its OK to be undefined/imported.

As you suggested I think the simplest solution would be write a native wrapper for the EM_JS function in the same file where it is defined.

Native wrapper function implemented with commit d1798d8, seems to work!

kleisauke · 2022-12-28T14:22:37Z

Marking as draft due to dependence on PR emscripten-core/emscripten#18438 (which would avoid the need of commit 42fdeeb).

After that, this PR can be re-titled as "Fix compatibility with Emscripten 3.1.28".

Available since Emscripten 3.1.24.

See commit: emscripten-core/emsdk@3d87d5e

kleisauke · 2022-12-29T08:39:19Z

Just dropped commit 42fdeeb and 8bcb426. This is ready for review now.

Note that commit f0e8a0e would mean that Emscripten 3.1.24 is the minimum required version to compile this. Let me know if you want a separate PR for that.

sbc100

emscripten stuff lgtm

sbc100 · 2022-12-29T11:23:06Z

src/wasm32/ffi.c

@@ -170,7 +170,7 @@ unbox_small_structs, (ffi_type type_ptr), {

 EM_JS_MACROS(
 void,
-ffi_call, (ffi_cif * cif, ffi_fp fn, void *rvalue, void **avalue),
+ffi_call_helper, (ffi_cif *cif, ffi_fp fn, void *rvalue, void **avalue),


Maybe call this ffi_call_js?

Good point, perhaps that's clearer. I aligned/borrowed this naming convention with those other EM_JS functions.

@hoodmane WDYT? Should we use *_js here (and possibly those other occurrences) instead of *_helper?

+1 for ffi_call_js.

I cherry picked this commit using ffi_call_js.

I just opened PR #22 for this.

kleisauke · 2022-12-29T13:05:00Z

Note that commit f0e8a0e would mean that Emscripten 3.1.24 is the minimum required version to compile this.

Thinking about this further, I think we could make the EM_JS_DEPS macro no-op with something like this:

/* The __EMSCRIPTEN_major__ etc macros used to be passed implicitly, version
 * 3.1.4 moved them to a version header and version 3.1.23 dropped the
 * backwards compatibility. To work consistently on all versions, include the
 * header only if the version macros aren't present.
 * https://github.com/emscripten-core/emscripten/commit/f99af02045357d3d8b12e63793cef36dfde4530a
 * https://github.com/emscripten-core/emscripten/commit/f76ddc702e4956aeedb658c49790cc352f892e4c
 */
#ifndef __EMSCRIPTEN_major__
#include <emscripten/version.h>
#endif

/* Make the EM_JS_DEPS macro no-op before Emscripten 3.1.24.
 */
#if __EMSCRIPTEN_major__*10000 + __EMSCRIPTEN_minor__*100 + __EMSCRIPTEN_tiny__ < 30124
#define EM_JS_DEPS(tag, deps)
#endif

(untested)

hoodmane · 2023-01-27T00:54:45Z

I'm going to close this. I merged some of it and other stuff is superseded now. Thanks @kleisauke!

kleisauke · 2023-01-27T09:45:03Z

Thanks! BTW, do you think we should enforce -sWASM_BIGINT? It might make the code a bit easier and avoids having to specify -DWASM_BIGINT during compilation.

libffi-emscripten/testsuite/emscripten/node-tests.sh

Lines 25 to 26 in e2f8291

    
           # We need to detect WASM_BIGINT support at compile time 
        
           export CFLAGS+=" -DWASM_BIGINT"

The projects depending on this repo (i.e. Pyodide/Python ctypes, wasm-vips, etc.) already builds with -sWASM_BIGINT by default (afaik).

hoodmane · 2023-01-27T16:28:15Z

Yeah it's a lot of work to support the no wasm bigint case. @pmp-p seems to be an evangelist for not using wasm-bigint but I'm not exactly sure why. compilation, runtime, and size are all notably less with WASM_BIGINT integration and I think support is missing in <3% of browsers in the wild. I know the godot game engine builds without it (but it doesn't use libffi).

For a long time we only thought we were supporting no wasm bigint since we were testing against browsers with js bigint integration. But the no wasm bigint case adds a lot of really complicated code:
e7582c7

hoodmane · 2023-01-28T00:26:14Z

By the way @kleisauke I would appreciate a PR adding the support for Emscripten < 3.1.24.

pmp-p · 2023-01-29T06:28:19Z

@pmp-p seems to be an evangelist for not using wasm-bigint

also add to that shared memory, atomics and threads, simd/neon ...

The reason is very simple, there are litterally truckload of devices out there stuck at different level of wasm implementation because they won't receive browser updates anymore. But when wasm is present and not behing a flag, it is obviously MVP 1.0 : so instead of wondering who has implemented feature or not just use the established standard.

Sticking to MVP is a way to stop stockpiling perfectly useable devices ( wasm is quite safe on unsafe devices ) in landfills

Another reason is for evaluating Wasi runtimes in a fair manner. As they don't all implement extra outside the mvp especially when it comes to embedding ( more features == more strain on device )

that's also why i proposed to have two type of wasm wheels on pypi, mvp (cpython-wasm) and bleeding edge (pyodide)

hoodmane · 2023-01-29T07:15:30Z

I don't think Emscripten supports targeting wasm mvp 1.0 at all, it always requires the mutable globals extension.

I am in favor of keeping support for no-wasm-bigint in this libffi port. This way if no-wasm-bigint Python builds want ctypes, they can have it.

There is a tradeoff between compatibility on the one hand and on the other hand performance and maintainability. Using wasm-bigint significantly reduces our exposure to weird bugs. It's a major issue for Pyodide: frequently when we update compiler versions, some collection of packages start failing to compile. Since we operate on a limited amount of donated effort, it doesn't make sense to expend the effort to avoid requiring a feature that is present in 98% of browsers in the wild.

pmp-p · 2023-01-29T09:03:33Z

so you think mvp+bigint+~~mutable globals~~(=> no threads see next post) should be the absolute minimum requirement and is a safe bet ?

do you remember where you got the info about <3% of browsers that don't support bigint ?

if pyodide can start on Chrome 81.0.4044 i'll change my mind, i'll try today

edit/ it does not, that would wipe out android kitkat devices, plenty of them still in storage waiting to be sold under $30 piece.

it works on Chrome 95.0.4638 so android 5.1 is saved for now.

So i guess i'll change my mind when a newer Wasm standard is published and kitkat stock has vanished.

sbc100 · 2023-01-29T16:42:50Z

I don't think Emscripten supports targeting wasm mvp 1.0 at all, it always requires the mutable globals extension.

IIUC, you can still target MVP wasm with emscripten, in fact that was the default until recently. The default was recently changed, but only to match that the new llvm defaults: https://reviews.llvm.org/D125728. But non-MVP features are not used then when targeting old runtimes: emscripten-core/emscripten#17689

Regarding mutable global specifically, those are only (by emscripten) used in multi-threaded builds.

. emscripten, by default, will still target MVP. We do use mutable globals if you

I am in favor of keeping support for no-wasm-bigint in this libffi port. This way if no-wasm-bigint Python builds want ctypes, they can have it.

There is a tradeoff between compatibility on the one hand and on the other hand performance and maintainability. Using wasm-bigint significantly reduces our exposure to weird bugs. It's a major issue for Pyodide: frequently when we update compiler versions, some collection of packages start failing to compile. Since we operate on a limited amount of donated effort, it doesn't make sense to expend the effort to avoid requiring a feature that is present in 98% of browsers in the wild.

hoodmane · 2023-01-29T16:53:40Z

Minimum chrome version for wasm bigint is 85 according to https://webassembly.org/roadmap/

Chrome 85 came out in August 2020 so it is only two and a half years old.

My 3% number came from the minimum supported versions on the webassembly roadmap plus can use browser usage statistics. I have no clue what methodology caniuse uses to collect those statistics.

sbc100 reviewed Dec 25, 2022

View reviewed changes

kleisauke marked this pull request as draft December 28, 2022 14:22

kleisauke added 4 commits December 29, 2022 09:31

Fix compatibility with Emscripten 3.1.28

b7f5e87

Take advantage of EM_JS_DEPS

f0e8a0e

Available since Emscripten 3.1.24.

nit: ensure correct placement of pointer asterisks

619d1fd

CI: remove use of EM_CONFIG env

e1dd627

See commit: emscripten-core/emsdk@3d87d5e

kleisauke force-pushed the tot-compat branch from ef44421 to e1dd627 Compare December 29, 2022 08:37

kleisauke marked this pull request as ready for review December 29, 2022 08:39

kleisauke changed the title ~~Fix compatibility with Emscripten >= 3.1.28~~ Fix compatibility with Emscripten 3.1.28 Dec 29, 2022

sbc100 reviewed Dec 29, 2022

View reviewed changes

Merge branch 'libffi-emscripten-main' into tot-compat

57538c3

hoodmane force-pushed the tot-compat branch from 583e360 to 57538c3 Compare January 24, 2023 21:05

hoodmane closed this Jan 27, 2023

kleisauke mentioned this pull request Jan 27, 2023

Use *_js instead of *_helper for EM_JS functions #22

Merged

kleisauke deleted the tot-compat branch January 27, 2023 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with Emscripten 3.1.28 #20

Fix compatibility with Emscripten 3.1.28 #20

kleisauke commented Dec 25, 2022 •

edited

Loading

sbc100 Dec 25, 2022

kleisauke Dec 25, 2022

kleisauke Dec 25, 2022

sbc100 Dec 25, 2022

kleisauke Dec 25, 2022

kleisauke commented Dec 28, 2022

kleisauke commented Dec 29, 2022

sbc100 left a comment

sbc100 Dec 29, 2022

kleisauke Dec 29, 2022 •

edited

Loading

hoodmane Jan 24, 2023

hoodmane Jan 24, 2023

kleisauke Jan 27, 2023

kleisauke commented Dec 29, 2022

hoodmane commented Jan 27, 2023

kleisauke commented Jan 27, 2023

hoodmane commented Jan 27, 2023

hoodmane commented Jan 28, 2023

pmp-p commented Jan 29, 2023 •

edited

Loading

hoodmane commented Jan 29, 2023

pmp-p commented Jan 29, 2023 •

edited

Loading

sbc100 commented Jan 29, 2023

hoodmane commented Jan 29, 2023

	FFI_API
	void ffi_call(ffi_cif *cif,
	void (*fn)(void),
	void *rvalue,
	void **avalue);

Fix compatibility with Emscripten 3.1.28 #20

Fix compatibility with Emscripten 3.1.28 #20

Conversation

kleisauke commented Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleisauke commented Dec 28, 2022

kleisauke commented Dec 29, 2022

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleisauke Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleisauke commented Dec 29, 2022

hoodmane commented Jan 27, 2023

kleisauke commented Jan 27, 2023

hoodmane commented Jan 27, 2023

hoodmane commented Jan 28, 2023

pmp-p commented Jan 29, 2023 • edited Loading

hoodmane commented Jan 29, 2023

pmp-p commented Jan 29, 2023 • edited Loading

sbc100 commented Jan 29, 2023

hoodmane commented Jan 29, 2023

kleisauke commented Dec 25, 2022 •

edited

Loading

kleisauke Dec 29, 2022 •

edited

Loading

pmp-p commented Jan 29, 2023 •

edited

Loading

pmp-p commented Jan 29, 2023 •

edited

Loading