From 16f27024d6e6d001b1c0ad1ab60b37d021ec1a30 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Fri, 7 Oct 2022 16:29:15 -0700 Subject: [PATCH] hal: Introduce `cstr_from_bytes_until_nul` helper function. Remove a handful of `unsafe` blocks from the Vulkan back end. --- CHANGELOG.md | 2 +- wgpu-hal/src/auxil/mod.rs | 21 +++++++++++++++++ wgpu-hal/src/vulkan/adapter.rs | 42 +++++++++++++++++---------------- wgpu-hal/src/vulkan/instance.rs | 22 ++++++++--------- 4 files changed, 55 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1aaa082935..4860574f91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,7 +59,7 @@ Bottom level categories: #### Vulkan -- Don't use a pointer to a local copy of a `PhysicalDeviceDriverProperties` struct after it has gone out of scope. In fact, don't make a local copy at all. By @jimblandy in [#3076](https://github.com/gfx-rs/wgpu/pull/3076). +- Don't use a pointer to a local copy of a `PhysicalDeviceDriverProperties` struct after it has gone out of scope. In fact, don't make a local copy at all. Introduce a helper function for building `CStr`s from C character arrays, and remove some `unsafe` blocks. By @jimblandy in [#3076](https://github.com/gfx-rs/wgpu/pull/3076). ## wgpu-0.14.0 (2022-10-05) diff --git a/wgpu-hal/src/auxil/mod.rs b/wgpu-hal/src/auxil/mod.rs index d8675ac0d0..5898d0df39 100644 --- a/wgpu-hal/src/auxil/mod.rs +++ b/wgpu-hal/src/auxil/mod.rs @@ -115,3 +115,24 @@ impl crate::TextureCopy { self.size = self.size.min(&max_src_size).min(&max_dst_size); } } + +/// Construct a `CStr` from a byte slice, up to the first zero byte. +/// +/// Return a `CStr` extending from the start of `bytes` up to and +/// including the first zero byte. If there is no zero byte in +/// `bytes`, return `None`. +/// +/// This can be removed when `CStr::from_bytes_until_nul` is stabilized. +/// ([#95027](https://github.com/rust-lang/rust/issues/95027)) +#[allow(dead_code)] +pub(crate) fn cstr_from_bytes_until_nul(bytes: &[std::ffi::c_char]) -> Option<&std::ffi::CStr> { + if bytes.contains(&0) { + // Safety for `CStr::from_ptr`: + // - We've ensured that the slice does contain a null terminator. + // - The range is valid to read, because the slice covers it. + // - The memory won't be changed, because the slice borrows it. + unsafe { Some(std::ffi::CStr::from_ptr(bytes.as_ptr())) } + } else { + None + } +} diff --git a/wgpu-hal/src/vulkan/adapter.rs b/wgpu-hal/src/vulkan/adapter.rs index bfb86f914f..0c035f6ae7 100644 --- a/wgpu-hal/src/vulkan/adapter.rs +++ b/wgpu-hal/src/vulkan/adapter.rs @@ -538,9 +538,10 @@ impl PhysicalDeviceCapabilities { } pub fn supports_extension(&self, extension: &CStr) -> bool { + use crate::auxil::cstr_from_bytes_until_nul; self.supported_extensions .iter() - .any(|ep| unsafe { CStr::from_ptr(ep.extension_name.as_ptr()) } == extension) + .any(|ep| cstr_from_bytes_until_nul(&ep.extension_name) == Some(extension)) } /// Map `requested_features` to the list of Vulkan extension strings required to create the logical device. @@ -885,14 +886,15 @@ impl super::Instance { &self, phd: vk::PhysicalDevice, ) -> Option> { + use crate::auxil::cstr_from_bytes_until_nul; use crate::auxil::db; let (phd_capabilities, phd_features) = self.shared.inspect(phd); let info = wgt::AdapterInfo { - name: unsafe { - CStr::from_ptr(phd_capabilities.properties.device_name.as_ptr()) - .to_str() + name: { + cstr_from_bytes_until_nul(&phd_capabilities.properties.device_name) + .and_then(|info| info.to_str().ok()) .unwrap_or("?") .to_owned() }, @@ -906,23 +908,23 @@ impl super::Instance { ash::vk::PhysicalDeviceType::CPU => wgt::DeviceType::Cpu, _ => wgt::DeviceType::Other, }, - driver: unsafe { - let driver_name = if let Some(ref driver) = phd_capabilities.driver { - CStr::from_ptr(driver.driver_name.as_ptr()).to_str().ok() - } else { - None - }; - - driver_name.unwrap_or("?").to_owned() + driver: { + phd_capabilities + .driver + .as_ref() + .and_then(|driver| cstr_from_bytes_until_nul(&driver.driver_name)) + .and_then(|name| name.to_str().ok()) + .unwrap_or("?") + .to_owned() }, - driver_info: unsafe { - let driver_info = if let Some(ref driver) = phd_capabilities.driver { - CStr::from_ptr(driver.driver_info.as_ptr()).to_str().ok() - } else { - None - }; - - driver_info.unwrap_or("?").to_owned() + driver_info: { + phd_capabilities + .driver + .as_ref() + .and_then(|driver| cstr_from_bytes_until_nul(&driver.driver_info)) + .and_then(|name| name.to_str().ok()) + .unwrap_or("?") + .to_owned() }, backend: wgt::Backend::Vulkan, }; diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index 55a8dec818..bb566c178c 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -201,10 +201,9 @@ impl super::Instance { // Only keep available extensions. extensions.retain(|&ext| { - if instance_extensions - .iter() - .any(|inst_ext| unsafe { CStr::from_ptr(inst_ext.extension_name.as_ptr()) == ext }) - { + if instance_extensions.iter().any(|inst_ext| { + crate::auxil::cstr_from_bytes_until_nul(&inst_ext.extension_name) == Some(ext) + }) { true } else { log::info!("Unable to find extension: {}", ext.to_string_lossy()); @@ -483,6 +482,8 @@ impl Drop for super::InstanceShared { impl crate::Instance for super::Instance { unsafe fn init(desc: &crate::InstanceDescriptor) -> Result { + use crate::auxil::cstr_from_bytes_until_nul; + let entry = match ash::Entry::load() { Ok(entry) => entry, Err(err) => { @@ -531,9 +532,9 @@ impl crate::Instance for super::Instance { })?; let nv_optimus_layer = CStr::from_bytes_with_nul(b"VK_LAYER_NV_optimus\0").unwrap(); - let has_nv_optimus = instance_layers - .iter() - .any(|inst_layer| CStr::from_ptr(inst_layer.layer_name.as_ptr()) == nv_optimus_layer); + let has_nv_optimus = instance_layers.iter().any(|inst_layer| { + cstr_from_bytes_until_nul(&inst_layer.layer_name) == Some(nv_optimus_layer) + }); // Check requested layers against the available layers let layers = { @@ -544,10 +545,9 @@ impl crate::Instance for super::Instance { // Only keep available layers. layers.retain(|&layer| { - if instance_layers - .iter() - .any(|inst_layer| CStr::from_ptr(inst_layer.layer_name.as_ptr()) == layer) - { + if instance_layers.iter().any(|inst_layer| { + cstr_from_bytes_until_nul(&inst_layer.layer_name) == Some(layer) + }) { true } else { log::warn!("Unable to find layer: {}", layer.to_string_lossy());