From fcf8b0f781d3da9e3afb9ea70a070d8c760cea26 Mon Sep 17 00:00:00 2001 From: Luo Zhihao Date: Sat, 11 Oct 2025 15:45:43 +0800 Subject: [PATCH 1/7] Add cargo features to configure safety checks --- godot-bindings/Cargo.toml | 3 ++ godot-bindings/build.rs | 2 +- godot-bindings/src/lib.rs | 20 +++++++++++ godot-codegen/src/generator/classes.rs | 5 +-- .../src/generator/native_structures.rs | 1 + godot-core/Cargo.toml | 3 ++ godot-core/build.rs | 1 + godot-core/src/builtin/collections/array.rs | 11 +++--- godot-core/src/classes/class_runtime.rs | 11 +++--- godot-core/src/meta/error/convert_error.rs | 4 +-- godot-core/src/meta/signature.rs | 4 +++ godot-core/src/obj/base.rs | 36 ++++++++++--------- godot-core/src/obj/casts.rs | 7 ++-- godot-core/src/obj/gd.rs | 5 ++- godot-core/src/obj/raw_gd.rs | 22 +++++++----- godot-core/src/obj/rtti.rs | 14 ++++---- godot-core/src/storage/mod.rs | 8 ++--- godot-ffi/Cargo.toml | 4 +++ godot-ffi/build.rs | 1 + godot-ffi/src/lib.rs | 16 +++++++-- godot/Cargo.toml | 4 +++ 21 files changed, 124 insertions(+), 58 deletions(-) diff --git a/godot-bindings/Cargo.toml b/godot-bindings/Cargo.toml index 9305c9b2f..6f7d60328 100644 --- a/godot-bindings/Cargo.toml +++ b/godot-bindings/Cargo.toml @@ -33,6 +33,9 @@ api-custom = ["dep:bindgen", "dep:regex", "dep:which"] api-custom-json = ["dep:nanoserde", "dep:bindgen", "dep:regex", "dep:which"] api-custom-extheader = [] +debug-checks-balanced = [] +release-checks-fast-unsafe = [] + [dependencies] gdextension-api = { workspace = true } diff --git a/godot-bindings/build.rs b/godot-bindings/build.rs index edceaa9f3..dbd915cf1 100644 --- a/godot-bindings/build.rs +++ b/godot-bindings/build.rs @@ -10,7 +10,7 @@ // It's the only purpose of this build.rs file. If a better solution is found, this file can be removed. #[rustfmt::skip] -fn main() { +fn main() { let mut count = 0; if cfg!(feature = "api-custom") { count += 1; } if cfg!(feature = "api-custom-json") { count += 1; } diff --git a/godot-bindings/src/lib.rs b/godot-bindings/src/lib.rs index 5e3346010..f522b23f2 100644 --- a/godot-bindings/src/lib.rs +++ b/godot-bindings/src/lib.rs @@ -267,3 +267,23 @@ pub fn before_api(major_minor: &str) -> bool { pub fn since_api(major_minor: &str) -> bool { !before_api(major_minor) } + +pub fn emit_checks_mode() { + let check_modes = ["fast-unsafe", "balanced", "paranoid"]; + let mut checks_level = if cfg!(debug_assertions) { 2 } else { 1 }; + #[cfg(debug_assertions)] + if cfg!(feature = "debug-checks-balanced") { + checks_level = 1; + } + #[cfg(not(debug_assertions))] + if cfg!(feature = "release-checks-fast-unsafe") { + checks_level = 0; + } + + for mode in check_modes.iter() { + println!(r#"cargo:rustc-check-cfg=cfg(checks_at_least, values("{mode}"))"#); + } + for mode in check_modes.iter().take(checks_level + 1) { + println!(r#"cargo:rustc-cfg=checks_at_least="{mode}""#); + } +} diff --git a/godot-codegen/src/generator/classes.rs b/godot-codegen/src/generator/classes.rs index 621946ef0..a8c48aa74 100644 --- a/godot-codegen/src/generator/classes.rs +++ b/godot-codegen/src/generator/classes.rs @@ -195,8 +195,9 @@ fn make_class(class: &Class, ctx: &mut Context, view: &ApiView) -> GeneratedClas fn __checked_id(&self) -> Option { // SAFETY: only Option due to layout-compatibility with RawGd; it is always Some because stored in Gd which is non-null. let rtti = unsafe { self.rtti.as_ref().unwrap_unchecked() }; - let instance_id = rtti.check_type::(); - Some(instance_id) + #[cfg(checks_at_least = "paranoid")] + rtti.check_type::(); + Some(rtti.instance_id()) } #[doc(hidden)] diff --git a/godot-codegen/src/generator/native_structures.rs b/godot-codegen/src/generator/native_structures.rs index 435b373b8..6c324051a 100644 --- a/godot-codegen/src/generator/native_structures.rs +++ b/godot-codegen/src/generator/native_structures.rs @@ -219,6 +219,7 @@ fn make_native_structure_field_and_accessor( let obj = #snake_field_name.upcast(); + #[cfg(checks_at_least = "balanced")] assert!(obj.is_instance_valid(), "provided node is dead"); let id = obj.instance_id().to_u64(); diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index a31cd61a1..486729b37 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -38,6 +38,9 @@ api-4-4 = ["godot-ffi/api-4-4"] api-4-5 = ["godot-ffi/api-4-5"] # ]] +debug-checks-balanced = ["godot-ffi/debug-checks-balanced"] +release-checks-fast-unsafe = ["godot-ffi/release-checks-fast-unsafe"] + [dependencies] godot-ffi = { path = "../godot-ffi", version = "=0.4.1" } diff --git a/godot-core/build.rs b/godot-core/build.rs index 624c73ebf..c7fb9707b 100644 --- a/godot-core/build.rs +++ b/godot-core/build.rs @@ -18,4 +18,5 @@ fn main() { godot_bindings::emit_godot_version_cfg(); godot_bindings::emit_wasm_nothreads_cfg(); + godot_bindings::emit_checks_mode(); } diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index a3beccd86..a1c6ee328 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -968,7 +968,7 @@ impl Array { } /// Validates that all elements in this array can be converted to integers of type `T`. - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] pub(crate) fn debug_validate_int_elements(&self) -> Result<(), ConvertError> { // SAFETY: every element is internally represented as Variant. let canonical_array = unsafe { self.assume_type_ref::() }; @@ -990,7 +990,7 @@ impl Array { } // No-op in Release. Avoids O(n) conversion checks, but still panics on access. - #[cfg(not(debug_assertions))] + #[cfg(not(checks_at_least = "paranoid"))] pub(crate) fn debug_validate_int_elements(&self) -> Result<(), ConvertError> { Ok(()) } @@ -1233,10 +1233,9 @@ impl Clone for Array { let copy = unsafe { self.clone_unchecked() }; // Double-check copy's runtime type in Debug mode. - if cfg!(debug_assertions) { - copy.with_checked_type().unwrap_or_else(|e| { - panic!("copied array should have same type as original array: {e}") - }) + if cfg!(checks_at_least = "paranoid") { + copy.with_checked_type() + .expect("copied array should have same type as original array") } else { copy } diff --git a/godot-core/src/classes/class_runtime.rs b/godot-core/src/classes/class_runtime.rs index f73ecada4..8573710bd 100644 --- a/godot-core/src/classes/class_runtime.rs +++ b/godot-core/src/classes/class_runtime.rs @@ -8,10 +8,10 @@ //! Runtime checks and inspection of Godot classes. use crate::builtin::{GString, StringName, Variant, VariantType}; -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] use crate::classes::{ClassDb, Object}; use crate::meta::CallContext; -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] use crate::meta::ClassId; use crate::obj::{bounds, Bounds, Gd, GodotClass, InstanceId, RawGd, Singleton}; use crate::sys; @@ -191,6 +191,7 @@ where Gd::::from_obj_sys(object_ptr) } +#[cfg(checks_at_least = "balanced")] pub(crate) fn ensure_object_alive( instance_id: InstanceId, old_object_ptr: sys::GDExtensionObjectPtr, @@ -211,7 +212,7 @@ pub(crate) fn ensure_object_alive( ); } -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] pub(crate) fn ensure_object_inherits(derived: ClassId, base: ClassId, instance_id: InstanceId) { if derived == base || base == Object::class_id() // for Object base, anything inherits by definition @@ -226,7 +227,7 @@ pub(crate) fn ensure_object_inherits(derived: ClassId, base: ClassId, instance_i ) } -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] pub(crate) fn ensure_binding_not_null(binding: sys::GDExtensionClassInstancePtr) where T: GodotClass + Bounds, @@ -254,7 +255,7 @@ where // Implementation of this file /// Checks if `derived` inherits from `base`, using a cache for _successful_ queries. -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] fn is_derived_base_cached(derived: ClassId, base: ClassId) -> bool { use std::collections::HashSet; diff --git a/godot-core/src/meta/error/convert_error.rs b/godot-core/src/meta/error/convert_error.rs index 12fa2f296..4e019d9e8 100644 --- a/godot-core/src/meta/error/convert_error.rs +++ b/godot-core/src/meta/error/convert_error.rs @@ -189,7 +189,7 @@ pub(crate) enum FromGodotError { }, /// Special case of `BadArrayType` where a custom int type such as `i8` cannot hold a dynamic `i64` value. - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] BadArrayTypeInt { expected_int_type: &'static str, value: i64, @@ -234,7 +234,7 @@ impl fmt::Display for FromGodotError { write!(f, "expected array of type {exp_class}, got {act_class}") } - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] Self::BadArrayTypeInt { expected_int_type, value, diff --git a/godot-core/src/meta/signature.rs b/godot-core/src/meta/signature.rs index 0af81e3eb..dadb4b02b 100644 --- a/godot-core/src/meta/signature.rs +++ b/godot-core/src/meta/signature.rs @@ -144,6 +144,8 @@ impl Signature { //$crate::out!("out_class_varcall: {call_ctx}"); // Note: varcalls are not safe from failing, if they happen through an object pointer -> validity check necessary. + // paranoid since we already check the validity in check_rtti, this is unlikely to happen. + #[cfg(checks_at_least = "paranoid")] if let Some(instance_id) = maybe_instance_id { crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx); } @@ -304,6 +306,8 @@ impl Signature { let call_ctx = CallContext::outbound(class_name, method_name); // $crate::out!("out_class_ptrcall: {call_ctx}"); + // paranoid since we already check the validity in check_rtti, this is unlikely to happen. + #[cfg(checks_at_least = "paranoid")] if let Some(instance_id) = maybe_instance_id { crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx); } diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index 1e02e038c..adae2e9e9 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] use std::cell::Cell; use std::cell::RefCell; use std::collections::hash_map::Entry; @@ -27,7 +27,7 @@ thread_local! { } /// Represents the initialization state of a `Base` object. -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum InitState { /// Object is being constructed (inside `I*::init()` or `Gd::from_init_fn()`). @@ -38,14 +38,14 @@ enum InitState { Script, } -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] macro_rules! base_from_obj { ($obj:expr, $state:expr) => { Base::from_obj($obj, $state) }; } -#[cfg(not(debug_assertions))] +#[cfg(not(checks_at_least = "paranoid"))] macro_rules! base_from_obj { ($obj:expr, $state:expr) => { Base::from_obj($obj) @@ -82,7 +82,7 @@ pub struct Base { /// Tracks the initialization state of this `Base` in Debug mode. /// /// Rc allows to "copy-construct" the base from an existing one, while still affecting the user-instance through the original `Base`. - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] init_state: Rc>, } @@ -95,13 +95,14 @@ impl Base { /// `base` must be alive at the time of invocation, i.e. user `init()` (which could technically destroy it) must not have run yet. /// If `base` is destroyed while the returned `Base` is in use, that constitutes a logic error, not a safety issue. pub(crate) unsafe fn from_base(base: &Base) -> Base { - debug_assert!(base.obj.is_instance_valid()); + #[cfg(checks_at_least = "paranoid")] + assert!(base.obj.is_instance_valid()); let obj = Gd::from_obj_sys_weak(base.obj.obj_sys()); Self { obj: ManuallyDrop::new(obj), - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] init_state: Rc::clone(&base.init_state), } } @@ -114,7 +115,8 @@ impl Base { /// `gd` must be alive at the time of invocation. If it is destroyed while the returned `Base` is in use, that constitutes a logic /// error, not a safety issue. pub(crate) unsafe fn from_script_gd(gd: &Gd) -> Self { - debug_assert!(gd.is_instance_valid()); + #[cfg(checks_at_least = "paranoid")] + assert!(gd.is_instance_valid()); let obj = Gd::from_obj_sys_weak(gd.obj_sys()); base_from_obj!(obj, InitState::Script) @@ -141,7 +143,7 @@ impl Base { base_from_obj!(obj, InitState::ObjectConstructing) } - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] fn from_obj(obj: Gd, init_state: InitState) -> Self { Self { obj: ManuallyDrop::new(obj), @@ -149,7 +151,7 @@ impl Base { } } - #[cfg(not(debug_assertions))] + #[cfg(not(checks_at_least = "paranoid"))] fn from_obj(obj: Gd) -> Self { Self { obj: ManuallyDrop::new(obj), @@ -176,7 +178,7 @@ impl Base { /// # Panics (Debug) /// If called outside an initialization function, or for ref-counted objects on a non-main thread. pub fn to_init_gd(&self) -> Gd { - #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols. + #[cfg(checks_at_least = "paranoid")] // debug_assert! still checks existence of symbols. assert!( self.is_initializing(), "Base::to_init_gd() can only be called during object initialization, inside I*::init() or Gd::from_init_fn()" @@ -248,7 +250,7 @@ impl Base { /// Finalizes the initialization of this `Base` and returns whether pub(crate) fn mark_initialized(&mut self) { - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] { assert_eq!( self.init_state.get(), @@ -265,7 +267,7 @@ impl Base { /// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed. #[doc(hidden)] pub fn __fully_constructed_gd(&self) -> Gd { - #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols. + #[cfg(checks_at_least = "paranoid")] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" @@ -296,7 +298,7 @@ impl Base { /// Returns a passive reference to the base object, for use in script contexts only. pub(crate) fn to_script_passive(&self) -> PassiveGd { - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] assert_eq!( self.init_state.get(), InitState::Script, @@ -308,7 +310,7 @@ impl Base { } /// Returns `true` if this `Base` is currently in the initializing state. - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] fn is_initializing(&self) -> bool { self.init_state.get() == InitState::ObjectConstructing } @@ -316,7 +318,7 @@ impl Base { /// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed. #[doc(hidden)] pub fn __constructed_gd(&self) -> Gd { - #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols. + #[cfg(checks_at_least = "paranoid")] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" @@ -333,7 +335,7 @@ impl Base { /// # Safety /// Caller must ensure that the underlying object remains valid for the entire lifetime of the returned `PassiveGd`. pub(crate) unsafe fn constructed_passive(&self) -> PassiveGd { - #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols. + #[cfg(checks_at_least = "paranoid")] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" diff --git a/godot-core/src/obj/casts.rs b/godot-core/src/obj/casts.rs index b0504c68b..820c6c475 100644 --- a/godot-core/src/obj/casts.rs +++ b/godot-core/src/obj/casts.rs @@ -48,7 +48,7 @@ impl CastSuccess { } /// Access shared reference to destination, without consuming object. - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] pub fn as_dest_ref(&self) -> &RawGd { self.check_validity(); &self.dest @@ -56,6 +56,7 @@ impl CastSuccess { /// Access exclusive reference to destination, without consuming object. pub fn as_dest_mut(&mut self) -> &mut RawGd { + #[cfg(checks_at_least = "paranoid")] self.check_validity(); &mut self.dest } @@ -70,13 +71,15 @@ impl CastSuccess { self.dest.instance_id_unchecked(), "traded_source must point to the same object as the destination" ); + #[cfg(checks_at_least = "paranoid")] self.check_validity(); std::mem::forget(traded_source); ManuallyDrop::into_inner(self.dest) } + #[cfg(checks_at_least = "paranoid")] fn check_validity(&self) { - debug_assert!(self.dest.is_null() || self.dest.is_instance_valid()); + assert!(self.dest.is_null() || self.dest.is_instance_valid()); } } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 474511f86..86714e796 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -783,7 +783,9 @@ where } // If ref_counted returned None, that means the instance was destroyed - if ref_counted != Some(false) || !self.is_instance_valid() { + if ref_counted != Some(false) + || (cfg!(checks_at_least = "balanced") && !self.is_instance_valid()) + { return error_or_panic("called free() on already destroyed object".to_string()); } @@ -791,6 +793,7 @@ where // static type information to be correct. This is a no-op in Release mode. // Skip check during panic unwind; would need to rewrite whole thing to use Result instead. Having BOTH panic-in-panic and bad type is // a very unlikely corner case. + #[cfg(checks_at_least = "paranoid")] if !is_panic_unwind { self.raw.check_dynamic_type(&CallContext::gd::("free")); } diff --git a/godot-core/src/obj/raw_gd.rs b/godot-core/src/obj/raw_gd.rs index ef7d6a913..743a066f8 100644 --- a/godot-core/src/obj/raw_gd.rs +++ b/godot-core/src/obj/raw_gd.rs @@ -360,7 +360,7 @@ impl RawGd { debug_assert!(!self.is_null(), "cannot upcast null object refs"); // In Debug builds, go the long path via Godot FFI to verify the results are the same. - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] { // SAFETY: we forget the object below and do not leave the function before. let ffi_dest = self.ffi_cast::().expect("failed FFI upcast"); @@ -381,16 +381,22 @@ impl RawGd { } } - /// Verify that the object is non-null and alive. In Debug mode, additionally verify that it is of type `T` or derived. + /// Verify that the object is non-null and alive. In paranoid mode, additionally verify that it is of type `T` or derived. pub(crate) fn check_rtti(&self, method_name: &'static str) { - let call_ctx = CallContext::gd::(method_name); + #[cfg(checks_at_least = "balanced")] + { + let call_ctx = CallContext::gd::(method_name); + #[cfg(checks_at_least = "paranoid")] + self.check_dynamic_type(&call_ctx); + let instance_id = unsafe { self.instance_id_unchecked().unwrap_unchecked() }; - let instance_id = self.check_dynamic_type(&call_ctx); - classes::ensure_object_alive(instance_id, self.obj_sys(), &call_ctx); + classes::ensure_object_alive(instance_id, self.obj_sys(), &call_ctx); + } } /// Checks only type, not alive-ness. Used in Gd in case of `free()`. - pub(crate) fn check_dynamic_type(&self, call_ctx: &CallContext<'static>) -> InstanceId { + #[cfg(checks_at_least = "paranoid")] + pub(crate) fn check_dynamic_type(&self, call_ctx: &CallContext<'static>) { debug_assert!( !self.is_null(), "{call_ctx}: cannot call method on null object", @@ -400,7 +406,7 @@ impl RawGd { // SAFETY: code surrounding RawGd ensures that `self` is non-null; above is just a sanity check against internal bugs. let rtti = unsafe { rtti.unwrap_unchecked() }; - rtti.check_type::() + rtti.check_type::(); } // Not pub(super) because used by godot::meta::args::ObjectArg. @@ -509,7 +515,7 @@ where let ptr: sys::GDExtensionClassInstancePtr = binding.cast(); - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] crate::classes::ensure_binding_not_null::(ptr); self.cached_storage_ptr.set(ptr); diff --git a/godot-core/src/obj/rtti.rs b/godot-core/src/obj/rtti.rs index 114cf8861..c26d9e228 100644 --- a/godot-core/src/obj/rtti.rs +++ b/godot-core/src/obj/rtti.rs @@ -21,7 +21,7 @@ pub struct ObjectRtti { instance_id: InstanceId, /// Only in Debug mode: dynamic class. - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] class_name: crate::meta::ClassId, // // TODO(bromeon): class_id is not always most-derived class; ObjectRtti is sometimes constructed from a base class, via RawGd::from_obj_sys_weak(). @@ -36,21 +36,19 @@ impl ObjectRtti { Self { instance_id, - #[cfg(debug_assertions)] + #[cfg(checks_at_least = "paranoid")] class_name: T::class_id(), } } - /// Checks that the object is of type `T` or derived. Returns instance ID. + /// Checks that the object is of type `T` or derived. /// /// # Panics - /// In Debug mode, if the object is not of type `T` or derived. + /// In paranoid mode, if the object is not of type `T` or derived. + #[cfg(checks_at_least = "paranoid")] #[inline] - pub fn check_type(&self) -> InstanceId { - #[cfg(debug_assertions)] + pub fn check_type(&self) { crate::classes::ensure_object_inherits(self.class_name, T::class_id(), self.instance_id); - - self.instance_id } #[inline] diff --git a/godot-core/src/storage/mod.rs b/godot-core/src/storage/mod.rs index 3061b6b5d..3f454b167 100644 --- a/godot-core/src/storage/mod.rs +++ b/godot-core/src/storage/mod.rs @@ -123,14 +123,14 @@ mod log_inactive { // ---------------------------------------------------------------------------------------------------------------------------------------------- // Tracking borrows in Debug mode -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] use borrow_info::DebugBorrowTracker; -#[cfg(not(debug_assertions))] +#[cfg(not(checks_at_least = "paranoid"))] use borrow_info_noop::DebugBorrowTracker; use crate::obj::{Base, GodotClass}; -#[cfg(debug_assertions)] +#[cfg(checks_at_least = "paranoid")] mod borrow_info { use std::backtrace::Backtrace; use std::fmt; @@ -195,7 +195,7 @@ mod borrow_info { } } -#[cfg(not(debug_assertions))] +#[cfg(not(checks_at_least = "paranoid"))] mod borrow_info_noop { use std::fmt; diff --git a/godot-ffi/Cargo.toml b/godot-ffi/Cargo.toml index f99655d0a..b612b81aa 100644 --- a/godot-ffi/Cargo.toml +++ b/godot-ffi/Cargo.toml @@ -30,6 +30,10 @@ api-4-4 = ["godot-bindings/api-4-4"] api-4-5 = ["godot-bindings/api-4-5"] # ]] + +debug-checks-balanced = ["godot-bindings/debug-checks-balanced"] +release-checks-fast-unsafe = ["godot-bindings/release-checks-fast-unsafe"] + [dependencies] [target.'cfg(target_os = "linux")'.dependencies] diff --git a/godot-ffi/build.rs b/godot-ffi/build.rs index aa08ea282..8b327ee8a 100644 --- a/godot-ffi/build.rs +++ b/godot-ffi/build.rs @@ -29,4 +29,5 @@ fn main() { godot_bindings::emit_godot_version_cfg(); godot_bindings::emit_wasm_nothreads_cfg(); + godot_bindings::emit_checks_mode(); } diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index ad7e6655e..47a4a48bc 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -254,11 +254,23 @@ pub unsafe fn deinitialize() { } } +fn safety_checks_string() -> &'static str { + if cfg!(checks_at_least = "paranoid") { + "paranoid" + } else if cfg!(checks_at_least = "balanced") { + "balanced" + } else if cfg!(checks_at_least = "fast-unsafe") { + "fast-unsafe" + } else { + unreachable!(); + } +} + fn print_preamble(version: GDExtensionGodotVersion) { let api_version: &'static str = GdextBuild::godot_static_version_string(); let runtime_version = read_version_string(&version); - - println!("Initialize godot-rust (API {api_version}, runtime {runtime_version})"); + let checks_mode = safety_checks_string(); + println!("Initialize godot-rust (API {api_version}, runtime {runtime_version}, safety checks {checks_mode})"); } /// # Safety diff --git a/godot/Cargo.toml b/godot/Cargo.toml index 7a373841a..72d897d2d 100644 --- a/godot/Cargo.toml +++ b/godot/Cargo.toml @@ -39,6 +39,10 @@ api-4-4 = ["godot-core/api-4-4"] api-4-5 = ["godot-core/api-4-5"] # ]] + +debug-checks-balanced = ["godot-core/debug-checks-balanced"] +release-checks-fast-unsafe = ["godot-core/release-checks-fast-unsafe"] + default = ["__codegen-full"] # Private features, they are under no stability guarantee From 3bffdf60e7b13d683ceaa6af379f1281ff983b98 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 25 Oct 2025 17:56:31 +0200 Subject: [PATCH 2/7] Terminology around safety/safeguard levels Renames: - "checks" -> "safeguards" - "debug" -> "dev" (matches Cargo profile) - "paranoid" -> "strict" - "fast-unsafe" -> "disengaged" --- godot-bindings/Cargo.toml | 5 +- godot-bindings/src/lib.rs | 22 +++---- godot-codegen/src/generator/classes.rs | 2 +- .../src/generator/native_structures.rs | 2 +- godot-core/Cargo.toml | 5 +- godot-core/build.rs | 2 +- godot-core/src/builtin/collections/array.rs | 6 +- godot-core/src/classes/class_runtime.rs | 12 ++-- godot-core/src/init/mod.rs | 16 +++++ godot-core/src/meta/error/convert_error.rs | 4 +- godot-core/src/meta/signature.rs | 4 +- godot-core/src/obj/base.rs | 34 +++++------ godot-core/src/obj/casts.rs | 8 +-- godot-core/src/obj/gd.rs | 4 +- godot-core/src/obj/raw_gd.rs | 10 ++-- godot-core/src/obj/rtti.rs | 6 +- godot-core/src/storage/mod.rs | 8 +-- godot-ffi/Cargo.toml | 6 +- godot-ffi/build.rs | 2 +- godot-ffi/src/lib.rs | 16 ++--- godot/Cargo.toml | 6 +- godot/src/lib.rs | 59 ++++++++++++++++++- 22 files changed, 155 insertions(+), 84 deletions(-) diff --git a/godot-bindings/Cargo.toml b/godot-bindings/Cargo.toml index 6f7d60328..d257389fc 100644 --- a/godot-bindings/Cargo.toml +++ b/godot-bindings/Cargo.toml @@ -33,8 +33,9 @@ api-custom = ["dep:bindgen", "dep:regex", "dep:which"] api-custom-json = ["dep:nanoserde", "dep:bindgen", "dep:regex", "dep:which"] api-custom-extheader = [] -debug-checks-balanced = [] -release-checks-fast-unsafe = [] +# Safeguard levels (see godot/lib.rs for detailed documentation). +safeguards-dev-balanced = [] +safeguards-release-disengaged = [] [dependencies] gdextension-api = { workspace = true } diff --git a/godot-bindings/src/lib.rs b/godot-bindings/src/lib.rs index f522b23f2..ce7642c8a 100644 --- a/godot-bindings/src/lib.rs +++ b/godot-bindings/src/lib.rs @@ -268,22 +268,22 @@ pub fn since_api(major_minor: &str) -> bool { !before_api(major_minor) } -pub fn emit_checks_mode() { - let check_modes = ["fast-unsafe", "balanced", "paranoid"]; - let mut checks_level = if cfg!(debug_assertions) { 2 } else { 1 }; +pub fn emit_safeguard_levels() { + let safeguard_modes = ["disengaged", "balanced", "strict"]; + let mut safeguards_level = if cfg!(debug_assertions) { 2 } else { 1 }; #[cfg(debug_assertions)] - if cfg!(feature = "debug-checks-balanced") { - checks_level = 1; + if cfg!(feature = "safeguards-dev-balanced") { + safeguards_level = 1; } #[cfg(not(debug_assertions))] - if cfg!(feature = "release-checks-fast-unsafe") { - checks_level = 0; + if cfg!(feature = "safeguards-release-disengaged") { + safeguards_level = 0; } - for mode in check_modes.iter() { - println!(r#"cargo:rustc-check-cfg=cfg(checks_at_least, values("{mode}"))"#); + for mode in safeguard_modes.iter() { + println!(r#"cargo:rustc-check-cfg=cfg(safeguards_at_least, values("{mode}"))"#); } - for mode in check_modes.iter().take(checks_level + 1) { - println!(r#"cargo:rustc-cfg=checks_at_least="{mode}""#); + for mode in safeguard_modes.iter().take(safeguards_level + 1) { + println!(r#"cargo:rustc-cfg=safeguards_at_least="{mode}""#); } } diff --git a/godot-codegen/src/generator/classes.rs b/godot-codegen/src/generator/classes.rs index a8c48aa74..9b35b05a3 100644 --- a/godot-codegen/src/generator/classes.rs +++ b/godot-codegen/src/generator/classes.rs @@ -195,7 +195,7 @@ fn make_class(class: &Class, ctx: &mut Context, view: &ApiView) -> GeneratedClas fn __checked_id(&self) -> Option { // SAFETY: only Option due to layout-compatibility with RawGd; it is always Some because stored in Gd which is non-null. let rtti = unsafe { self.rtti.as_ref().unwrap_unchecked() }; - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] rtti.check_type::(); Some(rtti.instance_id()) } diff --git a/godot-codegen/src/generator/native_structures.rs b/godot-codegen/src/generator/native_structures.rs index 6c324051a..3b6a7bb3b 100644 --- a/godot-codegen/src/generator/native_structures.rs +++ b/godot-codegen/src/generator/native_structures.rs @@ -219,7 +219,7 @@ fn make_native_structure_field_and_accessor( let obj = #snake_field_name.upcast(); - #[cfg(checks_at_least = "balanced")] + #[cfg(safeguards_at_least = "balanced")] assert!(obj.is_instance_valid(), "provided node is dead"); let id = obj.instance_id().to_u64(); diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index 486729b37..18f4b4124 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -38,8 +38,9 @@ api-4-4 = ["godot-ffi/api-4-4"] api-4-5 = ["godot-ffi/api-4-5"] # ]] -debug-checks-balanced = ["godot-ffi/debug-checks-balanced"] -release-checks-fast-unsafe = ["godot-ffi/release-checks-fast-unsafe"] +# Safeguard levels (see godot/lib.rs for detailed documentation). +safeguards-dev-balanced = ["godot-ffi/safeguards-dev-balanced"] +safeguards-release-disengaged = ["godot-ffi/safeguards-release-disengaged"] [dependencies] godot-ffi = { path = "../godot-ffi", version = "=0.4.1" } diff --git a/godot-core/build.rs b/godot-core/build.rs index c7fb9707b..1cb9dd503 100644 --- a/godot-core/build.rs +++ b/godot-core/build.rs @@ -18,5 +18,5 @@ fn main() { godot_bindings::emit_godot_version_cfg(); godot_bindings::emit_wasm_nothreads_cfg(); - godot_bindings::emit_checks_mode(); + godot_bindings::emit_safeguard_levels(); } diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index a1c6ee328..0cf63b47e 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -968,7 +968,7 @@ impl Array { } /// Validates that all elements in this array can be converted to integers of type `T`. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] pub(crate) fn debug_validate_int_elements(&self) -> Result<(), ConvertError> { // SAFETY: every element is internally represented as Variant. let canonical_array = unsafe { self.assume_type_ref::() }; @@ -990,7 +990,7 @@ impl Array { } // No-op in Release. Avoids O(n) conversion checks, but still panics on access. - #[cfg(not(checks_at_least = "paranoid"))] + #[cfg(not(safeguards_at_least = "strict"))] pub(crate) fn debug_validate_int_elements(&self) -> Result<(), ConvertError> { Ok(()) } @@ -1233,7 +1233,7 @@ impl Clone for Array { let copy = unsafe { self.clone_unchecked() }; // Double-check copy's runtime type in Debug mode. - if cfg!(checks_at_least = "paranoid") { + if cfg!(safeguards_at_least = "strict") { copy.with_checked_type() .expect("copied array should have same type as original array") } else { diff --git a/godot-core/src/classes/class_runtime.rs b/godot-core/src/classes/class_runtime.rs index 8573710bd..318bdaa1e 100644 --- a/godot-core/src/classes/class_runtime.rs +++ b/godot-core/src/classes/class_runtime.rs @@ -8,10 +8,10 @@ //! Runtime checks and inspection of Godot classes. use crate::builtin::{GString, StringName, Variant, VariantType}; -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] use crate::classes::{ClassDb, Object}; use crate::meta::CallContext; -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] use crate::meta::ClassId; use crate::obj::{bounds, Bounds, Gd, GodotClass, InstanceId, RawGd, Singleton}; use crate::sys; @@ -191,7 +191,7 @@ where Gd::::from_obj_sys(object_ptr) } -#[cfg(checks_at_least = "balanced")] +#[cfg(safeguards_at_least = "balanced")] pub(crate) fn ensure_object_alive( instance_id: InstanceId, old_object_ptr: sys::GDExtensionObjectPtr, @@ -212,7 +212,7 @@ pub(crate) fn ensure_object_alive( ); } -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] pub(crate) fn ensure_object_inherits(derived: ClassId, base: ClassId, instance_id: InstanceId) { if derived == base || base == Object::class_id() // for Object base, anything inherits by definition @@ -227,7 +227,7 @@ pub(crate) fn ensure_object_inherits(derived: ClassId, base: ClassId, instance_i ) } -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] pub(crate) fn ensure_binding_not_null(binding: sys::GDExtensionClassInstancePtr) where T: GodotClass + Bounds, @@ -255,7 +255,7 @@ where // Implementation of this file /// Checks if `derived` inherits from `base`, using a cache for _successful_ queries. -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] fn is_derived_base_cached(derived: ClassId, base: ClassId) -> bool { use std::collections::HashSet; diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index a1eee5ada..87e7283af 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -311,8 +311,12 @@ fn gdext_on_level_deinit(level: InitLevel) { /// responsible to uphold them: namely in GDScript code or other GDExtension bindings loaded by the engine. /// Violating this may cause undefined behavior, even when invoking _safe_ functions. /// +/// If you use the `disengaged` [safeguard level], you accept that UB becomes possible even **in safe Rust APIs**, if you use them wrong +/// (e.g. accessing a destroyed object). +/// /// [gdextension]: attr.gdextension.html /// [safety]: https://godot-rust.github.io/book/gdext/advanced/safety.html +/// [safeguard level]: ../index.html#safeguard-levels // FIXME intra-doc link #[doc(alias = "entry_symbol", alias = "entry_point")] pub unsafe trait ExtensionLibrary { @@ -530,6 +534,18 @@ unsafe fn ensure_godot_features_compatible() { out!("Check Godot precision setting..."); + #[cfg(feature = "debug-log")] // Display safeguards level in debug log. + let safeguards_level = if cfg!(safeguards_at_least = "strict") { + "strict" + } else if cfg!(safeguards_at_least = "balanced") { + "balanced" + } else if cfg!(safeguards_at_least = "disengaged") { + "disengaged" + } else { + "unknown" + }; + out!("Safeguards: {safeguards_level}"); + let os_class = StringName::from("OS"); let single = GString::from("single"); let double = GString::from("double"); diff --git a/godot-core/src/meta/error/convert_error.rs b/godot-core/src/meta/error/convert_error.rs index 4e019d9e8..53eaf74b0 100644 --- a/godot-core/src/meta/error/convert_error.rs +++ b/godot-core/src/meta/error/convert_error.rs @@ -189,7 +189,7 @@ pub(crate) enum FromGodotError { }, /// Special case of `BadArrayType` where a custom int type such as `i8` cannot hold a dynamic `i64` value. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] BadArrayTypeInt { expected_int_type: &'static str, value: i64, @@ -234,7 +234,7 @@ impl fmt::Display for FromGodotError { write!(f, "expected array of type {exp_class}, got {act_class}") } - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] Self::BadArrayTypeInt { expected_int_type, value, diff --git a/godot-core/src/meta/signature.rs b/godot-core/src/meta/signature.rs index dadb4b02b..b75717602 100644 --- a/godot-core/src/meta/signature.rs +++ b/godot-core/src/meta/signature.rs @@ -145,7 +145,7 @@ impl Signature { // Note: varcalls are not safe from failing, if they happen through an object pointer -> validity check necessary. // paranoid since we already check the validity in check_rtti, this is unlikely to happen. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] if let Some(instance_id) = maybe_instance_id { crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx); } @@ -307,7 +307,7 @@ impl Signature { // $crate::out!("out_class_ptrcall: {call_ctx}"); // paranoid since we already check the validity in check_rtti, this is unlikely to happen. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] if let Some(instance_id) = maybe_instance_id { crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx); } diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index adae2e9e9..f1201e316 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] use std::cell::Cell; use std::cell::RefCell; use std::collections::hash_map::Entry; @@ -27,7 +27,7 @@ thread_local! { } /// Represents the initialization state of a `Base` object. -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum InitState { /// Object is being constructed (inside `I*::init()` or `Gd::from_init_fn()`). @@ -38,14 +38,14 @@ enum InitState { Script, } -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] macro_rules! base_from_obj { ($obj:expr, $state:expr) => { Base::from_obj($obj, $state) }; } -#[cfg(not(checks_at_least = "paranoid"))] +#[cfg(not(safeguards_at_least = "strict"))] macro_rules! base_from_obj { ($obj:expr, $state:expr) => { Base::from_obj($obj) @@ -82,7 +82,7 @@ pub struct Base { /// Tracks the initialization state of this `Base` in Debug mode. /// /// Rc allows to "copy-construct" the base from an existing one, while still affecting the user-instance through the original `Base`. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] init_state: Rc>, } @@ -95,14 +95,14 @@ impl Base { /// `base` must be alive at the time of invocation, i.e. user `init()` (which could technically destroy it) must not have run yet. /// If `base` is destroyed while the returned `Base` is in use, that constitutes a logic error, not a safety issue. pub(crate) unsafe fn from_base(base: &Base) -> Base { - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] assert!(base.obj.is_instance_valid()); let obj = Gd::from_obj_sys_weak(base.obj.obj_sys()); Self { obj: ManuallyDrop::new(obj), - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] init_state: Rc::clone(&base.init_state), } } @@ -115,7 +115,7 @@ impl Base { /// `gd` must be alive at the time of invocation. If it is destroyed while the returned `Base` is in use, that constitutes a logic /// error, not a safety issue. pub(crate) unsafe fn from_script_gd(gd: &Gd) -> Self { - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] assert!(gd.is_instance_valid()); let obj = Gd::from_obj_sys_weak(gd.obj_sys()); @@ -143,7 +143,7 @@ impl Base { base_from_obj!(obj, InitState::ObjectConstructing) } - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] fn from_obj(obj: Gd, init_state: InitState) -> Self { Self { obj: ManuallyDrop::new(obj), @@ -151,7 +151,7 @@ impl Base { } } - #[cfg(not(checks_at_least = "paranoid"))] + #[cfg(not(safeguards_at_least = "strict"))] fn from_obj(obj: Gd) -> Self { Self { obj: ManuallyDrop::new(obj), @@ -178,7 +178,7 @@ impl Base { /// # Panics (Debug) /// If called outside an initialization function, or for ref-counted objects on a non-main thread. pub fn to_init_gd(&self) -> Gd { - #[cfg(checks_at_least = "paranoid")] // debug_assert! still checks existence of symbols. + #[cfg(safeguards_at_least = "strict")] // debug_assert! still checks existence of symbols. assert!( self.is_initializing(), "Base::to_init_gd() can only be called during object initialization, inside I*::init() or Gd::from_init_fn()" @@ -250,7 +250,7 @@ impl Base { /// Finalizes the initialization of this `Base` and returns whether pub(crate) fn mark_initialized(&mut self) { - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] { assert_eq!( self.init_state.get(), @@ -267,7 +267,7 @@ impl Base { /// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed. #[doc(hidden)] pub fn __fully_constructed_gd(&self) -> Gd { - #[cfg(checks_at_least = "paranoid")] // debug_assert! still checks existence of symbols. + #[cfg(safeguards_at_least = "strict")] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" @@ -298,7 +298,7 @@ impl Base { /// Returns a passive reference to the base object, for use in script contexts only. pub(crate) fn to_script_passive(&self) -> PassiveGd { - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] assert_eq!( self.init_state.get(), InitState::Script, @@ -310,7 +310,7 @@ impl Base { } /// Returns `true` if this `Base` is currently in the initializing state. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] fn is_initializing(&self) -> bool { self.init_state.get() == InitState::ObjectConstructing } @@ -318,7 +318,7 @@ impl Base { /// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed. #[doc(hidden)] pub fn __constructed_gd(&self) -> Gd { - #[cfg(checks_at_least = "paranoid")] // debug_assert! still checks existence of symbols. + #[cfg(safeguards_at_least = "strict")] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" @@ -335,7 +335,7 @@ impl Base { /// # Safety /// Caller must ensure that the underlying object remains valid for the entire lifetime of the returned `PassiveGd`. pub(crate) unsafe fn constructed_passive(&self) -> PassiveGd { - #[cfg(checks_at_least = "paranoid")] // debug_assert! still checks existence of symbols. + #[cfg(safeguards_at_least = "strict")] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" diff --git a/godot-core/src/obj/casts.rs b/godot-core/src/obj/casts.rs index 820c6c475..f748027e7 100644 --- a/godot-core/src/obj/casts.rs +++ b/godot-core/src/obj/casts.rs @@ -48,7 +48,7 @@ impl CastSuccess { } /// Access shared reference to destination, without consuming object. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] pub fn as_dest_ref(&self) -> &RawGd { self.check_validity(); &self.dest @@ -56,7 +56,7 @@ impl CastSuccess { /// Access exclusive reference to destination, without consuming object. pub fn as_dest_mut(&mut self) -> &mut RawGd { - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] self.check_validity(); &mut self.dest } @@ -71,14 +71,14 @@ impl CastSuccess { self.dest.instance_id_unchecked(), "traded_source must point to the same object as the destination" ); - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] self.check_validity(); std::mem::forget(traded_source); ManuallyDrop::into_inner(self.dest) } - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] fn check_validity(&self) { assert!(self.dest.is_null() || self.dest.is_instance_valid()); } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 86714e796..5ede7b6ba 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -784,7 +784,7 @@ where // If ref_counted returned None, that means the instance was destroyed if ref_counted != Some(false) - || (cfg!(checks_at_least = "balanced") && !self.is_instance_valid()) + || (cfg!(safeguards_at_least = "balanced") && !self.is_instance_valid()) { return error_or_panic("called free() on already destroyed object".to_string()); } @@ -793,7 +793,7 @@ where // static type information to be correct. This is a no-op in Release mode. // Skip check during panic unwind; would need to rewrite whole thing to use Result instead. Having BOTH panic-in-panic and bad type is // a very unlikely corner case. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] if !is_panic_unwind { self.raw.check_dynamic_type(&CallContext::gd::("free")); } diff --git a/godot-core/src/obj/raw_gd.rs b/godot-core/src/obj/raw_gd.rs index 743a066f8..61dddc637 100644 --- a/godot-core/src/obj/raw_gd.rs +++ b/godot-core/src/obj/raw_gd.rs @@ -360,7 +360,7 @@ impl RawGd { debug_assert!(!self.is_null(), "cannot upcast null object refs"); // In Debug builds, go the long path via Godot FFI to verify the results are the same. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] { // SAFETY: we forget the object below and do not leave the function before. let ffi_dest = self.ffi_cast::().expect("failed FFI upcast"); @@ -383,10 +383,10 @@ impl RawGd { /// Verify that the object is non-null and alive. In paranoid mode, additionally verify that it is of type `T` or derived. pub(crate) fn check_rtti(&self, method_name: &'static str) { - #[cfg(checks_at_least = "balanced")] + #[cfg(safeguards_at_least = "balanced")] { let call_ctx = CallContext::gd::(method_name); - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] self.check_dynamic_type(&call_ctx); let instance_id = unsafe { self.instance_id_unchecked().unwrap_unchecked() }; @@ -395,7 +395,7 @@ impl RawGd { } /// Checks only type, not alive-ness. Used in Gd in case of `free()`. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] pub(crate) fn check_dynamic_type(&self, call_ctx: &CallContext<'static>) { debug_assert!( !self.is_null(), @@ -515,7 +515,7 @@ where let ptr: sys::GDExtensionClassInstancePtr = binding.cast(); - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] crate::classes::ensure_binding_not_null::(ptr); self.cached_storage_ptr.set(ptr); diff --git a/godot-core/src/obj/rtti.rs b/godot-core/src/obj/rtti.rs index c26d9e228..e97e27182 100644 --- a/godot-core/src/obj/rtti.rs +++ b/godot-core/src/obj/rtti.rs @@ -21,7 +21,7 @@ pub struct ObjectRtti { instance_id: InstanceId, /// Only in Debug mode: dynamic class. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] class_name: crate::meta::ClassId, // // TODO(bromeon): class_id is not always most-derived class; ObjectRtti is sometimes constructed from a base class, via RawGd::from_obj_sys_weak(). @@ -36,7 +36,7 @@ impl ObjectRtti { Self { instance_id, - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] class_name: T::class_id(), } } @@ -45,7 +45,7 @@ impl ObjectRtti { /// /// # Panics /// In paranoid mode, if the object is not of type `T` or derived. - #[cfg(checks_at_least = "paranoid")] + #[cfg(safeguards_at_least = "strict")] #[inline] pub fn check_type(&self) { crate::classes::ensure_object_inherits(self.class_name, T::class_id(), self.instance_id); diff --git a/godot-core/src/storage/mod.rs b/godot-core/src/storage/mod.rs index 3f454b167..a3db331e5 100644 --- a/godot-core/src/storage/mod.rs +++ b/godot-core/src/storage/mod.rs @@ -123,14 +123,14 @@ mod log_inactive { // ---------------------------------------------------------------------------------------------------------------------------------------------- // Tracking borrows in Debug mode -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] use borrow_info::DebugBorrowTracker; -#[cfg(not(checks_at_least = "paranoid"))] +#[cfg(not(safeguards_at_least = "strict"))] use borrow_info_noop::DebugBorrowTracker; use crate::obj::{Base, GodotClass}; -#[cfg(checks_at_least = "paranoid")] +#[cfg(safeguards_at_least = "strict")] mod borrow_info { use std::backtrace::Backtrace; use std::fmt; @@ -195,7 +195,7 @@ mod borrow_info { } } -#[cfg(not(checks_at_least = "paranoid"))] +#[cfg(not(safeguards_at_least = "strict"))] mod borrow_info_noop { use std::fmt; diff --git a/godot-ffi/Cargo.toml b/godot-ffi/Cargo.toml index b612b81aa..641f36240 100644 --- a/godot-ffi/Cargo.toml +++ b/godot-ffi/Cargo.toml @@ -30,9 +30,9 @@ api-4-4 = ["godot-bindings/api-4-4"] api-4-5 = ["godot-bindings/api-4-5"] # ]] - -debug-checks-balanced = ["godot-bindings/debug-checks-balanced"] -release-checks-fast-unsafe = ["godot-bindings/release-checks-fast-unsafe"] +# Safeguard levels (see godot/lib.rs for detailed documentation). +safeguards-dev-balanced = ["godot-bindings/safeguards-dev-balanced"] +safeguards-release-disengaged = ["godot-bindings/safeguards-release-disengaged"] [dependencies] diff --git a/godot-ffi/build.rs b/godot-ffi/build.rs index 8b327ee8a..7dfed4d3c 100644 --- a/godot-ffi/build.rs +++ b/godot-ffi/build.rs @@ -29,5 +29,5 @@ fn main() { godot_bindings::emit_godot_version_cfg(); godot_bindings::emit_wasm_nothreads_cfg(); - godot_bindings::emit_checks_mode(); + godot_bindings::emit_safeguard_levels(); } diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 47a4a48bc..d9edd930e 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -254,13 +254,13 @@ pub unsafe fn deinitialize() { } } -fn safety_checks_string() -> &'static str { - if cfg!(checks_at_least = "paranoid") { - "paranoid" - } else if cfg!(checks_at_least = "balanced") { +fn safeguards_level_string() -> &'static str { + if cfg!(safeguards_at_least = "strict") { + "strict" + } else if cfg!(safeguards_at_least = "balanced") { "balanced" - } else if cfg!(checks_at_least = "fast-unsafe") { - "fast-unsafe" + } else if cfg!(safeguards_at_least = "disengaged") { + "disengaged" } else { unreachable!(); } @@ -269,8 +269,8 @@ fn safety_checks_string() -> &'static str { fn print_preamble(version: GDExtensionGodotVersion) { let api_version: &'static str = GdextBuild::godot_static_version_string(); let runtime_version = read_version_string(&version); - let checks_mode = safety_checks_string(); - println!("Initialize godot-rust (API {api_version}, runtime {runtime_version}, safety checks {checks_mode})"); + let safeguards_level = safeguards_level_string(); + println!("Initialize godot-rust (API {api_version}, runtime {runtime_version}, safeguards {safeguards_level})"); } /// # Safety diff --git a/godot/Cargo.toml b/godot/Cargo.toml index 72d897d2d..ddd1026d4 100644 --- a/godot/Cargo.toml +++ b/godot/Cargo.toml @@ -39,9 +39,9 @@ api-4-4 = ["godot-core/api-4-4"] api-4-5 = ["godot-core/api-4-5"] # ]] - -debug-checks-balanced = ["godot-core/debug-checks-balanced"] -release-checks-fast-unsafe = ["godot-core/release-checks-fast-unsafe"] +# Safeguard levels (see godot/lib.rs for detailed documentation). +safeguards-dev-balanced = ["godot-core/safeguards-dev-balanced"] +safeguards-release-disengaged = ["godot-core/safeguards-release-disengaged"] default = ["__codegen-full"] diff --git a/godot/src/lib.rs b/godot/src/lib.rs index ab149d85c..dcc16a133 100644 --- a/godot/src/lib.rs +++ b/godot/src/lib.rs @@ -58,6 +58,47 @@ //!

