-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Can we remove EMULATE_FUNCTION_POINTER_CASTS? #23952
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
Comments
We haven't used it since 2021 and it was expensive enough that we would really hate to switch back to it. So +1 from me to remove it. |
Also, all of these things like |
Very cool about not using I am not aware of any other users. I suggest we ask on the mailing list, and can remove it based on that. |
Users who use this setting will now see the following warning: ``` emcc: warning: EMULATE_FUNCTION_POINTER_CASTS is deprecated (no known users). Please open a bug if you have a continuing need for this setting [-Wdeprecated] ``` See emscripten-core#23952
Users who use this setting will now see the following warning: ``` emcc: warning: EMULATE_FUNCTION_POINTER_CASTS is deprecated (no known users). Please open a bug if you have a continuing need for this setting [-Wdeprecated] ``` See emscripten-core#23952
Users who use this setting will now see the following warning: ``` emcc: warning: EMULATE_FUNCTION_POINTER_CASTS is deprecated (no known users). Please open a bug if you have a continuing need for this setting [-Wdeprecated] ``` See #23952
Function pointer casts are used heavily by GLib (https://gitlab.gnome.org/GNOME/glib/-/issues/3670), which in turn is used by QEMU. For now QEMU avoids them with only some code uglification, but the deprecation is preventing usage of GObject with Emscripten. |
I'm curious about the statement: "GLib absolutely requires the ability to cast function pointers, and that’s not going to change. Quite apart from g_list_sort() and g_slist_sort(), the whole GObject signal infrastructure depends on it". To be clear its not casting that is the problem for emscripten but calling function via a signature they were not declared with. Is there some misunderstanding perhaps or does glib actually call the function pointers via the wrong signature (i.e. with more of fewer args than that function takes, or with args that differ from the declaration). I'm pretty sure this is technically undefined behaviour in C isn't it? <Edit: I commented on the upstream issue so lets continue this conversation there> |
It seems that glib has a hard requirement to be able to call function with too many and too few arguments: https://gitlab.gnome.org/GNOME/glib/-/blob/main/docs/toolchain-requirements.md?ref_type=heads#calling-functions-through-differently-typed-function-pointers Should we consider un-deprecating this setting in order to support glib applications? I'm inclined to myself. |
Yeah, after reading that, I think we should un-deprecate. glib is pretty important. Luckily keeping the feature around is not a major burden. |
This change effectively reverts emscripten-core#23953 It turns out there is an ongoing need for this in the glib project: See https://gitlab.gnome.org/GNOME/glib/-/issues/3670 and https://gitlab.gnome.org/GNOME/glib/-/blob/main/docs/toolchain-requirements.md?ref_type=heads#calling-functions-through-differently-typed-function-pointers Fixes: emscripten-core#23952
I wonder if glib could benefit from an approach like what Pyodide is using. In particular, what if we had a binaryen pass that took:
and replaced it with Of course you couldn't handle as many type signatures as @bonzini See how we handle this for Python here: |
This change effectively reverts emscripten-core#23953 It turns out there is an ongoing need for this in the glib project: See https://gitlab.gnome.org/GNOME/glib/-/issues/3670 and https://gitlab.gnome.org/GNOME/glib/-/blob/main/docs/toolchain-requirements.md?ref_type=heads#calling-functions-through-differently-typed-function-pointers Fixes: emscripten-core#23952
This setting is fairly complex and does not have enough test coverage.
The main user of this setting used to be python but apparently that is no longer the case: #23948 (reply in thread)
The text was updated successfully, but these errors were encountered: