Skip to content

Commit 2b9bdde

Browse files
committed
Refactor and simplify object liveness/RTTI checks
1 parent de25cc7 commit 2b9bdde

File tree

5 files changed

+116
-57
lines changed

5 files changed

+116
-57
lines changed

godot-codegen/src/generator/classes.rs

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -192,12 +192,14 @@ fn make_class(class: &Class, ctx: &mut Context, view: &ApiView) -> GeneratedClas
192192
let (assoc_memory, assoc_dyn_memory, is_exportable) = make_bounds(class, ctx);
193193

194194
let internal_methods = quote! {
195-
fn __checked_id(&self) -> Option<crate::obj::InstanceId> {
196-
// SAFETY: only Option due to layout-compatibility with RawGd<T>; it is always Some because stored in Gd<T> which is non-null.
197-
let rtti = unsafe { self.rtti.as_ref().unwrap_unchecked() };
198-
#[cfg(safeguards_strict)]
199-
rtti.check_type::<Self>();
200-
Some(rtti.instance_id())
195+
/// Creates a validated object for FFI boundary crossing.
196+
///
197+
/// Low-level internal method. Validation (liveness/type checks) depend on safeguard level.
198+
fn __validated_obj(&self) -> crate::obj::ValidatedObject {
199+
// SAFETY: Self has the same layout as RawGd<Self> (object_ptr + rtti fields in same order).
200+
let raw_gd = unsafe { std::mem::transmute::<&Self, &crate::obj::RawGd<Self>>(self) };
201+
202+
raw_gd.validated_object()
201203
}
202204

203205
#[doc(hidden)]
@@ -580,10 +582,10 @@ fn make_class_method_definition(
580582

581583
let table_index = ctx.get_table_index(&MethodTableKey::from_class(class, method));
582584

583-
let maybe_instance_id = if method.qualifier() == FnQualifier::Static {
585+
let validated_obj = if method.qualifier() == FnQualifier::Static {
584586
quote! { None }
585587
} else {
586-
quote! { self.__checked_id() }
588+
quote! { Some(self.__validated_obj()) }
587589
};
588590

589591
let fptr_access = if cfg!(feature = "codegen-lazy-fptrs") {
@@ -599,16 +601,14 @@ fn make_class_method_definition(
599601
quote! { fptr_by_index(#table_index) }
600602
};
601603

602-
let object_ptr = &receiver.ffi_arg;
603604
let ptrcall_invocation = quote! {
604605
let method_bind = sys::#get_method_table().#fptr_access;
605606

606607
Signature::<CallParams, CallRet>::out_class_ptrcall(
607608
method_bind,
608609
#rust_class_name,
609610
#rust_method_name,
610-
#object_ptr,
611-
#maybe_instance_id,
611+
#validated_obj,
612612
args,
613613
)
614614
};
@@ -620,8 +620,7 @@ fn make_class_method_definition(
620620
method_bind,
621621
#rust_class_name,
622622
#rust_method_name,
623-
#object_ptr,
624-
#maybe_instance_id,
623+
#validated_obj,
625624
args,
626625
varargs
627626
)

godot-core/src/classes/class_runtime.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,13 @@
1010
use crate::builtin::{GString, StringName, Variant, VariantType};
1111
#[cfg(safeguards_strict)]
1212
use crate::classes::{ClassDb, Object};
13+
#[cfg(safeguards_balanced)]
1314
use crate::meta::CallContext;
1415
#[cfg(safeguards_strict)]
1516
use crate::meta::ClassId;
16-
use crate::obj::{bounds, Bounds, Gd, GodotClass, InstanceId, RawGd, Singleton};
17+
#[cfg(safeguards_strict)]
18+
use crate::obj::Singleton;
19+
use crate::obj::{bounds, Bounds, Gd, GodotClass, InstanceId, RawGd};
1720
use crate::sys;
1821

1922
pub(crate) fn debug_string<T: GodotClass>(
@@ -191,6 +194,12 @@ where
191194
Gd::<T>::from_obj_sys(object_ptr)
192195
}
193196

197+
/// Checks that the object with the given instance ID is still alive and that the pointer is valid.
198+
///
199+
/// This does **not** perform type checking — use `ensure_object_type()` for that.
200+
///
201+
/// # Panics (balanced+strict safeguards)
202+
/// If the object has been freed or the instance ID points to a different object.
194203
#[cfg(safeguards_balanced)]
195204
pub(crate) fn ensure_object_alive(
196205
instance_id: InstanceId,

godot-core/src/meta/signature.rs

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use crate::meta::{
1717
FromGodot, GodotConvert, GodotType, InParamTuple, MethodParamOrReturnInfo, OutParamTuple,
1818
ParamTuple, ToGodot,
1919
};
20-
use crate::obj::{GodotClass, InstanceId};
20+
use crate::obj::{GodotClass, ValidatedObject};
2121

2222
pub(super) type CallResult<R> = Result<R, CallError>;
2323

@@ -62,7 +62,6 @@ where
6262
/// Receive a varcall from Godot, and return the value in `ret` as a variant pointer.
6363
///
6464
/// # Safety
65-
///
6665
/// A call to this function must be caused by Godot making a varcall with parameters `Params` and return type `Ret`.
6766
#[inline]
6867
pub unsafe fn in_varcall(
@@ -126,30 +125,20 @@ impl<Params: OutParamTuple, Ret: FromGodot> Signature<Params, Ret> {
126125
/// Make a varcall to the Godot engine for a class method.
127126
///
128127
/// # Safety
129-
///
130-
/// - `object_ptr` must be a live instance of a class with the type expected by `method_bind`
131128
/// - `method_bind` must expect explicit args `args`, varargs `varargs`, and return a value of type `Ret`
132129
#[inline]
133130
pub unsafe fn out_class_varcall(
134131
method_bind: sys::ClassMethodBind,
135132
// Separate parameters to reduce tokens in generated class API.
136133
class_name: &'static str,
137134
method_name: &'static str,
138-
object_ptr: sys::GDExtensionObjectPtr,
139-
maybe_instance_id: Option<InstanceId>, // if not static
135+
validated_obj: Option<ValidatedObject>,
140136
args: Params,
141137
varargs: &[Variant],
142138
) -> CallResult<Ret> {
143139
let call_ctx = CallContext::outbound(class_name, method_name);
144140
//$crate::out!("out_class_varcall: {call_ctx}");
145141

146-
// Note: varcalls are not safe from failing, if they happen through an object pointer -> validity check necessary.
147-
// paranoid since we already check the validity in check_rtti, this is unlikely to happen.
148-
#[cfg(safeguards_strict)]
149-
if let Some(instance_id) = maybe_instance_id {
150-
crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx);
151-
}
152-
153142
let class_fn = sys::interface_fn!(object_method_bind_call);
154143

155144
let variant = args.with_variants(|explicit_args| {
@@ -162,7 +151,7 @@ impl<Params: OutParamTuple, Ret: FromGodot> Signature<Params, Ret> {
162151
let mut err = sys::default_call_error();
163152
class_fn(
164153
method_bind.0,
165-
object_ptr,
154+
ValidatedObject::object_ptr(validated_obj.as_ref()),
166155
variant_ptrs.as_ptr(),
167156
variant_ptrs.len() as i64,
168157
return_ptr,
@@ -290,35 +279,26 @@ impl<Params: OutParamTuple, Ret: FromGodot> Signature<Params, Ret> {
290279
/// Make a ptrcall to the Godot engine for a class method.
291280
///
292281
/// # Safety
293-
///
294-
/// - `object_ptr` must be a live instance of a class with the type expected by `method_bind`
295282
/// - `method_bind` must expect explicit args `args`, and return a value of type `Ret`
296283
#[inline]
297284
pub unsafe fn out_class_ptrcall(
298285
method_bind: sys::ClassMethodBind,
299286
// Separate parameters to reduce tokens in generated class API.
300287
class_name: &'static str,
301288
method_name: &'static str,
302-
object_ptr: sys::GDExtensionObjectPtr,
303-
maybe_instance_id: Option<InstanceId>, // if not static
289+
validated_obj: Option<ValidatedObject>,
304290
args: Params,
305291
) -> Ret {
306292
let call_ctx = CallContext::outbound(class_name, method_name);
307293
// $crate::out!("out_class_ptrcall: {call_ctx}");
308294

309-
// paranoid since we already check the validity in check_rtti, this is unlikely to happen.
310-
#[cfg(safeguards_strict)]
311-
if let Some(instance_id) = maybe_instance_id {
312-
crate::classes::ensure_object_alive(instance_id, object_ptr, &call_ctx);
313-
}
314-
315295
let class_fn = sys::interface_fn!(object_method_bind_ptrcall);
316296

317297
unsafe {
318298
Self::raw_ptrcall(args, &call_ctx, |explicit_args, return_ptr| {
319299
class_fn(
320300
method_bind.0,
321-
object_ptr,
301+
ValidatedObject::object_ptr(validated_obj.as_ref()),
322302
explicit_args.as_ptr(),
323303
return_ptr,
324304
);

godot-core/src/obj/raw_gd.rs

Lines changed: 80 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -381,34 +381,61 @@ impl<T: GodotClass> RawGd<T> {
381381
}
382382
}
383383

384-
/// Verify that the object is non-null and alive. In paranoid mode, additionally verify that it is of type `T` or derived.
384+
/// Validates object for use in `RawGd`/`Gd` methods (not FFI boundary calls).
385+
///
386+
/// This is used for Rust-side object operations like `bind()`, `clone()`, `ffi_cast()`, etc.
387+
/// For FFI boundary calls (generated engine methods), validation happens in signature.rs instead.
388+
///
389+
/// # Panics
390+
/// If validation fails.
391+
#[cfg(safeguards_balanced)]
385392
pub(crate) fn check_rtti(&self, method_name: &'static str) {
386-
#[cfg(safeguards_balanced)]
387-
{
388-
let call_ctx = CallContext::gd::<T>(method_name);
389-
#[cfg(safeguards_strict)]
390-
self.check_dynamic_type(&call_ctx);
391-
let instance_id = unsafe { self.instance_id_unchecked().unwrap_unchecked() };
393+
let call_ctx = CallContext::gd::<T>(method_name);
392394

393-
classes::ensure_object_alive(instance_id, self.obj_sys(), &call_ctx);
394-
}
395+
// Type check (strict only).
396+
#[cfg(safeguards_strict)]
397+
self.check_dynamic_type(&call_ctx);
398+
399+
// SAFETY: check_rtti() is only called for non-null pointers.
400+
let instance_id = unsafe { self.instance_id_unchecked().unwrap_unchecked() };
401+
402+
// Liveness check (balanced + strict).
403+
classes::ensure_object_alive(instance_id, self.obj_sys(), &call_ctx);
395404
}
396405

397-
/// Checks only type, not alive-ness. Used in Gd<T> in case of `free()`.
406+
#[cfg(not(safeguards_balanced))]
407+
pub(crate) fn check_rtti(&self, _method_name: &'static str) {
408+
// Disengaged level: no-op.
409+
}
410+
411+
/// Checks only type, not liveness.
412+
///
413+
/// Used in specific scenarios like `Gd<T>::free()` where we need type validation
414+
/// but the object may already be dead. This is an internal helper for `check_rtti()`.
398415
#[cfg(safeguards_strict)]
416+
#[inline]
399417
pub(crate) fn check_dynamic_type(&self, call_ctx: &CallContext<'static>) {
400-
debug_assert!(
418+
assert!(
401419
!self.is_null(),
402-
"{call_ctx}: cannot call method on null object",
420+
"internal bug: {call_ctx}: cannot call method on null object",
403421
);
404422

405423
let rtti = self.cached_rtti.as_ref();
406424

407-
// SAFETY: code surrounding RawGd<T> ensures that `self` is non-null; above is just a sanity check against internal bugs.
425+
// SAFETY: RawGd non-null (checked above).
408426
let rtti = unsafe { rtti.unwrap_unchecked() };
409427
rtti.check_type::<T>();
410428
}
411429

430+
/// Creates a validated object for FFI boundary crossing.
431+
///
432+
/// This is a convenience wrapper around [`ValidatedObject::validate()`].
433+
#[doc(hidden)]
434+
#[inline]
435+
pub fn validated_object(&self) -> ValidatedObject {
436+
ValidatedObject::validate(self)
437+
}
438+
412439
// Not pub(super) because used by godot::meta::args::ObjectArg.
413440
pub(crate) fn obj_sys(&self) -> sys::GDExtensionObjectPtr {
414441
self.obj as sys::GDExtensionObjectPtr
@@ -428,7 +455,7 @@ where
428455
{
429456
/// Hands out a guard for a shared borrow, through which the user instance can be read.
430457
///
431-
/// See [`crate::obj::Gd::bind()`] for a more in depth explanation.
458+
/// See [`Gd::bind()`] for a more in depth explanation.
432459
// Note: possible names: write/read, hold/hold_mut, r/w, r/rw, ...
433460
pub(crate) fn bind(&self) -> GdRef<'_, T> {
434461
self.check_rtti("bind");
@@ -437,7 +464,7 @@ where
437464

438465
/// Hands out a guard for an exclusive borrow, through which the user instance can be read and written.
439466
///
440-
/// See [`crate::obj::Gd::bind_mut()`] for a more in depth explanation.
467+
/// See [`Gd::bind_mut()`] for a more in depth explanation.
441468
pub(crate) fn bind_mut(&mut self) -> GdMut<'_, T> {
442469
self.check_rtti("bind_mut");
443470
GdMut::from_guard(self.storage().unwrap().get_mut())
@@ -758,6 +785,44 @@ impl<T: GodotClass> fmt::Debug for RawGd<T> {
758785
}
759786
}
760787

788+
// ----------------------------------------------------------------------------------------------------------------------------------------------
789+
// Type-state for already-validated objects before an engine API call
790+
791+
/// Type-state object pointers that have been validated for engine API calls.
792+
///
793+
/// Can be passed to [`Signature`](meta::signature::Signature) methods. Performs the following checks depending on safeguard level:
794+
/// - `disengaged`: no validation.
795+
/// - `balanced`: liveness check only.
796+
/// - `strict`: liveness + type inheritance check.
797+
#[doc(hidden)]
798+
pub struct ValidatedObject {
799+
object_ptr: sys::GDExtensionObjectPtr,
800+
}
801+
802+
impl ValidatedObject {
803+
/// Validates a `RawGd<T>` according to the type's invariants (depending on safeguard level).
804+
///
805+
/// # Panics
806+
/// If validation fails.
807+
#[doc(hidden)]
808+
#[inline]
809+
pub fn validate<T: GodotClass>(raw_gd: &RawGd<T>) -> Self {
810+
raw_gd.check_rtti("validated_object");
811+
812+
Self {
813+
object_ptr: raw_gd.obj_sys(),
814+
}
815+
}
816+
817+
/// Extracts the object pointer from an `Option<ValidatedObject>`.
818+
///
819+
/// Returns null pointer for `None` (static methods), or the validated pointer for `Some`.
820+
#[inline]
821+
pub fn object_ptr(opt: Option<&Self>) -> sys::GDExtensionObjectPtr {
822+
opt.map(|v| v.object_ptr).unwrap_or(ptr::null_mut())
823+
}
824+
}
825+
761826
// ----------------------------------------------------------------------------------------------------------------------------------------------
762827
// Reusable functions, also shared with Gd, Variant, ObjectArg.
763828

godot-core/src/obj/rtti.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,15 @@ impl ObjectRtti {
4141
}
4242
}
4343

44-
/// Checks that the object is of type `T` or derived.
44+
/// Validates that the object's stored type matches or inherits from `T`.
4545
///
46-
/// # Panics
47-
/// In paranoid mode, if the object is not of type `T` or derived.
46+
/// Used internally by `RawGd::check_rtti()` for type validation in strict mode.
47+
///
48+
/// Only checks the cached type from RTTI construction time.
49+
/// This may not reflect runtime type changes (which shouldn't happen).
50+
///
51+
/// # Panics (strict safeguards)
52+
/// If the stored type does not inherit from `T`.
4853
#[cfg(safeguards_strict)]
4954
#[inline]
5055
pub fn check_type<T: GodotClass>(&self) {
@@ -53,6 +58,7 @@ impl ObjectRtti {
5358

5459
#[inline]
5560
pub fn instance_id(&self) -> InstanceId {
61+
// Do not add logic or validations here, this is passed in every FFI call.
5662
self.instance_id
5763
}
5864
}

0 commit comments

Comments
 (0)