//! //! +//! ## Safeguard levels +//! +//! godot-rust uses three tiers that differ in the amount of runtime checks and validations that are performed. \ +//! They can be configured via [Cargo features](#cargo-features). +//! +//! - 🛡️ **Strict** (default for dev builds) +//! +//! Lots of additional, sometimes expensive checks. Detects many bugs during development. +//! - `Gd::bind/bind_mut()` provides extensive diagnostics to locate runtime borrow errors. +//! - `Array` safe conversion checks (for types like `Array`). +//! - RTTI checks on object access (protect against type mismatch edge cases). +//! - Geometric invariants (e.g. normalized quaternions). +//! - Access to engine APIs outside valid scope.

+//! +//! - ⚖️ **Balanced** (default for release builds) +//! +//! Basic validity and invariant checks, reasonably fast. Within this level, you should not be able to encounter undefined behavior (UB) +//! in safe Rust code. Invariant violations may however cause panics and logic errors. +//! - Object liveness checks. +//! - `Gd::bind/bind_mut()` cause panics on borrow errors.

+//! +//! - ☣️ **Disengaged** +//! +//! Most checks disabled, sacrifices safety for raw speed. This renders a large part of the godot-rust API `unsafe` without polluting the +//! code; you opt in via `unsafe impl ExtensionLibrary`. +//! +//! Before using this, measure to ensure you truly need the last bit of performance (balanced should be fast enough for most cases; if not, +//! consider bringing it up). Also test your code thoroughly using the other levels first. Undefined behavior and crashes arising +//! from using this level are your full responsibility. When reporting a bug, make sure you can reproduce it under the balanced level. +//! - Unchecked object access -> instant UB if an object is dead. +//! - `Gd::bind/bind_mut()` are unchecked -> UB if mutable aliasing occurs. +//! +//!
+//!

