Use .init_array to call constructors in WASM instead of JS snippet#1476
Use .init_array to call constructors in WASM instead of JS snippet#1476Bromeon merged 2 commits intogodot-rust:masterfrom
Conversation
PgBiel
left a comment
There was a problem hiding this comment.
Mostly LGTM, let's try to wait for #1275 so we can at least make sure we aren't breaking anything important.
Big thanks for your research and PR!
This PR currently introduces breaking changes
I'd say it's unlikely anyone was using that in any sufficiently complicated way, and the fix is very straightforward (just remove the function definition, the wasm binary doesn't have to be renamed).
Of course, if policy mandates, we can always wait before 0.5 to land this, though I'm kinda leaning towards considering this as part of the wasm being experimental thing.
It's worth noting that if I manually call __wasm_call_ctors a second time, godot-rust already panics
Right. I think this is enough of a guarantee for me.
godot-ffi/src/plugins.rs
Outdated
| #[cfg(all(target_family = "wasm", not(target_os = "emscripten")))] | ||
| compile_error!("Wasm targets not using Emscripten are not supported."); |
There was a problem hiding this comment.
We can move this out of the macro, into the crate itself.
There was a problem hiding this comment.
Which crate? godot-ffi or godot?
There was a problem hiding this comment.
Done. Some checks are also in godot-ffi, I'm not sure why -- it seems to me like they could be here as well.
|
I tried running tests locally, using |
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1476 |
|
Should I rebase? |
|
I believe we should merge the tests PR with only unit tests. Integration tests will take a while, as they are substantially more complicated, so they can be implemented in a separate PR. So, we could test this PR with my hello-gdext-wasm project (for example) and make sure it passes the unit tests once merged, and, if that works out, this PR can likely be merged before integration tests. |
|
#1275 is now merged. Could you rebase and fix CI? 🙂 |
1a66f10 to
c4a158e
Compare
|
Done. I ran the CI on my fork and it passed, but it got a compilation warning on wasm due to the |
Bromeon
left a comment
There was a problem hiding this comment.
This is a beautiful cleanup, great job! 👍
I still need to test this with some projects.
[...] it got a compilation warning on wasm due to the
translate_functionalmacro helper now being unused. I deleted it. (Or do you want to keep it around, in case it's useful in the future?)
Yes, that would be nice -- you can annotate it with #[expect(dead_code)] I think.
This PR currently introduces breaking changes, as it removes the
ExtensionLibrary::override_wasm_binaryfunction, as it's no longer needed or used.
Just to understand this a bit better -- this was introduced in #1093 to allow custom binary names. In particular, it allows coexisting binaries for threads and nothreads, see motivating example in that PR.
Why is this no longer needed? It seems orthogonal to the registration problem addressed here.
Eventually, you could also squash commits 🙂
godot/src/lib.rs
Outdated
| // On non-Emscripen targets, wasm-ld will insert a call to __wasm_call_ctors | ||
| // (which calls all constructors) to the start all exported functions, if it | ||
| // detects that __wasm_call_ctors is never called and not exported. | ||
| // This could cause constructors to run multiple times. Emscripen should always | ||
| // export __wasm_call_ctors and call it at runtime. | ||
| // See https://github.com/godot-rust/gdext/pull/1476 for more info and links. |
There was a problem hiding this comment.
Nitpick: please use roughly 120-145 chars line width.
Because we no longer need to know the name of the binary, like on other platforms. The only reason the method existed was so that the JavaScript code would know which library to search for constructors to call. (Unless you planned for some other use, but I don't see what that would be.) |
130671c to
7243a26
Compare
Head branch was pushed to by a user without write access
04ddd01 to
7243a26
Compare
|
I accidentally merged master into by this branch. I removed the merge commit, but it still cleared workflow status, even though workflows already ran on this exact commit hash. 😞 Sorry, I didn't mean to do that, it's my first time seeing Github's UI with merge queues... At any rate, I think the PR is ready now, I've re-added |
|
That's too bad, in the meantime GitHub broke merge queues and Godot broke our This will have to wait a bit for a new opportunity window to merge... Thanks already for the great work nonetheless! |
7243a26 to
30aac2d
Compare
|
Thanks a lot for your contribution! 😊 |
|
By the way, I tested this PR just now on |
Currently, godot-rust uses some JavaScript to enumerate all constructors and call them in WASM build. However, Rust can nowadays leverage Emscripten's/
wasm-lds support for global constructors using the.init_arraysection, just like on Unix platforms. This is what therust-ctorandinventorycrates do. Aninventorypull request suggest that this should work in compilers newer than 1.85 ornightly-2024-12-18.I've personally tested it with the Dodge the Creeps example game. It worked in both threaded and unthreaded release and debug builds. The build configuration in the example is currently broken, so I used this Makefile to build it.
Breaking changes
This PR currently introduces breaking changes, as it removes the
ExtensionLibrary::override_wasm_binaryfunction, as it's no longer needed or used.This will of course break the build of anyone who overrides it. (Or anyone who calls it, but who does that?) This function is available even without the
experimental-wasmfeature.An alternative would be to just mark the function as
#[deprecated]until the next major release, or forever.Checks
It's worth noting that
inventoryadds astaticflag to each constructor on WebAssembly targets that makes the constructors idempotent by returning early on all but the first call.This is because
wasm-ldinserts a call to__wasm_call_ctorsto the start of every exported function if it heuristically detects "command-style linkage". It will do so only if__wasm_call_ctorsis neither called anywhere in the binary nor exported from the binary, i.e. if there is no chance that the constructors would be called otherwise. Emscripten will always mark__wasm_call_ctorsas exported (except in standalone mode, which we're not using and is probably incompatible with side modules).Since we're only supporting Emscripten, this should not be an issue for us. I added an assertion that emits a compile error when targeting non-Emscripten WebAssembly. It's worth noting that
inventoryonly added their checks when adding support for non-Emscripten targets.rust-ctordoesn't seem to contain any checks.We could of course still add some checks for repeated calls, but if we do, I think we should make repeated calls panic instead of doing nothing. If the built library doesn't call
__wasm_call_ctorsand doesn't export it either, then something must have gone wrong at build time. It's worth noting that if I manually call__wasm_call_ctorsa second time, godot-rust already panics withGodot class `…` is defined multiple times in Rust; ….