From 92084df65f52aa15b704279fb6d8d26a3ee71809 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 12 Jan 2024 00:08:38 +0100 Subject: [PATCH] generator: Wrap `_as_c_str()` getter for possibly-pointers in `Option` (#860) While this function is already marked `unsafe` to represent cases where an invalid pointer might be dereferenced, it should at least be obvious to the caller that there is a real chance for `NULL` pointers in these `CStr` getters, which will now be returned as `None`. This function won't be used in `Debug` now as the dereference operation is still `unsafe`. The `_as_c_str()` getters for static arrays is left untouched, as the data is read directly from the known-valid struct here. --- ash/src/vk/definitions.rs | 112 ++++++++++++++++++++++++++++---------- generator/src/lib.rs | 15 +++-- 2 files changed, 95 insertions(+), 32 deletions(-) diff --git a/ash/src/vk/definitions.rs b/ash/src/vk/definitions.rs index 04df6f5..e2af648 100644 --- a/ash/src/vk/definitions.rs +++ b/ash/src/vk/definitions.rs @@ -1062,8 +1062,12 @@ impl<'a> ApplicationInfo<'a> { self } #[inline] - pub unsafe fn application_name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_application_name) + pub unsafe fn application_name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_application_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_application_name)) + } } #[inline] pub fn application_version(mut self, application_version: u32) -> Self { @@ -1076,8 +1080,12 @@ impl<'a> ApplicationInfo<'a> { self } #[inline] - pub unsafe fn engine_name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_engine_name) + pub unsafe fn engine_name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_engine_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_engine_name)) + } } #[inline] pub fn engine_version(mut self, engine_version: u32) -> Self { @@ -3755,8 +3763,12 @@ impl<'a> PipelineShaderStageCreateInfo<'a> { self } #[inline] - pub unsafe fn name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_name) + pub unsafe fn name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_name)) + } } #[inline] pub fn specialization_info(mut self, specialization_info: &'a SpecializationInfo<'a>) -> Self { @@ -7869,8 +7881,12 @@ impl<'a> DisplayPropertiesKHR<'a> { self } #[inline] - pub unsafe fn display_name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.display_name) + pub unsafe fn display_name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.display_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.display_name)) + } } #[inline] pub fn physical_dimensions(mut self, physical_dimensions: Extent2D) -> Self { @@ -9172,8 +9188,12 @@ impl<'a> DebugMarkerObjectNameInfoEXT<'a> { self } #[inline] - pub unsafe fn object_name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_object_name) + pub unsafe fn object_name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_object_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_object_name)) + } } } #[repr(C)] @@ -9266,8 +9286,12 @@ impl<'a> DebugMarkerMarkerInfoEXT<'a> { self } #[inline] - pub unsafe fn marker_name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_marker_name) + pub unsafe fn marker_name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_marker_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_marker_name)) + } } #[inline] pub fn color(mut self, color: [f32; 4]) -> Self { @@ -18726,8 +18750,12 @@ impl<'a> DebugUtilsObjectNameInfoEXT<'a> { self } #[inline] - pub unsafe fn object_name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_object_name) + pub unsafe fn object_name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_object_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_object_name)) + } } } #[repr(C)] @@ -18816,8 +18844,12 @@ impl<'a> DebugUtilsLabelEXT<'a> { self } #[inline] - pub unsafe fn label_name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_label_name) + pub unsafe fn label_name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_label_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_label_name)) + } } #[inline] pub fn color(mut self, color: [f32; 4]) -> Self { @@ -18961,8 +18993,12 @@ impl<'a> DebugUtilsMessengerCallbackDataEXT<'a> { self } #[inline] - pub unsafe fn message_id_name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_message_id_name) + pub unsafe fn message_id_name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_message_id_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_message_id_name)) + } } #[inline] pub fn message_id_number(mut self, message_id_number: i32) -> Self { @@ -18975,8 +19011,12 @@ impl<'a> DebugUtilsMessengerCallbackDataEXT<'a> { self } #[inline] - pub unsafe fn message_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_message) + pub unsafe fn message_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_message.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_message)) + } } #[inline] pub fn queue_labels(mut self, queue_labels: &'a [DebugUtilsLabelEXT<'a>]) -> Self { @@ -42334,8 +42374,12 @@ impl<'a> CuFunctionCreateInfoNVX<'a> { self } #[inline] - pub unsafe fn name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_name) + pub unsafe fn name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_name)) + } } } #[repr(C)] @@ -44790,8 +44834,12 @@ impl<'a> CudaFunctionCreateInfoNV<'a> { self } #[inline] - pub unsafe fn name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_name) + pub unsafe fn name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_name)) + } } } #[repr(C)] @@ -51450,8 +51498,12 @@ impl<'a> ShaderCreateInfoEXT<'a> { self } #[inline] - pub unsafe fn name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_name) + pub unsafe fn name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_name)) + } } #[inline] pub fn set_layouts(mut self, set_layouts: &'a [DescriptorSetLayout]) -> Self { @@ -52262,8 +52314,12 @@ impl<'a> PipelineShaderStageNodeCreateInfoAMDX<'a> { self } #[inline] - pub unsafe fn name_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.p_name) + pub unsafe fn name_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.p_name.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.p_name)) + } } #[inline] pub fn index(mut self, index: u32) -> Self { diff --git a/generator/src/lib.rs b/generator/src/lib.rs index 0ea1b28..3717e19 100644 --- a/generator/src/lib.rs +++ b/generator/src/lib.rs @@ -838,6 +838,8 @@ impl FieldExt for vkxml::Field { fn is_pointer_to_static_sized_array(&self) -> bool { matches!(self.array, Some(vkxml::ArrayType::Dynamic)) + // TODO: This should not be hardcoded to one field name, but recognize this pattern somehow + // (by checking if the len field is an expression consisting of purely constants) && self.name.as_deref() == Some("pVersionData") } } @@ -2009,8 +2011,12 @@ fn derive_getters_and_setters( } #deprecated #[inline] - pub unsafe fn #param_ident_as_c_str(&self) -> &core::ffi::CStr { - core::ffi::CStr::from_ptr(self.#param_ident) + pub unsafe fn #param_ident_as_c_str(&self) -> Option<&core::ffi::CStr> { + if self.#param_ident.is_null() { + None + } else { + Some(core::ffi::CStr::from_ptr(self.#param_ident)) + } } }); } else if is_static_array(field) { @@ -2047,8 +2053,9 @@ fn derive_getters_and_setters( let mut slice_param_ty_tokens = field.safe_type_tokens(quote!('a), type_lifetime.clone(), None); - // vkxml wrongly annotates static arrays with a len= field is dynamic. These fields have a static length, - // and a runtime field to describe the actual number of valid items in this static array. + // vkxml considers static arrays with len= to be Dynamic (which they are to some + // extent). These fields have a static upper-bound length, and a runtime field to + // describe the actual number of valid items in this static array. if is_static_array(field) { let array_size_ident = format_ident!("{}", array_size.to_snake_case()); let param_ident_short_as_slice = format_ident!("{}_as_slice", param_ident_short);