Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from mem::zeroed to MaybeUninit<T> #19

Closed
gwihlidal opened this issue Oct 29, 2019 · 2 comments · Fixed by #39
Closed

Switch from mem::zeroed to MaybeUninit<T> #19

gwihlidal opened this issue Oct 29, 2019 · 2 comments · Fixed by #39

Comments

@gwihlidal
Copy link
Owner

gwihlidal commented Oct 29, 2019

Newer rust compiler now warns about usage of mem::zeroed.

warning: the type `ash::Instance` does not permit zero-initialization
   --> src/lib.rs:263:32
    |
263 |             instance: unsafe { mem::zeroed() },
    |                                ^^^^^^^^^^^^^
    |                                |
    |                                this code causes undefined behavior when executed
    |                                help: use `MaybeUninit<T>` instead
    |
note: Function pointers must be non-null (in this struct field)

This warning is currently silenced in lib.rs:

#![allow(invalid_value)]

It would be good to remove the allow pragma and switch to MaybeUninit.

@aloucks
Copy link
Contributor

aloucks commented Nov 5, 2019

I don't think MaybeUnit will help in the impl for Default. The Device and Instance structs contain function pointers that must be filled with valid values or it's UB.

I think there's a couple of options:

  1. Make the device and instance fields Option<Device> and Option<Instance> and return an error in in Allocator::new if either are None.
  2. Remove the Default implementation for AllocatorCreateInfo.
  3. Create dummy values for device and instance that are technically valid, but will panic if you attempt to use them.

I experimented with #3 and all unit tests are passing. I can open a PR if you want to go this route.

/// Construct `AllocatorCreateInfo` with default values
///
/// Note that the default `device` and `instance` fields are filled with dummy
/// implementations that will panic if used. These fields must be overwritten.
impl Default for AllocatorCreateInfo {
    fn default() -> Self {
        extern "C" fn get_device_proc_addr(
            _: ash::vk::Instance,
            _: *const std::os::raw::c_char,
        ) -> *const std::os::raw::c_void {
            std::ptr::null()
        }
        extern "C" fn get_instance_proc_addr(
            _: ash::vk::Instance,
            name: *const std::os::raw::c_char,
        ) -> *const std::os::raw::c_void {
            get_device_proc_addr as *const _
        }
        let static_fn = ash::vk::StaticFn::load(|_| get_instance_proc_addr as *const _);
        let instance = unsafe { ash::Instance::load(&static_fn, ash::vk::Instance::null()) };
        let device = unsafe { ash::Device::load(&instance.fp_v1_0(), ash::vk::Device::null()) };
        AllocatorCreateInfo {
            physical_device: ash::vk::PhysicalDevice::null(),
            device,
            instance,
            flags: AllocatorCreateFlags::NONE,
            preferred_large_heap_block_size: 0,
            frame_in_use_count: 0,
            heap_size_limits: None,
        }
    }
}

@gwihlidal
Copy link
Owner Author

Sure, that's probably a fine approach to start with for now.

aloucks added a commit to aloucks/vk-mem-rs that referenced this issue Sep 30, 2020
Nightly rust has now turned the mem::zeroed warning into a runtime
panic.

Fixes gwihlidal#19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants