From 271f8b6fec95e548a17081e3243f779c3021582c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 7 Dec 2020 17:25:18 +0100 Subject: [PATCH] Vk: tighten rules for extension-dependent code paths. To ensure a non-success VkResult is never missed. --- doc/developers.dox | 32 ++++++++++++++++++++++ src/Magnum/Vk/Buffer.cpp | 21 +++++++------- src/Magnum/Vk/Buffer.h | 6 ++-- src/Magnum/Vk/Image.cpp | 21 +++++++------- src/Magnum/Vk/Image.h | 6 ++-- src/Magnum/Vk/Implementation/DeviceState.h | 4 +-- 6 files changed, 62 insertions(+), 28 deletions(-) diff --git a/doc/developers.dox b/doc/developers.dox index 6e8563020..ef123624d 100644 --- a/doc/developers.dox +++ b/doc/developers.dox @@ -778,6 +778,38 @@ In order to remove Vulkan functionality, be sure to touch all places mentioned above, only in inverse --- but usually @ref developers-deprecation "deprecate first", unless it doesn't affect public API at all. +@section developers-vk-extension-dependent Checklist for Vulkan extension-dependent code paths + +Every time an extension-dependent code path is expected to be used several +times (such as render pass creation, but not one-time device property query, +for example), it should be implemented through a `*Implementation*()` function +pointers on a class that needs them: + +- The `*Implementation()` functions should be suffixed depending on version / + extension that adds them (so e.g. `ImplementationDefault`, + `ImplementationKHR`, `Implementation12`) +- The implementation should be `MAGNUM_VK_LOCAL` and @cpp static @ce, taking + the class via @cpp Type& self @ce if necessary. This avoids extra overhead + coming from member function pointers and doesn't require an @cpp #include @ce + of the full type on MSVC in order to avoid ABI issues (as insane as it + sounds, MSVC member function pointer size is different depending if the + type is incomplete or not, causing hard-to-debug crashes). +- Their API should match the latest functionality and doing compatibility + steps for previous versions, not the other way around (so for example + extracting "version 1" structures from "version 2" in order to call the + default functionality) +- The return value of the concrete Vulkan call should be always propagated + upwards with a @cpp return @ce, *even if* the call returns @cpp void @ce, + and the call site should be wrapping it in a + @ref MAGNUM_VK_INTERNAL_ASSERT_SUCCESS(). This is preferred over having the + assert repeated for each code path, as it generates less code. +- @cpp Implementation::InstanceState @ce / @cpp Implementation::DeviceState @ce + then contains the dispatching function pointer, choosing the appropriate + version based on what version and extensions are available. +- Finally, the extension should be implicitly enabled during instance / + device creation as appropriate --- for example, if the extension is core in + 1.2, the extension should be enabled only on 1.1 and below. + @section developers-dependency Checklist for adding, removing or updating a dependency 1. Verify that there are no important clients stuck on the old version with no diff --git a/src/Magnum/Vk/Buffer.cpp b/src/Magnum/Vk/Buffer.cpp index 2acd5377e..9aaa235b6 100644 --- a/src/Magnum/Vk/Buffer.cpp +++ b/src/Magnum/Vk/Buffer.cpp @@ -105,7 +105,7 @@ void Buffer::bindMemory(Memory& memory, const UnsignedLong offset) { info.buffer = _handle; info.memory = memory; info.memoryOffset = offset; - _device->state().bindBufferMemoryImplementation(*_device, 1, &info); + MAGNUM_VK_INTERNAL_ASSERT_SUCCESS(_device->state().bindBufferMemoryImplementation(*_device, 1, &info)); } void Buffer::bindDedicatedMemory(Memory&& memory) { @@ -132,28 +132,29 @@ VkBuffer Buffer::release() { } void Buffer::getMemoryRequirementsImplementationDefault(Device& device, const VkBufferMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements) { - device->GetBufferMemoryRequirements(device, info.buffer, &requirements.memoryRequirements); + return device->GetBufferMemoryRequirements(device, info.buffer, &requirements.memoryRequirements); } void Buffer::getMemoryRequirementsImplementationKHR(Device& device, const VkBufferMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements) { - device->GetBufferMemoryRequirements2KHR(device, &info, &requirements); + return device->GetBufferMemoryRequirements2KHR(device, &info, &requirements); } void Buffer::getMemoryRequirementsImplementation11(Device& device, const VkBufferMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements) { - device->GetBufferMemoryRequirements2(device, &info, &requirements); + return device->GetBufferMemoryRequirements2(device, &info, &requirements); } -void Buffer::bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* const infos) { +VkResult Buffer::bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* const infos) { for(std::size_t i = 0; i != count; ++i) - device->BindBufferMemory(device, infos[i].buffer, infos[i].memory, infos[i].memoryOffset); + if(VkResult result = device->BindBufferMemory(device, infos[i].buffer, infos[i].memory, infos[i].memoryOffset)) return result; + return VK_SUCCESS; } -void Buffer::bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* const infos) { - device->BindBufferMemory2KHR(device, count, infos); +VkResult Buffer::bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* const infos) { + return device->BindBufferMemory2KHR(device, count, infos); } -void Buffer::bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* const infos) { - device->BindBufferMemory2(device, count, infos); +VkResult Buffer::bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* const infos) { + return device->BindBufferMemory2(device, count, infos); } }} diff --git a/src/Magnum/Vk/Buffer.h b/src/Magnum/Vk/Buffer.h index 0177e9c34..e3f71d0d6 100644 --- a/src/Magnum/Vk/Buffer.h +++ b/src/Magnum/Vk/Buffer.h @@ -349,9 +349,9 @@ class MAGNUM_VK_EXPORT Buffer { MAGNUM_VK_LOCAL static void getMemoryRequirementsImplementationKHR(Device& device, const VkBufferMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements); MAGNUM_VK_LOCAL static void getMemoryRequirementsImplementation11(Device& device, const VkBufferMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements); - MAGNUM_VK_LOCAL static void bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* infos); - MAGNUM_VK_LOCAL static void bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* infos); - MAGNUM_VK_LOCAL static void bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* infos); + MAGNUM_VK_LOCAL static VkResult bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* infos); + MAGNUM_VK_LOCAL static VkResult bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* infos); + MAGNUM_VK_LOCAL static VkResult bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindBufferMemoryInfo* infos); /* Can't be a reference because of the NoCreate constructor */ Device* _device; diff --git a/src/Magnum/Vk/Image.cpp b/src/Magnum/Vk/Image.cpp index 4f2bb9411..29e8bdfd4 100644 --- a/src/Magnum/Vk/Image.cpp +++ b/src/Magnum/Vk/Image.cpp @@ -122,7 +122,7 @@ void Image::bindMemory(Memory& memory, const UnsignedLong offset) { info.image = _handle; info.memory = memory; info.memoryOffset = offset; - _device->state().bindImageMemoryImplementation(*_device, 1, &info); + MAGNUM_VK_INTERNAL_ASSERT_SUCCESS(_device->state().bindImageMemoryImplementation(*_device, 1, &info)); } void Image::bindDedicatedMemory(Memory&& memory) { @@ -149,28 +149,29 @@ VkImage Image::release() { } void Image::getMemoryRequirementsImplementationDefault(Device& device, const VkImageMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements) { - device->GetImageMemoryRequirements(device, info.image, &requirements.memoryRequirements); + return device->GetImageMemoryRequirements(device, info.image, &requirements.memoryRequirements); } void Image::getMemoryRequirementsImplementationKHR(Device& device, const VkImageMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements) { - device->GetImageMemoryRequirements2KHR(device, &info, &requirements); + return device->GetImageMemoryRequirements2KHR(device, &info, &requirements); } void Image::getMemoryRequirementsImplementation11(Device& device, const VkImageMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements) { - device->GetImageMemoryRequirements2(device, &info, &requirements); + return device->GetImageMemoryRequirements2(device, &info, &requirements); } -void Image::bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { +VkResult Image::bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { for(std::size_t i = 0; i != count; ++i) - device->BindImageMemory(device, infos[i].image, infos[i].memory, infos[i].memoryOffset); + if(VkResult result = device->BindImageMemory(device, infos[i].image, infos[i].memory, infos[i].memoryOffset)) return result; + return VK_SUCCESS; } -void Image::bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { - device->BindImageMemory2KHR(device, count, infos); +VkResult Image::bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { + return device->BindImageMemory2KHR(device, count, infos); } -void Image::bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { - device->BindImageMemory2(device, count, infos); +VkResult Image::bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* const infos) { + return device->BindImageMemory2(device, count, infos); } }} diff --git a/src/Magnum/Vk/Image.h b/src/Magnum/Vk/Image.h index 3b07a7c7e..2b9a0b4ff 100644 --- a/src/Magnum/Vk/Image.h +++ b/src/Magnum/Vk/Image.h @@ -512,9 +512,9 @@ class MAGNUM_VK_EXPORT Image { MAGNUM_VK_LOCAL static void getMemoryRequirementsImplementationKHR(Device& device, const VkImageMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements); MAGNUM_VK_LOCAL static void getMemoryRequirementsImplementation11(Device& device, const VkImageMemoryRequirementsInfo2& info, VkMemoryRequirements2& requirements); - MAGNUM_VK_LOCAL static void bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); - MAGNUM_VK_LOCAL static void bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); - MAGNUM_VK_LOCAL static void bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); + MAGNUM_VK_LOCAL static VkResult bindMemoryImplementationDefault(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); + MAGNUM_VK_LOCAL static VkResult bindMemoryImplementationKHR(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); + MAGNUM_VK_LOCAL static VkResult bindMemoryImplementation11(Device& device, UnsignedInt count, const VkBindImageMemoryInfo* infos); /* Can't be a reference because of the NoCreate constructor */ Device* _device; diff --git a/src/Magnum/Vk/Implementation/DeviceState.h b/src/Magnum/Vk/Implementation/DeviceState.h index 6b8f84711..473124cc2 100644 --- a/src/Magnum/Vk/Implementation/DeviceState.h +++ b/src/Magnum/Vk/Implementation/DeviceState.h @@ -37,8 +37,8 @@ struct DeviceState { /** @todo put this eventually into a dedicated buffer / image state struct? */ void(*getBufferMemoryRequirementsImplementation)(Device&, const VkBufferMemoryRequirementsInfo2&, VkMemoryRequirements2&); void(*getImageMemoryRequirementsImplementation)(Device&, const VkImageMemoryRequirementsInfo2&, VkMemoryRequirements2&); - void(*bindBufferMemoryImplementation)(Device&, UnsignedInt, const VkBindBufferMemoryInfo*); - void(*bindImageMemoryImplementation)(Device&, UnsignedInt, const VkBindImageMemoryInfo*); + VkResult(*bindBufferMemoryImplementation)(Device&, UnsignedInt, const VkBindBufferMemoryInfo*); + VkResult(*bindImageMemoryImplementation)(Device&, UnsignedInt, const VkBindImageMemoryInfo*); }; }}}