Skip to content

Commit fd39b37

Browse files
bors[bot]bors[bot]
authored
Try #133:
2 parents b3b9e0e + 08b934b commit fd39b37

File tree

17 files changed

+214
-197
lines changed

17 files changed

+214
-197
lines changed

‎.github/workflows/full-ci.yml

+5-2
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,7 @@ jobs:
138138
godot-itest:
139139
name: godot-itest (${{ matrix.name }})
140140
runs-on: ${{ matrix.os }}
141-
# TODO: continue-on-error: false, as soon as memory errors are fixed
142-
continue-on-error: ${{ contains(matrix.name, 'memcheck') }}
141+
continue-on-error: false
143142
timeout-minutes: 24
144143
strategy:
145144
fail-fast: false # cancel all jobs as soon as one fails?
@@ -165,6 +164,10 @@ jobs:
165164
rust-toolchain: stable
166165
godot-binary: godot.linuxbsd.editor.dev.x86_64
167166

167+
# Special Godot binaries compiled with AddressSanitizer/LeakSanitizer to detect UB/leaks.
168+
# Additionally, the Godot source is patched to make dlclose() a no-op, as unloading dynamic libraries loses stacktrace and
169+
# cause false positives like println!. See https://github.com/google/sanitizers/issues/89.
170+
# The gcc version can possibly be removed later, as it is slower and needs a larger artifact than the clang one.
168171
- name: linux-memcheck-gcc
169172
os: ubuntu-20.04
170173
rust-toolchain: stable

‎godot-codegen/src/class_generator.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -686,7 +686,7 @@ impl VariantFfi {
686686
fn type_ptr() -> Self {
687687
Self {
688688
sys_method: ident("sys_const"),
689-
from_sys_init_method: ident("from_sys_init"),
689+
from_sys_init_method: ident("from_sys_init_default"),
690690
}
691691
}
692692
}
@@ -876,7 +876,7 @@ fn make_return(
876876
}
877877
(None, Some(return_ty)) => {
878878
quote! {
879-
<#return_ty as sys::GodotFfi>::from_sys_init(|return_ptr| {
879+
<#return_ty as sys::GodotFfi>::from_sys_init_default(|return_ptr| {
880880
#ptrcall_invocation
881881
})
882882
}

‎godot-core/src/builtin/array.rs

