Skip to content

Add support for nothreads wasm builds (Godot 4.3+) #794

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

Merged
merged 1 commit into from
Jul 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions godot-bindings/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ homepage = "https://godot-rust.github.io"
# requiring no-default-features), so we unfortunately still need to depend on prebuilt and just ignore it.
# The artifact generator explicitly excludes that though (to avoid a quasi-circular dependency back to its repo).
[features]
experimental-wasm-nothreads = []

# [version-sync] [[
# [line] api-$kebabVersion = []
api-4-0 = []
Expand Down
16 changes: 16 additions & 0 deletions godot-bindings/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,22 @@ pub fn emit_godot_version_cfg() {
}
}

/// Emit `#[cfg(wasm_nothreads)]` flag when compiling to Wasm with the "experimental-wasm-nothreads" feature.
pub fn emit_wasm_nothreads_cfg() {
println!(r#"cargo:rustc-check-cfg=cfg(wasm_nothreads, values(none()))"#);

// The environment variable for target family has a list of applicable families separated by commas.
// For Emscripten in particular, this can be "unix,wasm". Therefore, to check for the Wasm target, we must check each item in the list.
#[cfg(feature = "experimental-wasm-nothreads")]
if std::env::var("CARGO_CFG_TARGET_FAMILY")
.expect("target family environment variable")
.split(',')
.any(|family| family == "wasm")
{
println!(r#"cargo:rustc-cfg=wasm_nothreads"#);
}
}

// Function for safely removal of build directory. Workaround for errors happening during CI builds:
// https://github.com/godot-rust/gdext/issues/616
pub fn remove_dir_all_reliable(path: &Path) {
Expand Down
1 change: 1 addition & 0 deletions godot-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ codegen-lazy-fptrs = [
double-precision = ["godot-codegen/double-precision"]
experimental-godot-api = ["godot-codegen/experimental-godot-api"]
experimental-threads = ["godot-ffi/experimental-threads"]
experimental-wasm-nothreads = ["godot-ffi/experimental-wasm-nothreads"]
debug-log = ["godot-ffi/debug-log"]
trace = []

Expand Down
1 change: 1 addition & 0 deletions godot-ffi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ codegen-rustfmt = ["godot-codegen/codegen-rustfmt"]
codegen-lazy-fptrs = ["godot-codegen/codegen-lazy-fptrs"]
experimental-godot-api = ["godot-codegen/experimental-godot-api"]
experimental-threads = []
experimental-wasm-nothreads = ["godot-bindings/experimental-wasm-nothreads"]
debug-log = []

api-custom = ["godot-bindings/api-custom"]
Expand Down
1 change: 1 addition & 0 deletions godot-ffi/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,5 @@ fn main() {
println!("cargo:rerun-if-changed=build.rs");

godot_bindings::emit_godot_version_cfg();
godot_bindings::emit_wasm_nothreads_cfg();
}
79 changes: 68 additions & 11 deletions godot-ffi/src/binding/single_threaded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,21 @@
//! If used from different threads then there will be runtime errors in debug mode and UB in release mode.

use std::cell::Cell;

#[cfg(not(wasm_nothreads))]
use std::thread::ThreadId;

use super::GodotBinding;
use crate::ManualInitCell;

pub(super) struct BindingStorage {
// No threading when linking against Godot with a nothreads Wasm build.
// Therefore, we just need to check if the bindings were initialized, as all accesses are from the main thread.
#[cfg(wasm_nothreads)]
initialized: Cell<bool>,

// Is used in to check that we've been called from the right thread, so must be thread-safe to access.
#[cfg(not(wasm_nothreads))]
main_thread_id: Cell<Option<ThreadId>>,
binding: ManualInitCell<GodotBinding>,
}
Expand All @@ -30,13 +38,59 @@ impl BindingStorage {
#[inline(always)]
unsafe fn storage() -> &'static Self {
static BINDING: BindingStorage = BindingStorage {
#[cfg(wasm_nothreads)]
initialized: Cell::new(false),

#[cfg(not(wasm_nothreads))]
main_thread_id: Cell::new(None),
binding: ManualInitCell::new(),
};

&BINDING
}

/// Returns whether the binding storage has already been initialized.
///
/// It is recommended to use this function for that purpose as the field to check varies depending on the compilation target.
fn initialized(&self) -> bool {
#[cfg(wasm_nothreads)]
return self.initialized.get();

#[cfg(not(wasm_nothreads))]
self.main_thread_id.get().is_some()
}

/// Marks the binding storage as initialized or deinitialized.
/// We store the thread ID to ensure future accesses to the binding only come from the main thread.
///
/// # Safety
/// Must be called from the main thread. Additionally, the binding storage must be initialized immediately
/// after this function if `initialized` is `true`, or deinitialized if it is `false`.
///
/// # Panics
/// If attempting to deinitialize before initializing, or vice-versa.
Comment on lines +70 to +71
Copy link
Member

Choose a reason for hiding this comment

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

The 2nd part is not checked though.

I think the precondition can be verified outside the #[cfg], e.g. like this:

if initialized == self.is_initialized() {
    if initialized {
        panic!("already initialized");
    } else {
        panic!("deinitialize without prior initialize");
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I think this means it wasn't checked before either. So we'll get a free bug fix :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Seems to be working fine.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Yes, it's always nice to have these drive-by bugfixes 🙂

unsafe fn set_initialized(&self, initialized: bool) {
if initialized == self.initialized() {
if initialized {
panic!("already initialized");
} else {
panic!("deinitialize without prior initialize");
}
}

// 'std::thread::current()' fails when linking to a Godot web build without threads. When compiling to wasm-nothreads,
// we assume it is impossible to have multi-threading, so checking if we are in the main thread is not needed.
// Therefore, we don't store the thread ID, but rather just whether initialization already occurred.
#[cfg(wasm_nothreads)]
self.initialized.set(initialized);

#[cfg(not(wasm_nothreads))]
{
let thread_id = initialized.then(|| std::thread::current().id());
self.main_thread_id.set(thread_id);
}
}

/// Initialize the binding storage, this must be called before any other public functions.
///
/// # Safety
Expand All @@ -49,9 +103,10 @@ impl BindingStorage {
// in which case we can tell that the storage has been initialized, and we don't access `binding`.
let storage = unsafe { Self::storage() };

storage
.main_thread_id
.set(Some(std::thread::current().id()));
// SAFETY: We are about to initialize the binding below, so marking the binding as initialized is correct.
// If we can't initialize the binding at this point, we get a panic before changing the status, thus the
// binding won't be set.
unsafe { storage.set_initialized(true) };

// SAFETY: We are the first thread to set this binding (possibly after deinitialize), as otherwise the above set() would fail and
// return early. We also know initialize() is not called concurrently with anything else that can call another method on the binding,
Expand All @@ -70,12 +125,10 @@ impl BindingStorage {
// SAFETY: We only call this once no other operations happen anymore, i.e. no other access to the binding.
let storage = unsafe { Self::storage() };

storage
.main_thread_id
.get()
.expect("deinitialize without prior initialize");

storage.main_thread_id.set(None);
// SAFETY: We are about to deinitialize the binding below, so marking the binding as deinitialized is correct.
// If we can't deinitialize the binding at this point, we get a panic before changing the status, thus the
// binding won't be deinitialized.
unsafe { storage.set_initialized(false) };

// SAFETY: We are the only thread that can access the binding, and we know that it's initialized.
unsafe {
Expand All @@ -92,7 +145,10 @@ impl BindingStorage {
pub unsafe fn get_binding_unchecked() -> &'static GodotBinding {
let storage = Self::storage();

if cfg!(debug_assertions) {
// We only check if we are in the main thread in debug builds if we aren't building for a non-threaded Godot build,
// since we could otherwise assume there won't be multi-threading.
#[cfg(all(debug_assertions, not(wasm_nothreads)))]
{
let main_thread_id = storage.main_thread_id.get().expect(
"Godot engine not available; make sure you are not calling it from unit/doc tests",
);
Expand All @@ -111,7 +167,8 @@ impl BindingStorage {
pub fn is_initialized() -> bool {
// SAFETY: We don't access the binding.
let storage = unsafe { Self::storage() };
storage.main_thread_id.get().is_some()

storage.initialized()
}
}

Expand Down
1 change: 1 addition & 0 deletions godot/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ double-precision = ["godot-core/double-precision"]
experimental-godot-api = ["godot-core/experimental-godot-api"]
experimental-threads = ["godot-core/experimental-threads"]
experimental-wasm = []
experimental-wasm-nothreads = ["godot-core/experimental-wasm-nothreads"]
codegen-rustfmt = ["godot-core/codegen-rustfmt"]
lazy-function-tables = ["godot-core/codegen-lazy-fptrs"]
serde = ["godot-core/serde"]
Expand Down
8 changes: 7 additions & 1 deletion godot/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
//! * **`api-custom`**
//!
//! Sets the [**API level**](https://godot-rust.github.io/book/toolchain/godot-version.html) to the specified Godot version,
//! or a custom-built local binary.
//! or a custom-built local binary.
//! You can use at most one `api-*` feature. If absent, the current Godot minor version is used, with patch level 0.<br><br>
//!
//! * **`double-precision`**
Expand Down Expand Up @@ -124,6 +124,12 @@ pub mod __docs;
#[cfg(all(feature = "lazy-function-tables", feature = "experimental-threads"))]
compile_error!("Thread safety for lazy function pointers is not yet implemented.");

#[cfg(all(
feature = "experimental-wasm-nothreads",
feature = "experimental-threads"
))]
compile_error!("Cannot use 'experimental-threads' with a nothreads Wasm build yet.");

#[cfg(all(target_family = "wasm", not(feature = "experimental-wasm")))]
compile_error!("Must opt-in using `experimental-wasm` Cargo feature; keep in mind that this is work in progress");

Expand Down
Loading