Safeguards are a recent addition to godot-rust and need calibrating over time. If you are unhappy with how the balanced level +//! performs in basic operations, consider bringing it up for discussion. We'd like to offer the disengaged level for power users who +//! really need it, but it shouldn't be the only choice for decent runtime performance, as it comes with heavy trade-offs.

+//! +//!

As of v0.4, the above checks are not fully implemented yet. Neither are they guarantees; categorization may change over time.

+//!
+//! +//! //! ## Cargo features //! //! The following features can be enabled for this crate. All of them are off by default. @@ -114,9 +155,9 @@ //! //! By default, Wasm threads are enabled and require the flag `"-C", "link-args=-pthread"` in the `wasm32-unknown-unknown` target. //! This must be kept in sync with Godot's Web export settings (threading support enabled). To disable it, use **additionally* the feature -//! `experimental-wasm-nothreads`.

+//! `experimental-wasm-nothreads`. //! -//! It is recommended to use this feature in combination with `lazy-function-tables` to reduce the size of the generated Wasm binary. +//! It is recommended to use this feature in combination with `lazy-function-tables` to reduce the size of the generated Wasm binary.

//! //! * **`experimental-wasm-nothreads`** //! @@ -136,7 +177,19 @@ //! This feature requires at least Godot 4.3. //! See also: [`#[derive(GodotClass)]`](register/derive.GodotClass.html#documentation) //! -//! _Integrations:_ +//! _Safeguards:_ +//! +//! See [Safeguard levels](#safeguard-levels). Levels can only be downgraded by 1 at the moment. +//! +//! * **`safeguards-dev-balanced`** +//! +//! For the `dev` Cargo profile, use the **balanced** safeguard level instead of the default strict level.