+18-26
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,11 @@ impl_builtin_froms!(Array;
7070

7171
impl<T: VariantMetadata> TypedArray<T> {
7272
fn from_opaque(opaque: sys::types::OpaqueArray) -> Self {
73-
let array = Self {
73+
// Note: type is not yet checked at this point, because array has not yet been initialized!
74+
Self {
7475
opaque,
7576
_phantom: PhantomData,
76-
};
77-
array.check_type();
78-
array
77+
}
7978
}
8079

8180
/// Returns the number of elements in the array. Equivalent of `size()` in Godot.
@@ -320,8 +319,9 @@ impl<T: VariantMetadata> TypedArray<T> {
320319
/// # Panics
321320
///
322321
/// If the inner type doesn't match `T` or no type is set at all.
323-
fn check_type(&self) {
322+
fn with_checked_type(self) -> Self {
324323
assert_eq!(self.type_info(), TypeInfo::new::<T>());
324+
self
325325
}
326326

327327
/// Sets the type of the inner array. Can only be called once, directly after creation.
@@ -567,14 +567,9 @@ impl<T: VariantMetadata + ToVariant> TypedArray<T> {
567567
// }
568568

569569
impl<T: VariantMetadata> GodotFfi for TypedArray<T> {
570-
ffi_methods! {
571-
type sys::GDExtensionTypePtr = *mut Opaque;
572-
fn from_sys;
573-
fn sys;
574-
fn write_sys;
575-
}
570+
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
576571

577-
unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
572+
unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
578573
// Can't use uninitialized pointer -- Array CoW implementation in C++ expects that on
579574
// assignment, the target CoW pointer is either initialized or nullptr
580575

@@ -598,28 +593,25 @@ impl<T: VariantMetadata> fmt::Debug for TypedArray<T> {
598593
/// [`Array::duplicate_deep()`].
599594
impl<T: VariantMetadata> Share for TypedArray<T> {
600595
fn share(&self) -> Self {
601-
unsafe {
596+
let array = unsafe {
602597
Self::from_sys_init(|self_ptr| {
603598
let ctor = ::godot_ffi::builtin_fn!(array_construct_copy);
604599
let args = [self.sys_const()];
605600
ctor(self_ptr, args.as_ptr());
606601
})
607-
}
602+
};
603+
array.with_checked_type()
608604
}
609605
}
610606

611607
impl<T: VariantMetadata> Default for TypedArray<T> {
612608
#[inline]
613609
fn default() -> Self {
614-
// Note: can't use from_sys_init(), as that calls the default constructor
615-
// (because most assignments expect initialized target type)
616-
let mut uninit = std::mem::MaybeUninit::<TypedArray<T>>::uninit();
617610
let mut array = unsafe {
618-
let self_ptr = (*uninit.as_mut_ptr()).sys_mut();
619-
sys::builtin_call! {
620-
array_construct_default(self_ptr, std::ptr::null_mut())
621-
};
622-
uninit.assume_init()
611+
Self::from_sys_init(|self_ptr| {
612+
let ctor = sys::builtin_fn!(array_construct_default);
613+
ctor(self_ptr, std::ptr::null_mut())
614+
})
623615
};
624616
array.init_inner_type();
625617
array
@@ -661,14 +653,14 @@ impl<T: VariantMetadata> FromVariant for TypedArray<T> {
661653
if variant.get_type() != Self::variant_type() {
662654
return Err(VariantConversionError);
663655
}
664-
let result = unsafe {
665-
Self::from_sys_init(|self_ptr| {
656+
let array = unsafe {
657+
Self::from_sys_init_default(|self_ptr| {
666658
let array_from_variant = sys::builtin_fn!(array_from_variant);
667659
array_from_variant(self_ptr, variant.var_sys());
668660
})
669661
};
670662

671-
Ok(result)
663+
Ok(array.with_checked_type())
672664
}
673665
}
674666

@@ -773,7 +765,7 @@ impl<T: VariantMetadata> PartialEq for TypedArray<T> {
773765
let mut result = false;
774766
sys::builtin_call! {
775767
array_operator_equal(self.sys(), other.sys(), result.sys_mut())
776-
};
768+
}
777769
result
778770
}
779771
}

‎godot-core/src/builtin/dictionary.rs

+2-7
Original file line numberDiff line numberDiff line change
@@ -240,14 +240,9 @@ impl Dictionary {
240240
// Traits
241241

242242
impl GodotFfi for Dictionary {
243-
ffi_methods! {
244-
type sys::GDExtensionTypePtr = *mut Opaque;
245-
fn from_sys;
246-
fn sys;
247-
fn write_sys;
248-
}
243+
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
249244

250-
unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
245+
unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
251246
// Can't use uninitialized pointer -- Dictionary CoW implementation in C++ expects that on
252247
// assignment, the target CoW pointer is either initialized or nullptr
253248

‎godot-core/src/builtin/macros.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ macro_rules! impl_builtin_traits_inner {
3333
#[inline]
3434
fn clone(&self) -> Self {
3535
unsafe {
36-
Self::from_sys_init(|self_ptr| {
36+
Self::from_sys_init_default(|self_ptr| {
3737
let ctor = ::godot_ffi::builtin_fn!($gd_method);
3838
let args = [self.sys_const()];
3939
ctor(self_ptr, args.as_ptr());
@@ -116,7 +116,7 @@ macro_rules! impl_builtin_traits_inner {
116116
return Err($crate::builtin::variant::VariantConversionError)
117117
}
118118
let result = unsafe {
119-
Self::from_sys_init(|self_ptr| {
119+
Self::from_sys_init_default(|self_ptr| {
120120
let converter = sys::builtin_fn!($gd_method);
121121
converter(self_ptr, variant.var_sys());
122122
})
@@ -168,7 +168,7 @@ macro_rules! impl_builtin_froms {
168168
$(impl From<&$From> for $To {
169169
fn from(other: &$From) -> Self {
170170
unsafe {
171-
Self::from_sys_init(|ptr| {
171+
Self::from_sys_init(|ptr| { // !!!
172172
let args = [other.sys_const()];
173173
::godot_ffi::builtin_call! {
174174
$from_fn(ptr, args.as_ptr())

‎godot-core/src/builtin/node_path.rs

+9-3
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
use crate::builtin::GodotString;
88
use godot_ffi as sys;
9-
use godot_ffi::{ffi_methods, GodotFfi};
9+
use godot_ffi::{ffi_methods, GDExtensionTypePtr, GodotFfi};
1010
use std::fmt::{Display, Formatter, Result as FmtResult};
1111

1212
pub struct NodePath {
@@ -21,12 +21,18 @@ impl NodePath {
2121

2222
impl GodotFfi for NodePath {
2323
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
24+
25+
unsafe fn from_sys_init_default(init_fn: impl FnOnce(GDExtensionTypePtr)) -> Self {
26+
let mut result = Self::default();
27+
init_fn(result.sys_mut());
28+
result
29+
}
2430
}
2531

2632
impl From<&GodotString> for NodePath {
2733
fn from(path: &GodotString) -> Self {
2834
unsafe {
29-
Self::from_sys_init(|self_ptr| {
35+
Self::from_sys_init_default(|self_ptr| {
3036
let ctor = sys::builtin_fn!(node_path_from_string);
3137
let args = [path.sys_const()];
3238
ctor(self_ptr, args.as_ptr());
@@ -38,7 +44,7 @@ impl From<&GodotString> for NodePath {
3844
impl From<&NodePath> for GodotString {
3945
fn from(path: &NodePath) -> Self {
4046
unsafe {
41-
Self::from_sys_init(|self_ptr| {
47+
Self::from_sys_init_default(|self_ptr| {
4248
let ctor = sys::builtin_fn!(string_from_node_path);
4349
let args = [path.sys_const()];
4450
ctor(self_ptr, args.as_ptr());

‎godot-core/src/builtin/others.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ impl Callable {
5050
// upcast not needed
5151
let method = method.into();
5252
unsafe {
53-
Self::from_sys_init(|self_ptr| {
53+
Self::from_sys_init_default(|self_ptr| {
5454
let ctor = sys::builtin_fn!(callable_from_object_method);
5555
let args = [object.sys_const(), method.sys_const()];
5656
ctor(self_ptr, args.as_ptr());
@@ -66,6 +66,7 @@ impl Callable {
6666

6767
impl_builtin_traits! {
6868
for Callable {
69+
// Default => callable_construct_default;
6970
FromVariant => callable_from_variant;
7071
}
7172
}

‎godot-core/src/builtin/packed_array.rs

+2-7
Original file line numberDiff line numberDiff line change
@@ -386,14 +386,9 @@ macro_rules! impl_packed_array {
386386
}
387387

388388
impl GodotFfi for $PackedArray {
389-
ffi_methods! {
390-
type sys::GDExtensionTypePtr = *mut Opaque;
391-
fn from_sys;
392-
fn sys;
393-
fn write_sys;
394-
}
389+
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
395390

396-
unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
391+
unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
397392
// Can't use uninitialized pointer -- Vector CoW implementation in C++ expects that
398393
// on assignment, the target CoW pointer is either initialized or nullptr
399394

‎godot-core/src/builtin/string.rs

+3-10
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,9 @@ impl GodotString {
3737
}
3838

3939
impl GodotFfi for GodotString {
40-
ffi_methods! {
41-
type sys::GDExtensionTypePtr = *mut Opaque;
42-
fn from_sys;
43-
fn sys;
44-
fn write_sys;
45-
}
40+
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
4641

47-
unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
42+
unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
4843
// Can't use uninitialized pointer -- String CoW implementation in C++ expects that on assignment,
4944
// the target CoW pointer is either initialized or nullptr
5045

@@ -56,9 +51,7 @@ impl GodotFfi for GodotString {
5651

5752
impl Default for GodotString {
5853
fn default() -> Self {
59-
// Note: can't use from_sys_init(), as that calls the default constructor
60-
// (because most assignments expect initialized target type)
61-
54+
// FIXME use from_sys_init()
6255
let mut uninit = std::mem::MaybeUninit::<GodotString>::uninit();
6356

6457
unsafe {

‎godot-core/src/builtin/string_name.rs

+4-9
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,9 @@ impl StringName {
3333
}
3434

3535
impl GodotFfi for StringName {
36-
ffi_methods! {
37-
type sys::GDExtensionTypePtr = *mut Opaque;
38-
fn from_sys;
39-
fn sys;
40-
fn write_sys;
41-
}
36+
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
4237

43-
unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
38+
unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
4439
// Can't use uninitialized pointer -- StringName implementation in C++ expects that on assignment,
4540
// the target type is a valid string (possibly empty)
4641

@@ -106,7 +101,7 @@ impl Hash for StringName {
106101
impl From<&GodotString> for StringName {
107102
fn from(s: &GodotString) -> Self {
108103
unsafe {
109-
Self::from_sys_init(|self_ptr| {
104+
Self::from_sys_init_default(|self_ptr| {
110105
let ctor = sys::builtin_fn!(string_name_from_string);
111106
let args = [s.sys_const()];
112107
ctor(self_ptr, args.as_ptr());
@@ -128,7 +123,7 @@ where
128123
impl From<&StringName> for GodotString {
129124
fn from(s: &StringName) -> Self {
130125
unsafe {
131-
Self::from_sys_init(|self_ptr| {
126+
Self::from_sys_init_default(|self_ptr| {
132127
let ctor = sys::builtin_fn!(string_from_string_name);
133128
let args = [s.sys_const()];
134129
ctor(self_ptr, args.as_ptr());

‎godot-core/src/builtin/variant/impls.rs

+10-8
Original file line numberDiff line numberDiff line change
@@ -55,19 +55,21 @@ macro_rules! impl_variant_traits {
5555

5656
impl FromVariant for $T {
5757
fn try_from_variant(variant: &Variant) -> Result<Self, VariantConversionError> {
58+
// Type check -- at the moment, this must strictly match
59+
if variant.get_type() != Self::variant_type() {
60+
return Err(VariantConversionError)
61+
}
62+
5863
// In contrast to T -> Variant, the conversion Variant -> T assumes
5964
// that the destination is initialized (at least for some T). For example:
6065
// void String::operator=(const String &p_str) { _cowdata._ref(p_str._cowdata); }
6166
// does a copy-on-write and explodes if this->_cowdata is not initialized.
62-
// We can thus NOT use Self::from_sys_init().
63-
if variant.get_type() != Self::variant_type() {
64-
return Err(VariantConversionError)
65-
}
66-
let mut value = <$T>::default();
67+
// We can thus NOT use Self::from_sys_init(). let mut value = <$T>::default();
6768
let result = unsafe {
68-
let converter = sys::builtin_fn!($to_fn);
69-
converter(value.sys_mut(), variant.var_sys());
70-
value
69+
<$T>::from_sys_init_default(|self_ptr| {
70+
let converter = sys::builtin_fn!($to_fn);
71+
converter(self_ptr, variant.var_sys());
72+
})
7173
};
7274

7375
Ok(result)

‎godot-core/src/builtin/variant/mod.rs

+8-13
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,7 @@ pub struct Variant {
2626
impl Variant {
2727
/// Create an empty variant (`null` value in GDScript).
2828
pub fn nil() -> Self {
29-
unsafe {
30-
Self::from_var_sys_init(|variant_ptr| {
31-
interface_fn!(variant_new_nil)(variant_ptr);
32-
})
33-
}
29+
Self::default()
3430
}
3531

3632
/// Create a variant holding a non-nil value.
@@ -180,14 +176,9 @@ impl Variant {
180176
}
181177

182178
impl GodotFfi for Variant {
183-
ffi_methods! {
184-
type sys::GDExtensionTypePtr = *mut Opaque;
185-
fn from_sys;
186-
fn sys;
187-
fn write_sys;
188-
}
179+
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
189180

190-
unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
181+
unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
191182
// Can't use uninitialized pointer -- Variant implementation in C++ expects that on assignment,
192183
// the target pointer is an initialized Variant
193184

@@ -217,7 +208,11 @@ impl Drop for Variant {
217208

218209
impl Default for Variant {
219210
fn default() -> Self {
220-
Self::nil()
211+
unsafe {
212+
Self::from_var_sys_init(|variant_ptr| {
213+
interface_fn!(variant_new_nil)(variant_ptr);
214+
})
215+
}
221216
}
222217
}
223218

0 commit comments

Comments
 (0)