-
-
Notifications
You must be signed in to change notification settings - Fork 268
Add 32-bit architecture support (wasm32) #1444
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
Enable cross-compilation to 32-bit targets (e.g., wasm) by addressing bindgen layout test incompatibilities. Problem: Bindgen generates compile-time struct size/alignment assertions based on the host architecture (64-bit). When cross-compiling to 32-bit, these assertions fail because pointer sizes differ (8 bytes vs 4 bytes). Solution: - For `api-custom` mode: disable layout_tests in bindgen configuration - For prebuilt mode: strip layout assertion blocks only when targeting 32-bit (CARGO_CFG_TARGET_POINTER_WIDTH == "32") Native 64-bit builds retain all safety checks intact. The assertions are only removed for 32-bit targets where they would be incorrect anyway (validating 64-bit sizes against 32-bit structs).
Add a `wasm-check` job that verifies compilation for wasm32-unknown-emscripten. Since wasm32 is a 32-bit target, this catches pointer width regressions before merge. Related: godot-rust#347
|
Hi, and thank you for the contribution! I do not think that manually modifying the bindgen result is the right approach. We should generate Rust files for the correct target upstream in the prebuilt artifact. Unfortunately this is not as simple as opening a PR, I have to modify the generator itself and adjust some scripts. So I wanted to do this for a while; it's also what's blocking #1275 -- which would also add a first integrated CI support for Wasm. I'll try to find some time soon... Could I notify you once that's taking shape? Since you have a game and an established workflow, it would be cool to test it 🙂 |
The short version is -> it works surprisingly well, in terms of performance, the web "release" version is approaching the desktop debug version in terms of performance, which is not that bad. All inputs etc. -> worked like a charm. I understand hacking one's way through the generated bindgen result and ignoring the 32 vs 64 difference may not be how you want to build a solid, stable library. Makes sense. But I am indeed curious about what's next. I'll continue experimenting with web support, regardless of whether it's the right way to do it, it allows me to prototype and early-debunk possible blockers. Thanks for taking the time to consider the PR! |
|
Okay, add a patch to match the latest updates in godot-rust/godot4-prebuilt#2 With that, I have been able to do locally: ^ which, looks good. Thanks for your feedback, I think it looks a little better now indeed, preparing both bindings 32+64, upstream in godot4-prebuilt and using the right ones here. Will NOT work out-of-the-box as I had (as expected) to change my [EDIT] all this in multiple commits etc. -> happy to squash/rebase that, but keeping the complete history now as it may be easier to revert/hack while this is not fully baked. |
Prebuilt bindings previously used a runtime workaround that stripped layout tests when cross-compiling to 32-bit. This was fragile and didn't validate struct layouts on 32-bit targets. Changes: - Add TargetPointerWidth enum and generate_rust_binding_for_target() for generating bindings for a specific architecture via clang's --target flag - Export write_gdextension_headers_for_target() for godot4-prebuilt to generate both 32-bit and 64-bit bindings from a single host - Update prebuilt mode to select bindings using CARGO_CFG_TARGET_POINTER_WIDTH (the actual cross-compile target, not the host) - Remove strip_bindgen_layout_tests() workaround - no longer needed since prebuilt artifacts now include proper architecture-specific bindings. This should be squashed with previous commits, but until this is validated let's keep full history of what's there. Requirements: - this requires godot-rust/godot4-prebuilt#2 or similar, will not work with current tip-of-master.
|
Hello! First of all, I appreciate your enthusiasm. Unfortunately this PR is not up to our standards.
Why? // Only disable layout tests when cross-compiling between different pointer widths.
// Layout tests are generated based on the host architecture but validated on the target,
// which causes failures when cross-compiling (e.g., from 64-bit host to 32-bit target)
// because struct sizes differ. See: https://github.com/godot-rust/gdext/issues/347.If bindgen wouldn't be able to generate proper layout tests for target platform we compile to, then cross compiling would be pretty much impossible. The bindgen FAQ explicitly shows example with 32bit platform – You can verify that given checks are alright on your own by cross-compiling simple file... or by comparing If there is any problem with layout tests in a bindgen then it needs to be properly documented. In other words, we need to know:
Example: #1401. Avoid turning off all the safety checks – while flawed, they have a precise meaning and are there to shield us from very nasty bugs. Ignoring layout tests might result in catastrophic UB in the future. Don't "make problem go away", if there is any problem then it needs to be resolved. Nonetheless, thank you for your contribution! Once @Bromeon found the time to update the prebuilt artifacts, we'd be happy to hear if the new setup works for your case 🙏. Best, |
Thanks for the extended explanation & links. If anything, just know I am around and ready to experiment whatever acceptable solution there could be to have this 32-bit support. I'll close the PR to avoid maintainers spending time on it, as my understanding is that whatever is going to end up in |
Closes #347
This PR enables cross-compilation to 32-bit targets such as wasm32-unknown-emscripten.
Problem
Bindgen generates compile-time struct size/alignment assertions based on the host architecture. When cross-compiling from a 64-bit host to a 32-bit target, these assertions fail because pointer widths differ (8 bytes vs 4 bytes).
Solution
The fix handles both binding modes:
CI
Added a wasm-check job that verifies compilation for wasm32-unknown-emscripten. This should catch pointer width regressions. (not sure how I can check that, how CI works exactly etc. but I get a feeling adding a non-regression test won't harm, I suspect the real is more complex than this naive "add a target on matrix", cf #1275)
Testing
I have a working local prototype with my game https://gitlab.com/liberecofr/liquidwar7 running on wasm32.
Required Cargo.toml configuration:
The
getrandombit is because on that project, this crate is used, it's not strictly related to this PR but I thought it was interesting to mention it as a) if you want to reproduce, you'd need it and b) random number generation is not so rare in games, so the workaround may be interesting to know.Required .cargo/config.toml:
The
ERROR_ON_UNDEFINED_SYMBOLS=0is key otherwise my program would die with a cryptic error sayinginvoke_viiiis not defined...Commits