+//! +//! * **`safeguards-release-disengaged`** +//! +//! For the `release` Cargo profile, use the **disengaged** safeguard level instead of the default balanced level. +//! +//! _Third-party integrations:_ //! //! * **`serde`** //! From bde2c1ab4814edd728b1e594010fb23aa061d96d Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 25 Oct 2025 19:17:10 +0200 Subject: [PATCH 3/7] Make #[cfg] model for safeguards easier to understand Use simple #[cfg]s: `safeguards_strict` + `safeguards_balanced`. This implies "at least this safety level". No more indirect `*_at_least = "string"` conditions and extra negation. --- godot-bindings/src/lib.rs | 16 ++++++--- godot-codegen/src/generator/classes.rs | 2 +- .../src/generator/native_structures.rs | 2 +- godot-core/src/builtin/collections/array.rs | 6 ++-- godot-core/src/classes/class_runtime.rs | 12 +++---- godot-core/src/init/mod.rs | 8 ++--- godot-core/src/meta/error/convert_error.rs | 4 +-- godot-core/src/meta/signature.rs | 4 +-- godot-core/src/obj/base.rs | 35 ++++++++++--------- godot-core/src/obj/casts.rs | 8 ++--- godot-core/src/obj/gd.rs | 13 ++++--- godot-core/src/obj/raw_gd.rs | 10 +++--- godot-core/src/obj/rtti.rs | 6 ++-- godot-core/src/storage/mod.rs | 8 ++--- godot-ffi/src/lib.rs | 8 ++--- godot/src/lib.rs | 2 +- 16 files changed, 73 insertions(+), 71 deletions(-) diff --git a/godot-bindings/src/lib.rs b/godot-bindings/src/lib.rs index ce7642c8a..464fd3983 100644 --- a/godot-bindings/src/lib.rs +++ b/godot-bindings/src/lib.rs @@ -269,8 +269,10 @@ pub fn since_api(major_minor: &str) -> bool { } pub fn emit_safeguard_levels() { - let safeguard_modes = ["disengaged", "balanced", "strict"]; + // Levels: disengaged (0), balanced (1), strict (2) let mut safeguards_level = if cfg!(debug_assertions) { 2 } else { 1 }; + + // Override default level with Cargo feature, in dev/release profiles. #[cfg(debug_assertions)] if cfg!(feature = "safeguards-dev-balanced") { safeguards_level = 1; @@ -280,10 +282,14 @@ pub fn emit_safeguard_levels() { safeguards_level = 0; } - for mode in safeguard_modes.iter() { - println!(r#"cargo:rustc-check-cfg=cfg(safeguards_at_least, values("{mode}"))"#); + println!(r#"cargo:rustc-check-cfg=cfg(safeguards_balanced)"#); + println!(r#"cargo:rustc-check-cfg=cfg(safeguards_strict)"#); + + // Emit #[cfg]s cumulatively: strict builds get both balanced and strict. + if safeguards_level >= 1 { + println!(r#"cargo:rustc-cfg=safeguards_balanced"#); } - for mode in safeguard_modes.iter().take(safeguards_level + 1) { - println!(r#"cargo:rustc-cfg=safeguards_at_least="{mode}""#); + if safeguards_level >= 2 { + println!(r#"cargo:rustc-cfg=safeguards_strict"#); } } diff --git a/godot-codegen/src/generator/classes.rs b/godot-codegen/src/generator/classes.rs index 9b35b05a3..8b6a031e0 100644 --- a/godot-codegen/src/generator/classes.rs +++ b/godot-codegen/src/generator/classes.rs @@ -195,7 +195,7 @@ fn make_class(class: &Class, ctx: &mut Context, view: &ApiView) -> GeneratedClas fn __checked_id(&self) -> Option { // SAFETY: only Option due to layout-compatibility with RawGd; it is always Some because stored in Gd which is non-null. let rtti = unsafe { self.rtti.as_ref().unwrap_unchecked() }; - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] rtti.check_type::(); Some(rtti.instance_id()) } diff --git a/godot-codegen/src/generator/native_structures.rs b/godot-codegen/src/generator/native_structures.rs index 3b6a7bb3b..01f7bfd50 100644 --- a/godot-codegen/src/generator/native_structures.rs +++ b/godot-codegen/src/generator/native_structures.rs @@ -219,7 +219,7 @@ fn make_native_structure_field_and_accessor( let obj = #snake_field_name.upcast(); - #[cfg(safeguards_at_least = "balanced")] + #[cfg(safeguards_balanced)] assert!(obj.is_instance_valid(), "provided node is dead"); let id = obj.instance_id().to_u64(); diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index 0cf63b47e..4079b7bdc 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -968,7 +968,7 @@ impl Array { } /// Validates that all elements in this array can be converted to integers of type `T`. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] pub(crate) fn debug_validate_int_elements(&self) -> Result<(), ConvertError> { // SAFETY: every element is internally represented as Variant. let canonical_array = unsafe { self.assume_type_ref::() }; @@ -990,7 +990,7 @@ impl Array { } // No-op in Release. Avoids O(n) conversion checks, but still panics on access. - #[cfg(not(safeguards_at_least = "strict"))] + #[cfg(not(safeguards_strict))] pub(crate) fn debug_validate_int_elements(&self) -> Result<(), ConvertError> { Ok(()) } @@ -1233,7 +1233,7 @@ impl Clone for Array { let copy = unsafe { self.clone_unchecked() }; // Double-check copy's runtime type in Debug mode. - if cfg!(safeguards_at_least = "strict") { + if cfg!(safeguards_strict) { copy.with_checked_type() .expect("copied array should have same type as original array") } else { diff --git a/godot-core/src/classes/class_runtime.rs b/godot-core/src/classes/class_runtime.rs index 318bdaa1e..7e82063f3 100644 --- a/godot-core/src/classes/class_runtime.rs +++ b/godot-core/src/classes/class_runtime.rs @@ -8,10 +8,10 @@ //! Runtime checks and inspection of Godot classes. use crate::builtin::{GString, StringName, Variant, VariantType}; -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] use crate::classes::{ClassDb, Object}; use crate::meta::CallContext; -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] use crate::meta::ClassId; use crate::obj::{bounds, Bounds, Gd, GodotClass, InstanceId, RawGd, Singleton}; use crate::sys; @@ -191,7 +191,7 @@ where Gd::::from_obj_sys(object_ptr) } -#[cfg(safeguards_at_least = "balanced")] +#[cfg(safeguards_balanced)] pub(crate) fn ensure_object_alive( instance_id: InstanceId, old_object_ptr: sys::GDExtensionObjectPtr, @@ -212,7 +212,7 @@ pub(crate) fn ensure_object_alive( ); } -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] pub(crate) fn ensure_object_inherits(derived: ClassId, base: ClassId, instance_id: InstanceId) { if derived == base || base == Object::class_id() // for Object base, anything inherits by definition @@ -227,7 +227,7 @@ pub(crate) fn ensure_object_inherits(derived: ClassId, base: ClassId, instance_i ) } -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] pub(crate) fn ensure_binding_not_null(binding: sys::GDExtensionClassInstancePtr) where T: GodotClass + Bounds, @@ -255,7 +255,7 @@ where // Implementation of this file /// Checks if `derived` inherits from `base`, using a cache for _successful_ queries. -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] fn is_derived_base_cached(derived: ClassId, base: ClassId) -> bool { use std::collections::HashSet; diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index 87e7283af..e35b65bae 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -535,14 +535,12 @@ unsafe fn ensure_godot_features_compatible() { out!("Check Godot precision setting..."); #[cfg(feature = "debug-log")] // Display safeguards level in debug log. - let safeguards_level = if cfg!(safeguards_at_least = "strict") { + let safeguards_level = if cfg!(safeguards_strict) { "strict" - } else if cfg!(safeguards_at_least = "balanced") { + } else if cfg!(safeguards_balanced) { "balanced" - } else if cfg!(safeguards_at_least = "disengaged") { - "disengaged" } else { - "unknown" + "disengaged" }; out!("Safeguards: {safeguards_level}"); diff --git a/godot-core/src/meta/error/convert_error.rs b/godot-core/src/meta/error/convert_error.rs index 53eaf74b0..ea5996d5f 100644 --- a/godot-core/src/meta/error/convert_error.rs +++ b/godot-core/src/meta/error/convert_error.rs @@ -189,7 +189,7 @@ pub(crate) enum FromGodotError { }, /// Special case of `BadArrayType` where a custom int type such as `i8` cannot hold a dynamic `i64` value. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] BadArrayTypeInt { expected_int_type: &'static str, value: i64, @@ -234,7 +234,7 @@ impl fmt::Display for FromGodotError { write!(f, "expected array of type {exp_class}, got {act_class}") } - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] Self::BadArrayTypeInt { expected_int_type, value, diff --git a/godot-core/src/meta/signature.rs b/godot-core/src/meta/signature.rs index b75717602..470add4b8 100644 --- a/godot-core/src/meta/signature.rs +++ b/godot-core/src/meta/signature.rs @@ -145,7 +145,7 @@ impl Signature { // Note: varcalls are not safe from failing, if they happen through an object pointer -> validity check necessary. // paranoid since we already check the validity in check_rtti, this is unlikely to happen. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] if let Some(instance_id) = maybe_instance_id { crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx); } @@ -307,7 +307,7 @@ impl Signature { // $crate::out!("out_class_ptrcall: {call_ctx}"); // paranoid since we already check the validity in check_rtti, this is unlikely to happen. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] if let Some(instance_id) = maybe_instance_id { crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx); } diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index f1201e316..0a359968d 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -5,13 +5,14 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] use std::cell::Cell; use std::cell::RefCell; use std::collections::hash_map::Entry; use std::collections::HashMap; use std::fmt::{Debug, Display, Formatter, Result as FmtResult}; use std::mem::ManuallyDrop; +#[cfg(safeguards_strict)] use std::rc::Rc; use crate::builtin::Callable; @@ -27,7 +28,7 @@ thread_local! { } /// Represents the initialization state of a `Base` object. -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum InitState { /// Object is being constructed (inside `I*::init()` or `Gd::from_init_fn()`). @@ -38,14 +39,14 @@ enum InitState { Script, } -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] macro_rules! base_from_obj { ($obj:expr, $state:expr) => { Base::from_obj($obj, $state) }; } -#[cfg(not(safeguards_at_least = "strict"))] +#[cfg(not(safeguards_strict))] macro_rules! base_from_obj { ($obj:expr, $state:expr) => { Base::from_obj($obj) @@ -82,7 +83,7 @@ pub struct Base { /// Tracks the initialization state of this `Base` in Debug mode. /// /// Rc allows to "copy-construct" the base from an existing one, while still affecting the user-instance through the original `Base`. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] init_state: Rc>, } @@ -95,14 +96,14 @@ impl Base { /// `base` must be alive at the time of invocation, i.e. user `init()` (which could technically destroy it) must not have run yet. /// If `base` is destroyed while the returned `Base` is in use, that constitutes a logic error, not a safety issue. pub(crate) unsafe fn from_base(base: &Base) -> Base { - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] assert!(base.obj.is_instance_valid()); let obj = Gd::from_obj_sys_weak(base.obj.obj_sys()); Self { obj: ManuallyDrop::new(obj), - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] init_state: Rc::clone(&base.init_state), } } @@ -115,7 +116,7 @@ impl Base { /// `gd` must be alive at the time of invocation. If it is destroyed while the returned `Base` is in use, that constitutes a logic /// error, not a safety issue. pub(crate) unsafe fn from_script_gd(gd: &Gd) -> Self { - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] assert!(gd.is_instance_valid()); let obj = Gd::from_obj_sys_weak(gd.obj_sys()); @@ -143,7 +144,7 @@ impl Base { base_from_obj!(obj, InitState::ObjectConstructing) } - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] fn from_obj(obj: Gd, init_state: InitState) -> Self { Self { obj: ManuallyDrop::new(obj), @@ -151,7 +152,7 @@ impl Base { } } - #[cfg(not(safeguards_at_least = "strict"))] + #[cfg(not(safeguards_strict))] fn from_obj(obj: Gd) -> Self { Self { obj: ManuallyDrop::new(obj), @@ -178,7 +179,7 @@ impl Base { /// # Panics (Debug) /// If called outside an initialization function, or for ref-counted objects on a non-main thread. pub fn to_init_gd(&self) -> Gd { - #[cfg(safeguards_at_least = "strict")] // debug_assert! still checks existence of symbols. + #[cfg(safeguards_strict)] // debug_assert! still checks existence of symbols. assert!( self.is_initializing(), "Base::to_init_gd() can only be called during object initialization, inside I*::init() or Gd::from_init_fn()" @@ -250,7 +251,7 @@ impl Base { /// Finalizes the initialization of this `Base` and returns whether pub(crate) fn mark_initialized(&mut self) { - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] { assert_eq!( self.init_state.get(), @@ -267,7 +268,7 @@ impl Base { /// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed. #[doc(hidden)] pub fn __fully_constructed_gd(&self) -> Gd { - #[cfg(safeguards_at_least = "strict")] // debug_assert! still checks existence of symbols. + #[cfg(safeguards_strict)] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" @@ -298,7 +299,7 @@ impl Base { /// Returns a passive reference to the base object, for use in script contexts only. pub(crate) fn to_script_passive(&self) -> PassiveGd { - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] assert_eq!( self.init_state.get(), InitState::Script, @@ -310,7 +311,7 @@ impl Base { } /// Returns `true` if this `Base` is currently in the initializing state. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] fn is_initializing(&self) -> bool { self.init_state.get() == InitState::ObjectConstructing } @@ -318,7 +319,7 @@ impl Base { /// Returns a [`Gd`] referencing the base object, assuming the derived object is fully constructed. #[doc(hidden)] pub fn __constructed_gd(&self) -> Gd { - #[cfg(safeguards_at_least = "strict")] // debug_assert! still checks existence of symbols. + #[cfg(safeguards_strict)] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::to_gd(), base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" @@ -335,7 +336,7 @@ impl Base { /// # Safety /// Caller must ensure that the underlying object remains valid for the entire lifetime of the returned `PassiveGd`. pub(crate) unsafe fn constructed_passive(&self) -> PassiveGd { - #[cfg(safeguards_at_least = "strict")] // debug_assert! still checks existence of symbols. + #[cfg(safeguards_strict)] // debug_assert! still checks existence of symbols. assert!( !self.is_initializing(), "WithBaseField::base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()" diff --git a/godot-core/src/obj/casts.rs b/godot-core/src/obj/casts.rs index f748027e7..9571963e2 100644 --- a/godot-core/src/obj/casts.rs +++ b/godot-core/src/obj/casts.rs @@ -48,7 +48,7 @@ impl CastSuccess { } /// Access shared reference to destination, without consuming object. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] pub fn as_dest_ref(&self) -> &RawGd { self.check_validity(); &self.dest @@ -56,7 +56,7 @@ impl CastSuccess { /// Access exclusive reference to destination, without consuming object. pub fn as_dest_mut(&mut self) -> &mut RawGd { - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] self.check_validity(); &mut self.dest } @@ -71,14 +71,14 @@ impl CastSuccess { self.dest.instance_id_unchecked(), "traded_source must point to the same object as the destination" ); - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] self.check_validity(); std::mem::forget(traded_source); ManuallyDrop::into_inner(self.dest) } - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] fn check_validity(&self) { assert!(self.dest.is_null() || self.dest.is_instance_valid()); } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 5ede7b6ba..18af4b086 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -15,8 +15,8 @@ use sys::{static_assert_eq_size_align, SysPtr as _}; use crate::builtin::{Callable, GString, NodePath, StringName, Variant}; use crate::meta::error::{ConvertError, FromFfiError}; use crate::meta::{ - ArrayElement, AsArg, CallContext, ClassId, FromGodot, GodotConvert, GodotType, - PropertyHintInfo, RefArg, ToGodot, + ArrayElement, AsArg, ClassId, FromGodot, GodotConvert, GodotType, PropertyHintInfo, RefArg, + ToGodot, }; use crate::obj::{ bounds, cap, Bounds, DynGd, GdDerefTarget, GdMut, GdRef, GodotClass, Inherits, InstanceId, @@ -783,9 +783,7 @@ where } // If ref_counted returned None, that means the instance was destroyed - if ref_counted != Some(false) - || (cfg!(safeguards_at_least = "balanced") && !self.is_instance_valid()) - { + if ref_counted != Some(false) || (cfg!(safeguards_balanced) && !self.is_instance_valid()) { return error_or_panic("called free() on already destroyed object".to_string()); } @@ -793,9 +791,10 @@ where // static type information to be correct. This is a no-op in Release mode. // Skip check during panic unwind; would need to rewrite whole thing to use Result instead. Having BOTH panic-in-panic and bad type is // a very unlikely corner case. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] if !is_panic_unwind { - self.raw.check_dynamic_type(&CallContext::gd::("free")); + self.raw + .check_dynamic_type(&crate::meta::CallContext::gd::("free")); } // SAFETY: object must be alive, which was just checked above. No multithreading here. diff --git a/godot-core/src/obj/raw_gd.rs b/godot-core/src/obj/raw_gd.rs index 61dddc637..e8d7c23d6 100644 --- a/godot-core/src/obj/raw_gd.rs +++ b/godot-core/src/obj/raw_gd.rs @@ -360,7 +360,7 @@ impl RawGd { debug_assert!(!self.is_null(), "cannot upcast null object refs"); // In Debug builds, go the long path via Godot FFI to verify the results are the same. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] { // SAFETY: we forget the object below and do not leave the function before. let ffi_dest = self.ffi_cast::().expect("failed FFI upcast"); @@ -383,10 +383,10 @@ impl RawGd { /// Verify that the object is non-null and alive. In paranoid mode, additionally verify that it is of type `T` or derived. pub(crate) fn check_rtti(&self, method_name: &'static str) { - #[cfg(safeguards_at_least = "balanced")] + #[cfg(safeguards_balanced)] { let call_ctx = CallContext::gd::(method_name); - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] self.check_dynamic_type(&call_ctx); let instance_id = unsafe { self.instance_id_unchecked().unwrap_unchecked() }; @@ -395,7 +395,7 @@ impl RawGd { } /// Checks only type, not alive-ness. Used in Gd in case of `free()`. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] pub(crate) fn check_dynamic_type(&self, call_ctx: &CallContext<'static>) { debug_assert!( !self.is_null(), @@ -515,7 +515,7 @@ where let ptr: sys::GDExtensionClassInstancePtr = binding.cast(); - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] crate::classes::ensure_binding_not_null::(ptr); self.cached_storage_ptr.set(ptr); diff --git a/godot-core/src/obj/rtti.rs b/godot-core/src/obj/rtti.rs index e97e27182..bf21a3c47 100644 --- a/godot-core/src/obj/rtti.rs +++ b/godot-core/src/obj/rtti.rs @@ -21,7 +21,7 @@ pub struct ObjectRtti { instance_id: InstanceId, /// Only in Debug mode: dynamic class. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] class_name: crate::meta::ClassId, // // TODO(bromeon): class_id is not always most-derived class; ObjectRtti is sometimes constructed from a base class, via RawGd::from_obj_sys_weak(). @@ -36,7 +36,7 @@ impl ObjectRtti { Self { instance_id, - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] class_name: T::class_id(), } } @@ -45,7 +45,7 @@ impl ObjectRtti { /// /// # Panics /// In paranoid mode, if the object is not of type `T` or derived. - #[cfg(safeguards_at_least = "strict")] + #[cfg(safeguards_strict)] #[inline] pub fn check_type(&self) { crate::classes::ensure_object_inherits(self.class_name, T::class_id(), self.instance_id); diff --git a/godot-core/src/storage/mod.rs b/godot-core/src/storage/mod.rs index a3db331e5..552fd4df1 100644 --- a/godot-core/src/storage/mod.rs +++ b/godot-core/src/storage/mod.rs @@ -123,14 +123,14 @@ mod log_inactive { // ---------------------------------------------------------------------------------------------------------------------------------------------- // Tracking borrows in Debug mode -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] use borrow_info::DebugBorrowTracker; -#[cfg(not(safeguards_at_least = "strict"))] +#[cfg(not(safeguards_strict))] use borrow_info_noop::DebugBorrowTracker; use crate::obj::{Base, GodotClass}; -#[cfg(safeguards_at_least = "strict")] +#[cfg(safeguards_strict)] mod borrow_info { use std::backtrace::Backtrace; use std::fmt; @@ -195,7 +195,7 @@ mod borrow_info { } } -#[cfg(not(safeguards_at_least = "strict"))] +#[cfg(not(safeguards_strict))] mod borrow_info_noop { use std::fmt; diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index d9edd930e..2dd2a5c37 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -255,14 +255,12 @@ pub unsafe fn deinitialize() { } fn safeguards_level_string() -> &'static str { - if cfg!(safeguards_at_least = "strict") { + if cfg!(safeguards_strict) { "strict" - } else if cfg!(safeguards_at_least = "balanced") { + } else if cfg!(safeguards_balanced) { "balanced" - } else if cfg!(safeguards_at_least = "disengaged") { - "disengaged" } else { - unreachable!(); + "disengaged" } } diff --git a/godot/src/lib.rs b/godot/src/lib.rs index dcc16a133..d6c75b5e7 100644 --- a/godot/src/lib.rs +++ b/godot/src/lib.rs @@ -179,7 +179,7 @@ //! //! _Safeguards:_ //! -//! See [Safeguard levels](#safeguard-levels). Levels can only be downgraded by 1 at the moment. +//! See [Safeguard levels](#safeguard-levels). //! //! * **`safeguards-dev-balanced`** //! From 010b5a5442cfcd72d73981d28386752d978837e3 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 25 Oct 2025 22:51:22 +0200 Subject: [PATCH 4/7] Update itests to conform with new safeguard levels --- itest/rust/build.rs | 1 + .../builtin_tests/containers/variant_test.rs | 14 ++++----- itest/rust/src/framework/mod.rs | 8 +++++ itest/rust/src/object_tests/base_init_test.rs | 14 +++++---- itest/rust/src/object_tests/base_test.rs | 16 +++++----- .../rust/src/object_tests/object_swap_test.rs | 4 +-- itest/rust/src/object_tests/object_test.rs | 30 +++++++++---------- 7 files changed, 50 insertions(+), 37 deletions(-) diff --git a/itest/rust/build.rs b/itest/rust/build.rs index fd16833bd..6ca87b303 100644 --- a/itest/rust/build.rs +++ b/itest/rust/build.rs @@ -272,6 +272,7 @@ fn main() { rustfmt_if_needed(vec![rust_file]); godot_bindings::emit_godot_version_cfg(); + godot_bindings::emit_safeguard_levels(); // The godot crate has a __codegen-full default feature that enables the godot-codegen/codegen-full feature. When compiling the entire // workspace itest also gets compiled with full codegen due to feature unification. This causes compiler errors since the diff --git a/itest/rust/src/builtin_tests/containers/variant_test.rs b/itest/rust/src/builtin_tests/containers/variant_test.rs index 4eadfbb32..18e620157 100644 --- a/itest/rust/src/builtin_tests/containers/variant_test.rs +++ b/itest/rust/src/builtin_tests/containers/variant_test.rs @@ -21,7 +21,7 @@ use godot::obj::{Gd, InstanceId, NewAlloc, NewGd}; use godot::sys::GodotFfi; use crate::common::roundtrip; -use crate::framework::{expect_panic, itest, runs_release}; +use crate::framework::{expect_panic, expect_panic_or_ub, itest, runs_release}; const TEST_BASIS: Basis = Basis::from_rows( Vector3::new(1.0, 2.0, 3.0), @@ -272,7 +272,7 @@ fn variant_dead_object_conversions() { ); // Variant::to(). - expect_panic("Variant::to() with dead object should panic", || { + expect_panic_or_ub("Variant::to() with dead object should panic", || { let _: Gd = variant.to(); }); @@ -361,15 +361,15 @@ fn variant_array_from_untyped_conversions() { ) } -// Same builtin type INT, but incompatible integers (Debug-only). +// Same builtin type INT, but incompatible integers (strict-safeguards only). #[itest] fn variant_array_bad_integer_conversions() { let i32_array: Array = array![1, 2, 160, -40]; let i32_variant = i32_array.to_variant(); let i8_back = i32_variant.try_to::>(); - // In Debug mode, we expect an error upon conversion. - #[cfg(debug_assertions)] + // In strict safeguard mode, we expect an error upon conversion. + #[cfg(safeguards_strict)] { let err = i8_back.expect_err("Array -> Array conversion should fail"); assert_eq!( @@ -378,8 +378,8 @@ fn variant_array_bad_integer_conversions() { ) } - // In Release mode, we expect the conversion to succeed, but a panic to occur on element access. - #[cfg(not(debug_assertions))] + // In balanced/disengaged modes, we expect the conversion to succeed, but a panic to occur on element access. + #[cfg(not(safeguards_strict))] { let i8_array = i8_back.expect("Array -> Array conversion should succeed"); expect_panic("accessing element 160 as i8 should panic", || { diff --git a/itest/rust/src/framework/mod.rs b/itest/rust/src/framework/mod.rs index e5ced4764..4fb7a4b3a 100644 --- a/itest/rust/src/framework/mod.rs +++ b/itest/rust/src/framework/mod.rs @@ -200,6 +200,14 @@ pub fn expect_panic(context: &str, code: impl FnOnce()) { ); } +/// Run for code that panics in *strict* and *balanced* safeguard levels, but cause UB in *disengaged* level. +/// +/// The code is not executed for the latter. +pub fn expect_panic_or_ub(_context: &str, _code: impl FnOnce()) { + #[cfg(safeguards_balanced)] + expect_panic(_context, _code); +} + pin_project_lite::pin_project! { pub struct ExpectPanicFuture { context: &'static str, diff --git a/itest/rust/src/object_tests/base_init_test.rs b/itest/rust/src/object_tests/base_init_test.rs index d78e58ade..43a7d9497 100644 --- a/itest/rust/src/object_tests/base_init_test.rs +++ b/itest/rust/src/object_tests/base_init_test.rs @@ -10,7 +10,7 @@ use godot::builtin::Vector2; use godot::classes::notify::ObjectNotification; use godot::classes::{ClassDb, IRefCounted, RefCounted}; use godot::meta::ToGodot; -use godot::obj::{Base, Gd, InstanceId, NewAlloc, NewGd, Singleton, WithBaseField}; +use godot::obj::{Base, Gd, InstanceId, NewGd, Singleton, WithBaseField}; use godot::register::{godot_api, GodotClass}; use godot::task::TaskHandle; @@ -65,13 +65,14 @@ fn base_init_extracted_gd() { // Checks bad practice of rug-pulling the base pointer. #[itest] +#[cfg(safeguards_balanced)] fn base_init_freed_gd() { let mut free_executed = false; expect_panic("base object is destroyed", || { let _obj = Gd::::from_init_fn(|base| { let obj = base.to_init_gd(); - obj.free(); // Causes the problem, but doesn't panic yet. + obj.free(); // Causes the problem, but doesn't panic yet. UB in safeguards-disengaged. free_executed = true; Based { base, i: 456 } @@ -144,27 +145,28 @@ fn verify_complex_init((obj, base): (Gd, Gd)) -> Instance id } -#[cfg(debug_assertions)] +#[cfg(safeguards_strict)] #[itest] fn base_init_outside_init() { + use godot::obj::NewAlloc; let mut obj = Based::new_alloc(); expect_panic("to_init_gd() outside init() function", || { let guard = obj.bind_mut(); - let _gd = guard.base.to_init_gd(); // Panics in Debug builds. + let _gd = guard.base.to_init_gd(); // Panics in strict safeguard mode. }); obj.free(); } -#[cfg(debug_assertions)] +#[cfg(safeguards_strict)] #[itest] fn base_init_to_gd() { expect_panic("WithBaseField::to_gd() inside init() function", || { let _obj = Gd::::from_init_fn(|base| { let temp_obj = Based { base, i: 999 }; - // Call to self.to_gd() during initialization should panic in Debug builds. + // Call to self.to_gd() during initialization should panic in strict safeguard mode. let _gd = godot::obj::WithBaseField::to_gd(&temp_obj); temp_obj diff --git a/itest/rust/src/object_tests/base_test.rs b/itest/rust/src/object_tests/base_test.rs index 7278cc32e..52ca2b2bc 100644 --- a/itest/rust/src/object_tests/base_test.rs +++ b/itest/rust/src/object_tests/base_test.rs @@ -7,7 +7,7 @@ use godot::prelude::*; -use crate::framework::{expect_panic, itest}; +use crate::framework::{expect_panic_or_ub, itest}; #[itest(skip)] fn base_test_is_weak() { @@ -82,6 +82,8 @@ fn base_gd_self() { } // Hardening against https://github.com/godot-rust/gdext/issues/711. +// Furthermore, this now keeps expect_panic_or_ub() instead of expect_panic(), although it's questionable if much remains with all these checks +// disabled. There is still some logic remaining, and an alternative would be to #[cfg(safeguards_balanced)] this. #[itest] fn base_smuggling() { let (mut obj, extracted_base) = create_object_with_extracted_base(); @@ -98,19 +100,19 @@ fn base_smuggling() { extracted_base_obj.free(); // Access to object should now fail. - expect_panic("object with dead base: calling base methods", || { + expect_panic_or_ub("object with dead base: calling base methods", || { obj.get_position(); }); - expect_panic("object with dead base: bind()", || { + expect_panic_or_ub("object with dead base: bind()", || { obj.bind(); }); - expect_panic("object with dead base: instance_id()", || { + expect_panic_or_ub("object with dead base: instance_id()", || { obj.instance_id(); }); - expect_panic("object with dead base: clone()", || { + expect_panic_or_ub("object with dead base: clone()", || { let _ = obj.clone(); }); - expect_panic("object with dead base: upcast()", || { + expect_panic_or_ub("object with dead base: upcast()", || { obj.upcast::(); }); @@ -118,7 +120,7 @@ fn base_smuggling() { let (obj, extracted_base) = create_object_with_extracted_base(); obj.free(); - expect_panic("accessing extracted base of dead object", || { + expect_panic_or_ub("accessing extracted base of dead object", || { extracted_base.__constructed_gd().get_position(); }); } diff --git a/itest/rust/src/object_tests/object_swap_test.rs b/itest/rust/src/object_tests/object_swap_test.rs index b1cea92ea..d636bd01e 100644 --- a/itest/rust/src/object_tests/object_swap_test.rs +++ b/itest/rust/src/object_tests/object_swap_test.rs @@ -10,8 +10,8 @@ // A lot these tests also exist in the `object_test` module, where they test object lifetime rather than type swapping. // TODO consolidate them, so that it's less likely to forget edge cases. -// Disabled in Release mode, since we don't perform the subtype check there. -#![cfg(debug_assertions)] +// Disabled in balanced/disengaged levels, since we don't perform the subtype check there. +#![cfg(safeguards_strict)] use godot::builtin::GString; use godot::classes::{Node, Node3D, Object}; diff --git a/itest/rust/src/object_tests/object_test.rs b/itest/rust/src/object_tests/object_test.rs index 6fd31ae30..fa817ea3d 100644 --- a/itest/rust/src/object_tests/object_test.rs +++ b/itest/rust/src/object_tests/object_test.rs @@ -22,7 +22,7 @@ use godot::obj::{Base, Gd, Inherits, InstanceId, NewAlloc, NewGd, RawGd, Singlet use godot::register::{godot_api, GodotClass}; use godot::sys::{self, interface_fn, GodotFfi}; -use crate::framework::{expect_panic, itest, TestContext}; +use crate::framework::{expect_panic, expect_panic_or_ub, itest, TestContext}; // TODO: // * make sure that ptrcalls are used when possible (i.e. when type info available; maybe GDScript integration test) @@ -171,7 +171,7 @@ fn object_instance_id_when_freed() { node.clone().free(); // destroys object without moving out of reference assert!(!node.is_instance_valid()); - expect_panic("instance_id() on dead object", move || { + expect_panic_or_ub("instance_id() on dead object", move || { node.instance_id(); }); } @@ -235,7 +235,7 @@ fn object_user_bind_after_free() { let copy = obj.clone(); obj.free(); - expect_panic("bind() on dead user object", move || { + expect_panic_or_ub("bind() on dead user object", move || { let _ = copy.bind(); }); } @@ -247,7 +247,7 @@ fn object_user_free_during_bind() { let copy = obj.clone(); // TODO clone allowed while bound? - expect_panic("direct free() on user while it's bound", move || { + expect_panic_or_ub("direct free() on user while it's bound", move || { copy.free(); }); @@ -275,7 +275,7 @@ fn object_engine_freed_argument_passing(ctx: &TestContext) { // Destroy object and then pass it to a Godot engine API. node.free(); - expect_panic("pass freed Gd to Godot engine API (T=Node)", || { + expect_panic_or_ub("pass freed Gd to Godot engine API (T=Node)", || { tree.add_child(&node2); }); } @@ -288,10 +288,10 @@ fn object_user_freed_casts() { // Destroy object and then pass it to a Godot engine API (upcast itself works, see other tests). obj.free(); - expect_panic("Gd::upcast() on dead object (T=user)", || { + expect_panic_or_ub("Gd::upcast() on dead object (T=user)", || { let _ = obj2.upcast::(); }); - expect_panic("Gd::cast() on dead object (T=user)", || { + expect_panic_or_ub("Gd::cast() on dead object (T=user)", || { let _ = base_obj.cast::(); }); } @@ -306,7 +306,7 @@ fn object_user_freed_argument_passing() { // Destroy object and then pass it to a Godot engine API (upcast itself works, see other tests). obj.free(); - expect_panic("pass freed Gd to Godot engine API (T=user)", || { + expect_panic_or_ub("pass freed Gd to Godot engine API (T=user)", || { engine.register_singleton("NeverRegistered", &obj2); }); } @@ -344,7 +344,7 @@ fn object_user_call_after_free() { let mut copy = obj.clone(); obj.free(); - expect_panic("call() on dead user object", move || { + expect_panic_or_ub("call() on dead user object", move || { let _ = copy.call("get_instance_id", &[]); }); } @@ -355,7 +355,7 @@ fn object_engine_use_after_free() { let copy = node.clone(); node.free(); - expect_panic("call method on dead engine object", move || { + expect_panic_or_ub("call method on dead engine object", move || { copy.get_position(); }); } @@ -366,7 +366,7 @@ fn object_engine_use_after_free_varcall() { let mut copy = node.clone(); node.free(); - expect_panic("call method on dead engine object", move || { + expect_panic_or_ub("call method on dead engine object", move || { copy.call_deferred("get_position", &[]); }); } @@ -411,13 +411,13 @@ fn object_dead_eq() { { let lhs = a.clone(); - expect_panic("Gd::eq() panics when one operand is dead", move || { + expect_panic_or_ub("Gd::eq() panics when one operand is dead", move || { let _ = lhs == b; }); } { let rhs = a.clone(); - expect_panic("Gd::ne() panics when one operand is dead", move || { + expect_panic_or_ub("Gd::ne() panics when one operand is dead", move || { let _ = b2 != rhs; }); } @@ -826,7 +826,7 @@ fn object_engine_manual_double_free() { let node2 = node.clone(); node.free(); - expect_panic("double free()", move || { + expect_panic_or_ub("double free()", move || { node2.free(); }); } @@ -845,7 +845,7 @@ fn object_user_double_free() { let obj2 = obj.clone(); obj.call("free", &[]); - expect_panic("double free()", move || { + expect_panic_or_ub("double free()", move || { obj2.free(); }); } From a19eda55c0e9493324e5b3eea02ff6c7a6b3c9ec Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 26 Oct 2025 18:39:45 +0100 Subject: [PATCH 5/7] Fix UB (check accidentally only done in strict, not balanced) --- godot-core/src/obj/base.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index 0a359968d..05cd82e47 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -96,8 +96,11 @@ impl Base { /// `base` must be alive at the time of invocation, i.e. user `init()` (which could technically destroy it) must not have run yet. /// If `base` is destroyed while the returned `Base` is in use, that constitutes a logic error, not a safety issue. pub(crate) unsafe fn from_base(base: &Base) -> Base { - #[cfg(safeguards_strict)] - assert!(base.obj.is_instance_valid()); + #[cfg(safeguards_balanced)] + assert!( + base.obj.is_instance_valid(), + "Cannot construct Base; was object freed during initialization?" + ); let obj = Gd::from_obj_sys_weak(base.obj.obj_sys()); From 9c3e8e1275222f00422e6642c9ddf18a9c32b92b Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 26 Oct 2025 16:07:32 +0100 Subject: [PATCH 6/7] Refactor and simplify object liveness/RTTI checks --- godot-codegen/src/generator/classes.rs | 25 ++++--- godot-core/src/classes/class_runtime.rs | 11 ++- godot-core/src/meta/signature.rs | 30 ++------ godot-core/src/obj/raw_gd.rs | 95 +++++++++++++++++++++---- godot-core/src/obj/rtti.rs | 12 +++- 5 files changed, 116 insertions(+), 57 deletions(-) diff --git a/godot-codegen/src/generator/classes.rs b/godot-codegen/src/generator/classes.rs index 8b6a031e0..6d9462de3 100644 --- a/godot-codegen/src/generator/classes.rs +++ b/godot-codegen/src/generator/classes.rs @@ -192,12 +192,14 @@ fn make_class(class: &Class, ctx: &mut Context, view: &ApiView) -> GeneratedClas let (assoc_memory, assoc_dyn_memory, is_exportable) = make_bounds(class, ctx); let internal_methods = quote! { - fn __checked_id(&self) -> Option { - // SAFETY: only Option due to layout-compatibility with RawGd; it is always Some because stored in Gd which is non-null. - let rtti = unsafe { self.rtti.as_ref().unwrap_unchecked() }; - #[cfg(safeguards_strict)] - rtti.check_type::(); - Some(rtti.instance_id()) + /// Creates a validated object for FFI boundary crossing. + /// + /// Low-level internal method. Validation (liveness/type checks) depend on safeguard level. + fn __validated_obj(&self) -> crate::obj::ValidatedObject { + // SAFETY: Self has the same layout as RawGd (object_ptr + rtti fields in same order). + let raw_gd = unsafe { std::mem::transmute::<&Self, &crate::obj::RawGd>(self) }; + + raw_gd.validated_object() } #[doc(hidden)] @@ -580,10 +582,10 @@ fn make_class_method_definition( let table_index = ctx.get_table_index(&MethodTableKey::from_class(class, method)); - let maybe_instance_id = if method.qualifier() == FnQualifier::Static { + let validated_obj = if method.qualifier() == FnQualifier::Static { quote! { None } } else { - quote! { self.__checked_id() } + quote! { Some(self.__validated_obj()) } }; let fptr_access = if cfg!(feature = "codegen-lazy-fptrs") { @@ -599,7 +601,6 @@ fn make_class_method_definition( quote! { fptr_by_index(#table_index) } }; - let object_ptr = &receiver.ffi_arg; let ptrcall_invocation = quote! { let method_bind = sys::#get_method_table().#fptr_access; @@ -607,8 +608,7 @@ fn make_class_method_definition( method_bind, #rust_class_name, #rust_method_name, - #object_ptr, - #maybe_instance_id, + #validated_obj, args, ) }; @@ -620,8 +620,7 @@ fn make_class_method_definition( method_bind, #rust_class_name, #rust_method_name, - #object_ptr, - #maybe_instance_id, + #validated_obj, args, varargs ) diff --git a/godot-core/src/classes/class_runtime.rs b/godot-core/src/classes/class_runtime.rs index 7e82063f3..ef7cd0df5 100644 --- a/godot-core/src/classes/class_runtime.rs +++ b/godot-core/src/classes/class_runtime.rs @@ -10,10 +10,13 @@ use crate::builtin::{GString, StringName, Variant, VariantType}; #[cfg(safeguards_strict)] use crate::classes::{ClassDb, Object}; +#[cfg(safeguards_balanced)] use crate::meta::CallContext; #[cfg(safeguards_strict)] use crate::meta::ClassId; -use crate::obj::{bounds, Bounds, Gd, GodotClass, InstanceId, RawGd, Singleton}; +#[cfg(safeguards_strict)] +use crate::obj::Singleton; +use crate::obj::{bounds, Bounds, Gd, GodotClass, InstanceId, RawGd}; use crate::sys; pub(crate) fn debug_string( @@ -191,6 +194,12 @@ where Gd::::from_obj_sys(object_ptr) } +/// Checks that the object with the given instance ID is still alive and that the pointer is valid. +/// +/// This does **not** perform type checking — use `ensure_object_type()` for that. +/// +/// # Panics (balanced+strict safeguards) +/// If the object has been freed or the instance ID points to a different object. #[cfg(safeguards_balanced)] pub(crate) fn ensure_object_alive( instance_id: InstanceId, diff --git a/godot-core/src/meta/signature.rs b/godot-core/src/meta/signature.rs index 470add4b8..4a9529deb 100644 --- a/godot-core/src/meta/signature.rs +++ b/godot-core/src/meta/signature.rs @@ -17,7 +17,7 @@ use crate::meta::{ FromGodot, GodotConvert, GodotType, InParamTuple, MethodParamOrReturnInfo, OutParamTuple, ParamTuple, ToGodot, }; -use crate::obj::{GodotClass, InstanceId}; +use crate::obj::{GodotClass, ValidatedObject}; pub(super) type CallResult = Result; @@ -62,7 +62,6 @@ where /// Receive a varcall from Godot, and return the value in `ret` as a variant pointer. /// /// # Safety - /// /// A call to this function must be caused by Godot making a varcall with parameters `Params` and return type `Ret`. #[inline] pub unsafe fn in_varcall( @@ -126,8 +125,6 @@ impl Signature { /// Make a varcall to the Godot engine for a class method. /// /// # Safety - /// - /// - `object_ptr` must be a live instance of a class with the type expected by `method_bind` /// - `method_bind` must expect explicit args `args`, varargs `varargs`, and return a value of type `Ret` #[inline] pub unsafe fn out_class_varcall( @@ -135,21 +132,13 @@ impl Signature { // Separate parameters to reduce tokens in generated class API. class_name: &'static str, method_name: &'static str, - object_ptr: sys::GDExtensionObjectPtr, - maybe_instance_id: Option, // if not static + validated_obj: Option, args: Params, varargs: &[Variant], ) -> CallResult { let call_ctx = CallContext::outbound(class_name, method_name); //$crate::out!("out_class_varcall: {call_ctx}"); - // Note: varcalls are not safe from failing, if they happen through an object pointer -> validity check necessary. - // paranoid since we already check the validity in check_rtti, this is unlikely to happen. - #[cfg(safeguards_strict)] - if let Some(instance_id) = maybe_instance_id { - crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx); - } - let class_fn = sys::interface_fn!(object_method_bind_call); let variant = args.with_variants(|explicit_args| { @@ -162,7 +151,7 @@ impl Signature { let mut err = sys::default_call_error(); class_fn( method_bind.0, - object_ptr, + ValidatedObject::object_ptr(validated_obj.as_ref()), variant_ptrs.as_ptr(), variant_ptrs.len() as i64, return_ptr, @@ -290,8 +279,6 @@ impl Signature { /// Make a ptrcall to the Godot engine for a class method. /// /// # Safety - /// - /// - `object_ptr` must be a live instance of a class with the type expected by `method_bind` /// - `method_bind` must expect explicit args `args`, and return a value of type `Ret` #[inline] pub unsafe fn out_class_ptrcall( @@ -299,26 +286,19 @@ impl Signature { // Separate parameters to reduce tokens in generated class API. class_name: &'static str, method_name: &'static str, - object_ptr: sys::GDExtensionObjectPtr, - maybe_instance_id: Option, // if not static + validated_obj: Option, args: Params, ) -> Ret { let call_ctx = CallContext::outbound(class_name, method_name); // $crate::out!("out_class_ptrcall: {call_ctx}"); - // paranoid since we already check the validity in check_rtti, this is unlikely to happen. - #[cfg(safeguards_strict)] - if let Some(instance_id) = maybe_instance_id { - crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx); - } - let class_fn = sys::interface_fn!(object_method_bind_ptrcall); unsafe { Self::raw_ptrcall(args, &call_ctx, |explicit_args, return_ptr| { class_fn( method_bind.0, - object_ptr, + ValidatedObject::object_ptr(validated_obj.as_ref()), explicit_args.as_ptr(), return_ptr, ); diff --git a/godot-core/src/obj/raw_gd.rs b/godot-core/src/obj/raw_gd.rs index e8d7c23d6..e260064f3 100644 --- a/godot-core/src/obj/raw_gd.rs +++ b/godot-core/src/obj/raw_gd.rs @@ -381,34 +381,61 @@ impl RawGd { } } - /// Verify that the object is non-null and alive. In paranoid mode, additionally verify that it is of type `T` or derived. + /// Validates object for use in `RawGd`/`Gd` methods (not FFI boundary calls). + /// + /// This is used for Rust-side object operations like `bind()`, `clone()`, `ffi_cast()`, etc. + /// For FFI boundary calls (generated engine methods), validation happens in signature.rs instead. + /// + /// # Panics + /// If validation fails. + #[cfg(safeguards_balanced)] pub(crate) fn check_rtti(&self, method_name: &'static str) { - #[cfg(safeguards_balanced)] - { - let call_ctx = CallContext::gd::(method_name); - #[cfg(safeguards_strict)] - self.check_dynamic_type(&call_ctx); - let instance_id = unsafe { self.instance_id_unchecked().unwrap_unchecked() }; + let call_ctx = CallContext::gd::(method_name); - classes::ensure_object_alive(instance_id, self.obj_sys(), &call_ctx); - } + // Type check (strict only). + #[cfg(safeguards_strict)] + self.check_dynamic_type(&call_ctx); + + // SAFETY: check_rtti() is only called for non-null pointers. + let instance_id = unsafe { self.instance_id_unchecked().unwrap_unchecked() }; + + // Liveness check (balanced + strict). + classes::ensure_object_alive(instance_id, self.obj_sys(), &call_ctx); } - /// Checks only type, not alive-ness. Used in Gd in case of `free()`. + #[cfg(not(safeguards_balanced))] + pub(crate) fn check_rtti(&self, _method_name: &'static str) { + // Disengaged level: no-op. + } + + /// Checks only type, not liveness. + /// + /// Used in specific scenarios like `Gd::free()` where we need type validation + /// but the object may already be dead. This is an internal helper for `check_rtti()`. #[cfg(safeguards_strict)] + #[inline] pub(crate) fn check_dynamic_type(&self, call_ctx: &CallContext<'static>) { - debug_assert!( + assert!( !self.is_null(), - "{call_ctx}: cannot call method on null object", + "internal bug: {call_ctx}: cannot call method on null object", ); let rtti = self.cached_rtti.as_ref(); - // SAFETY: code surrounding RawGd ensures that `self` is non-null; above is just a sanity check against internal bugs. + // SAFETY: RawGd non-null (checked above). let rtti = unsafe { rtti.unwrap_unchecked() }; rtti.check_type::(); } + /// Creates a validated object for FFI boundary crossing. + /// + /// This is a convenience wrapper around [`ValidatedObject::validate()`]. + #[doc(hidden)] + #[inline] + pub fn validated_object(&self) -> ValidatedObject { + ValidatedObject::validate(self) + } + // Not pub(super) because used by godot::meta::args::ObjectArg. pub(crate) fn obj_sys(&self) -> sys::GDExtensionObjectPtr { self.obj as sys::GDExtensionObjectPtr @@ -428,7 +455,7 @@ where { /// Hands out a guard for a shared borrow, through which the user instance can be read. /// - /// See [`crate::obj::Gd::bind()`] for a more in depth explanation. + /// See [`Gd::bind()`] for a more in depth explanation. // Note: possible names: write/read, hold/hold_mut, r/w, r/rw, ... pub(crate) fn bind(&self) -> GdRef<'_, T> { self.check_rtti("bind"); @@ -437,7 +464,7 @@ where /// Hands out a guard for an exclusive borrow, through which the user instance can be read and written. /// - /// See [`crate::obj::Gd::bind_mut()`] for a more in depth explanation. + /// See [`Gd::bind_mut()`] for a more in depth explanation. pub(crate) fn bind_mut(&mut self) -> GdMut<'_, T> { self.check_rtti("bind_mut"); GdMut::from_guard(self.storage().unwrap().get_mut()) @@ -758,6 +785,44 @@ impl fmt::Debug for RawGd { } } +// ---------------------------------------------------------------------------------------------------------------------------------------------- +// Type-state for already-validated objects before an engine API call + +/// Type-state object pointers that have been validated for engine API calls. +/// +/// Can be passed to [`Signature`](meta::signature::Signature) methods. Performs the following checks depending on safeguard level: +/// - `disengaged`: no validation. +/// - `balanced`: liveness check only. +/// - `strict`: liveness + type inheritance check. +#[doc(hidden)] +pub struct ValidatedObject { + object_ptr: sys::GDExtensionObjectPtr, +} + +impl ValidatedObject { + /// Validates a `RawGd` according to the type's invariants (depending on safeguard level). + /// + /// # Panics + /// If validation fails. + #[doc(hidden)] + #[inline] + pub fn validate(raw_gd: &RawGd) -> Self { + raw_gd.check_rtti("validated_object"); + + Self { + object_ptr: raw_gd.obj_sys(), + } + } + + /// Extracts the object pointer from an `Option`. + /// + /// Returns null pointer for `None` (static methods), or the validated pointer for `Some`. + #[inline] + pub fn object_ptr(opt: Option<&Self>) -> sys::GDExtensionObjectPtr { + opt.map(|v| v.object_ptr).unwrap_or(ptr::null_mut()) + } +} + // ---------------------------------------------------------------------------------------------------------------------------------------------- // Reusable functions, also shared with Gd, Variant, ObjectArg. diff --git a/godot-core/src/obj/rtti.rs b/godot-core/src/obj/rtti.rs index bf21a3c47..1a22b7663 100644 --- a/godot-core/src/obj/rtti.rs +++ b/godot-core/src/obj/rtti.rs @@ -41,10 +41,15 @@ impl ObjectRtti { } } - /// Checks that the object is of type `T` or derived. + /// Validates that the object's stored type matches or inherits from `T`. /// - /// # Panics - /// In paranoid mode, if the object is not of type `T` or derived. + /// Used internally by `RawGd::check_rtti()` for type validation in strict mode. + /// + /// Only checks the cached type from RTTI construction time. + /// This may not reflect runtime type changes (which shouldn't happen). + /// + /// # Panics (strict safeguards) + /// If the stored type does not inherit from `T`. #[cfg(safeguards_strict)] #[inline] pub fn check_type(&self) { @@ -53,6 +58,7 @@ impl ObjectRtti { #[inline] pub fn instance_id(&self) -> InstanceId { + // Do not add logic or validations here, this is passed in every FFI call. self.instance_id } } From a14de8f2aa0cfa2bf2ee77938e07cf4a87cea995 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 26 Oct 2025 18:47:05 +0100 Subject: [PATCH 7/7] Add new CI jobs with safeguard Cargo features --- .github/composite/godot-itest/action.yml | 25 ++++++++++++++++-- .github/workflows/full-ci.yml | 32 ++++++++++++++++++++---- .github/workflows/minimal-ci.yml | 26 +++++++++++++++++-- 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/.github/composite/godot-itest/action.yml b/.github/composite/godot-itest/action.yml index 7045fd458..4a383eb14 100644 --- a/.github/composite/godot-itest/action.yml +++ b/.github/composite/godot-itest/action.yml @@ -56,6 +56,11 @@ inputs: default: '' description: "If provided, acts as an argument for '--target', and results in output files written to ./target/{target}" + rust-target-dir: + required: false + default: '' + description: "If provided, determines where to find the generated dylib. Example: 'release' if Godot editor build would look for debug otherwise." + rust-cache-key: required: false default: '' @@ -151,6 +156,7 @@ runs: env: RUSTFLAGS: ${{ inputs.rust-env-rustflags }} TARGET: ${{ inputs.rust-target }} + OUTPUT_DIR: ${{ inputs.rust-target-dir || 'debug' }} run: | targetArgs="" if [[ -n "$TARGET" ]]; then @@ -162,8 +168,23 @@ runs: # Instead of modifying .gdextension, rename the output directory. if [[ -n "$TARGET" ]]; then - rm -rf target/debug - mv target/$TARGET/debug target + rm -rf target/debug target/release + echo "Build output (tree -L 2 target/$TARGET/$OUTPUT_DIR):" + tree -L 2 target/$TARGET/$OUTPUT_DIR || echo "(tree not installed)" + mv target/$TARGET/$OUTPUT_DIR target/ || { + echo "::error::Output dir $TARGET/$OUTPUT_DIR not found" + exit 1 + } + # echo "Intermediate dir (tree -L 2 target):" + # tree -L 2 target || echo "(tree not installed)" + if [[ "$OUTPUT_DIR" != "debug" ]]; then + mv target/$OUTPUT_DIR target/debug + fi + echo "Resulting dir (tree -L 2 target):" + tree -L 2 target || echo "(tree not installed)" + # echo "itest.gdextension contents:" + # cat itest/godot/itest.gdextension + # echo "----------------------------------------------------" fi shell: bash diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index a11adf0a6..17dbebb25 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -378,14 +378,14 @@ jobs: godot-prebuilt-patch: '4.2.2' hot-reload: api-4-2 - # Memory checks: special Godot binaries compiled with AddressSanitizer/LeakSanitizer to detect UB/leaks. + # Memory checks: special Godot binaries compiled with AddressSanitizer/LeakSanitizer to detect UB/leaks. Always Linux. # See also https://rustc-dev-guide.rust-lang.org/sanitizers.html. # # Additionally, the Godot source is patched to make dlclose() a no-op, as unloading dynamic libraries loses stacktrace and # cause false positives like println!. See https://github.com/google/sanitizers/issues/89. # # There is also a gcc variant besides clang, which is currently not used. - - name: linux-memcheck-nightly + - name: memcheck-nightly os: ubuntu-22.04 artifact-name: linux-memcheck-nightly godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san @@ -395,7 +395,28 @@ jobs: # Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two. rust-target: x86_64-unknown-linux-gnu - - name: linux-memcheck-4.2 + - name: memcheck-release-disengaged + os: ubuntu-22.04 + artifact-name: linux-memcheck-nightly + godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san + rust-toolchain: nightly + rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address + # Currently without itest/codegen-full-experimental. + rust-extra-args: --release --features godot/safeguards-release-disengaged + rust-target: x86_64-unknown-linux-gnu + rust-target-dir: release # We're running Godot debug build with Rust release dylib. + + - name: memcheck-dev-balanced + os: ubuntu-22.04 + artifact-name: linux-memcheck-nightly + godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san + rust-toolchain: nightly + rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address + # Currently without itest/codegen-full-experimental. + rust-extra-args: --features godot/safeguards-dev-balanced + rust-target: x86_64-unknown-linux-gnu + + - name: memcheck-4.2 os: ubuntu-22.04 artifact-name: linux-memcheck-4.2 godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san @@ -419,6 +440,7 @@ jobs: rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }} rust-env-rustflags: ${{ matrix.rust-env-rustflags }} rust-target: ${{ matrix.rust-target }} + rust-target-dir: ${{ matrix.rust-target-dir }} rust-cache-key: ${{ matrix.rust-cache-key }} with-llvm: ${{ contains(matrix.name, 'macos') && contains(matrix.rust-extra-args, 'api-custom') }} godot-check-header: ${{ matrix.godot-check-header }} @@ -500,7 +522,7 @@ jobs: - name: "Determine success or failure" run: | DEPENDENCIES='${{ toJson(needs) }}' - + echo "Dependency jobs:" all_success=true for job in $(echo "$DEPENDENCIES" | jq -r 'keys[]'); do @@ -510,7 +532,7 @@ jobs: all_success=false fi done - + if [[ "$all_success" == "false" ]]; then echo "One or more dependency jobs failed or were cancelled." exit 1 diff --git a/.github/workflows/minimal-ci.yml b/.github/workflows/minimal-ci.yml index 691f7669f..ae02c0bce 100644 --- a/.github/workflows/minimal-ci.yml +++ b/.github/workflows/minimal-ci.yml @@ -210,9 +210,9 @@ jobs: godot-binary: godot.linuxbsd.editor.dev.x86_64 godot-prebuilt-patch: '4.2.2' - # Memory checkers + # Memory checkers (always Linux). - - name: linux-memcheck + - name: memcheck os: ubuntu-22.04 artifact-name: linux-memcheck-nightly godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san @@ -222,6 +222,27 @@ jobs: # Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two. rust-target: x86_64-unknown-linux-gnu + - name: memcheck-release-disengaged + os: ubuntu-22.04 + artifact-name: linux-memcheck-nightly + godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san + rust-toolchain: nightly + rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address + # Currently without itest/codegen-full-experimental. + rust-extra-args: --release --features godot/safeguards-release-disengaged + rust-target: x86_64-unknown-linux-gnu + rust-target-dir: release # We're running Godot debug build with Rust release dylib. + + - name: memcheck-dev-balanced + os: ubuntu-22.04 + artifact-name: linux-memcheck-nightly + godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san + rust-toolchain: nightly + rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address + # Currently without itest/codegen-full-experimental. + rust-extra-args: --features godot/safeguards-dev-balanced + rust-target: x86_64-unknown-linux-gnu + steps: - uses: actions/checkout@v4 @@ -236,6 +257,7 @@ jobs: rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }} rust-env-rustflags: ${{ matrix.rust-env-rustflags }} rust-target: ${{ matrix.rust-target }} + rust-target-dir: ${{ matrix.rust-target-dir }} with-llvm: ${{ contains(matrix.name, 'macos') && contains(matrix.rust-extra-args, 'api-custom') }} godot-check-header: ${{ matrix.godot-check-header }} godot-indirect-json: ${{ matrix.godot-indirect-json }}