layers: Handle partial bound image/buffer

spencer-lunarg · spencer-lunarg · commit 185128d63660 · 2024-06-24T11:12:34.000-05:00
diff --git a/layers/best_practices/best_practices_validation.h b/layers/best_practices/best_practices_validation.h
@@ -742,6 +742,11 @@ class BestPractices : public ValidationStateTracker {
 
     void ManualPostCallRecordEndCommandBuffer(VkCommandBuffer commandBuffer, const RecordObject& record_obj);
 
+    void ManualPostCallRecordBindBufferMemory2(VkDevice device, uint32_t bindInfoCount, const VkBindBufferMemoryInfo* pBindInfos,
+                                               const RecordObject& record_obj);
+    void ManualPostCallRecordBindImageMemory2(VkDevice device, uint32_t bindInfoCount, const VkBindImageMemoryInfo* pBindInfos,
+                                              const RecordObject& record_obj);
+
     void ManualPostCallRecordCreateComputePipelines(VkDevice device, VkPipelineCache pipelineCache, uint32_t createInfoCount,
                                                     const VkComputePipelineCreateInfo* pCreateInfos,
                                                     const VkAllocationCallbacks* pAllocator, VkPipeline* pPipelines,
@@ -834,24 +839,23 @@ class BestPractices : public ValidationStateTracker {
 // Include code-generated functions
 #include "generated/best_practices.h"
   protected:
-    std::shared_ptr<vvl::CommandBuffer> CreateCmdBufferState(VkCommandBuffer cb, const VkCommandBufferAllocateInfo* create_info,
+    std::shared_ptr<vvl::CommandBuffer> CreateCmdBufferState(VkCommandBuffer handle, const VkCommandBufferAllocateInfo* create_info,
                                                              const vvl::CommandPool* pool) final;
 
-    std::shared_ptr<vvl::Swapchain> CreateSwapchainState(const VkSwapchainCreateInfoKHR* create_info,
-                                                         VkSwapchainKHR swapchain) final;
+    std::shared_ptr<vvl::Swapchain> CreateSwapchainState(const VkSwapchainCreateInfoKHR* create_info, VkSwapchainKHR handle) final;
 
-    std::shared_ptr<vvl::PhysicalDevice> CreatePhysicalDeviceState(VkPhysicalDevice phys_dev) final;
+    std::shared_ptr<vvl::PhysicalDevice> CreatePhysicalDeviceState(VkPhysicalDevice handle) final;
 
-    std::shared_ptr<vvl::Image> CreateImageState(VkImage img, const VkImageCreateInfo* pCreateInfo,
+    std::shared_ptr<vvl::Image> CreateImageState(VkImage handle, const VkImageCreateInfo* pCreateInfo,
                                                  VkFormatFeatureFlags2KHR features) final;
 
-    std::shared_ptr<vvl::Image> CreateImageState(VkImage img, const VkImageCreateInfo* pCreateInfo, VkSwapchainKHR swapchain,
+    std::shared_ptr<vvl::Image> CreateImageState(VkImage handle, const VkImageCreateInfo* pCreateInfo, VkSwapchainKHR swapchain,
                                                  uint32_t swapchain_index, VkFormatFeatureFlags2KHR features) final;
 
-    std::shared_ptr<vvl::DescriptorPool> CreateDescriptorPoolState(VkDescriptorPool pool,
+    std::shared_ptr<vvl::DescriptorPool> CreateDescriptorPoolState(VkDescriptorPool handle,
                                                                    const VkDescriptorPoolCreateInfo* pCreateInfo) final;
 
-    std::shared_ptr<vvl::DeviceMemory> CreateDeviceMemoryState(VkDeviceMemory mem, const VkMemoryAllocateInfo* p_alloc_info,
+    std::shared_ptr<vvl::DeviceMemory> CreateDeviceMemoryState(VkDeviceMemory handle, const VkMemoryAllocateInfo* p_alloc_info,
                                                                uint64_t fake_address, const VkMemoryType& memory_type,
                                                                const VkMemoryHeap& memory_heap,
                                                                std::optional<vvl::DedicatedBinding>&& dedicated_binding,
diff --git a/layers/best_practices/bp_device_memory.cpp b/layers/best_practices/bp_device_memory.cpp
@@ -311,3 +311,26 @@ std::shared_ptr<vvl::DeviceMemory> BestPractices::CreateDeviceMemoryState(
     return std::static_pointer_cast<vvl::DeviceMemory>(std::make_shared<bp_state::DeviceMemory>(
         handle, pAllocateInfo, fake_address, memory_type, memory_heap, std::move(dedicated_binding), physical_device_count));
 }
+
+void BestPractices::ManualPostCallRecordBindBufferMemory2(VkDevice device, uint32_t bindInfoCount,
+                                                          const VkBindBufferMemoryInfo* pBindInfos,
+                                                          const RecordObject& record_obj) {
+    if (record_obj.result != VK_SUCCESS && bindInfoCount > 1) {
+        // Details of check found in https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/5527
+        LogWarning(
+            "BestPractices-Partial-Bound-Buffer", device, record_obj.location,
+            "all buffer are now in an indeterminate state because the call failed to return VK_SUCCESS. The best action to take "
+            "is to destroy the buffers instead of trying to rebind");
+    }
+}
+
+void BestPractices::ManualPostCallRecordBindImageMemory2(VkDevice device, uint32_t bindInfoCount,
+                                                         const VkBindImageMemoryInfo* pBindInfos, const RecordObject& record_obj) {
+    if (record_obj.result != VK_SUCCESS && bindInfoCount > 1) {
+        // Details of check found in https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/5527
+        LogWarning(
+            "BestPractices-Partial-Bound-Image", device, record_obj.location,
+            "all image are now in an indeterminate state because the call failed to return VK_SUCCESS. The best action to take is "
+            "to destroy the images instead of trying to rebind");
+    }
+}
diff --git a/layers/core_checks/cc_device_memory.cpp b/layers/core_checks/cc_device_memory.cpp
@@ -288,19 +288,17 @@ bool CoreChecks::ValidateSetMemBinding(const vvl::DeviceMemory &memory_state, co
 
     const bool bind_2 = (loc.function != Func::vkBindBufferMemory) && (loc.function != Func::vkBindImageMemory);
     auto typed_handle = mem_binding.Handle();
+    const bool is_buffer = typed_handle.type == kVulkanObjectTypeBuffer;
 
     if (mem_binding.sparse) {
-        const char *vuid = nullptr;
-        const char *handle_type = nullptr;
-        if (typed_handle.type == kVulkanObjectTypeBuffer) {
-            handle_type = "BUFFER";
+        const char *vuid = kVUIDUndefined;
+        const char *handle_type = is_buffer ? "BUFFER" : "IMAGE";
+        if (is_buffer) {
             vuid = bind_2 ? "VUID-VkBindBufferMemoryInfo-buffer-01030" : "VUID-vkBindBufferMemory-buffer-01030";
-        } else if (typed_handle.type == kVulkanObjectTypeImage) {
-            handle_type = "IMAGE";
-            vuid = bind_2 ? "VUID-VkBindImageMemoryInfo-image-01045" : "VUID-vkBindImageMemory-image-01045";
         } else {
-            assert(false);  // Unsupported object type
+            vuid = bind_2 ? "VUID-VkBindImageMemoryInfo-image-01045" : "VUID-vkBindImageMemory-image-01045";
         }
+
         const LogObjectList objlist(memory_state.Handle(), typed_handle);
         skip |= LogError(vuid, objlist, loc,
                          "attempting to bind %s to %s which was created with sparse memory flags "
@@ -309,19 +307,30 @@ bool CoreChecks::ValidateSetMemBinding(const vvl::DeviceMemory &memory_state, co
     }
 
     const auto *prev_binding = mem_binding.MemState();
-    if (prev_binding) {
-        const char *vuid = nullptr;
-        if (typed_handle.type == kVulkanObjectTypeBuffer) {
+    if (prev_binding || mem_binding.partial_bound) {
+        const char *vuid = kVUIDUndefined;
+        if (is_buffer) {
             vuid = bind_2 ? "VUID-VkBindBufferMemoryInfo-buffer-07459" : "VUID-vkBindBufferMemory-buffer-07459";
-        } else if (typed_handle.type == kVulkanObjectTypeImage) {
+        } else {
             vuid = bind_2 ? "VUID-VkBindImageMemoryInfo-image-07460" : "VUID-vkBindImageMemory-image-07460";
+        }
+
+        if (mem_binding.partial_bound) {
+            Func bind_call = is_buffer ? Func::vkBindBufferMemory2 : Func::vkBindImageMemory2;
+            const char *handle_type = is_buffer ? "buffer" : "image";
+            const LogObjectList objlist(memory_state.Handle(), typed_handle);
+            skip |= LogError(
+                vuid, objlist, loc,
+                "attempting to bind %s to %s which is in an indeterminate (possibly bound) state. A previous call to %s failed and "
+                "we have to assume the %s was bound (but best advise is to handle the case and recreate the %s).",
+                FormatHandle(memory_state.Handle()).c_str(), FormatHandle(typed_handle).c_str(), String(bind_call), handle_type,
+                handle_type);
         } else {
-            assert(false);  // Unsupported object type
+            const LogObjectList objlist(memory_state.Handle(), typed_handle, prev_binding->Handle());
+            skip |= LogError(vuid, objlist, loc, "attempting to bind %s to %s which has already been bound to %s.",
+                             FormatHandle(memory_state.Handle()).c_str(), FormatHandle(typed_handle).c_str(),
+                             FormatHandle(prev_binding->Handle()).c_str());
         }
-        const LogObjectList objlist(memory_state.Handle(), typed_handle, prev_binding->Handle());
-        skip |= LogError(vuid, objlist, loc, "attempting to bind %s to %s which has already been bound to %s.",
-                         FormatHandle(memory_state.Handle()).c_str(), FormatHandle(typed_handle).c_str(),
-                         FormatHandle(prev_binding->Handle()).c_str());
     }
     return skip;
 }
@@ -1063,7 +1072,6 @@ bool CoreChecks::PreCallValidateBindBufferMemory(VkDevice device, VkBuffer buffe
 bool CoreChecks::PreCallValidateBindBufferMemory2(VkDevice device, uint32_t bindInfoCount, const VkBindBufferMemoryInfo *pBindInfos,
                                                   const ErrorObject &error_obj) const {
     bool skip = false;
-
     for (uint32_t i = 0; i < bindInfoCount; i++) {
         const Location loc = error_obj.location.dot(Field::pBindInfos, i);
         skip |= ValidateBindBufferMemory(pBindInfos[i].buffer, pBindInfos[i].memory, pBindInfos[i].memoryOffset,
@@ -2061,7 +2069,16 @@ bool CoreChecks::PreCallValidateBindImageMemory2(VkDevice device, uint32_t bindI
 
 void CoreChecks::PostCallRecordBindImageMemory2(VkDevice device, uint32_t bindInfoCount, const VkBindImageMemoryInfo *pBindInfos,
                                                 const RecordObject &record_obj) {
-    if (VK_SUCCESS != record_obj.result) return;
+    if (VK_SUCCESS != record_obj.result) {
+        if (bindInfoCount > 1) {
+            for (uint32_t i = 0; i < bindInfoCount; i++) {
+                if (auto image_state = Get<vvl::Image>(pBindInfos[i].image)) {
+                    image_state->partial_bound = true;
+                }
+            }
+        }
+        return;
+    }
     StateTracker::PostCallRecordBindImageMemory2(device, bindInfoCount, pBindInfos, record_obj);
 
     for (uint32_t i = 0; i < bindInfoCount; i++) {
diff --git a/layers/state_tracker/device_memory_state.h b/layers/state_tracker/device_memory_state.h
@@ -314,6 +314,10 @@ class Bindable : public StateObject {
     const VkExternalMemoryHandleTypeFlags external_memory_handle_types;
     const bool sparse;       // Is this object being bound with sparse memory or not?
     const bool unprotected;  // can't be used for protected memory
+
+    // For when an array of binds don't succeed and the object is in an indeterminate state
+    bool partial_bound = false;
+
   private:
     mutable bool need_to_recache_invalid_memory_ = false;
     BindableMemoryTracker *memory_tracker_;
diff --git a/layers/state_tracker/state_tracker.cpp b/layers/state_tracker/state_tracker.cpp
@@ -1614,6 +1614,17 @@ void ValidationStateTracker::PostCallRecordBindBufferMemory(VkDevice device, VkB
 void ValidationStateTracker::PostCallRecordBindBufferMemory2(VkDevice device, uint32_t bindInfoCount,
                                                              const VkBindBufferMemoryInfo *pBindInfos,
                                                              const RecordObject &record_obj) {
+    if (VK_SUCCESS != record_obj.result) {
+        if (bindInfoCount > 1) {
+            for (uint32_t i = 0; i < bindInfoCount; i++) {
+                if (auto buffer_state = Get<vvl::Buffer>(pBindInfos[i].buffer)) {
+                    buffer_state->partial_bound = true;
+                }
+            }
+        }
+        return;
+    }
+
     for (uint32_t i = 0; i < bindInfoCount; i++) {
         UpdateBindBufferMemoryState(pBindInfos[i].buffer, pBindInfos[i].memory, pBindInfos[i].memoryOffset);
     }
diff --git a/layers/vulkan/generated/best_practices.cpp b/layers/vulkan/generated/best_practices.cpp
@@ -747,6 +747,7 @@ void BestPractices::PostCallRecordResetCommandBuffer(VkCommandBuffer commandBuff
 void BestPractices::PostCallRecordBindBufferMemory2(VkDevice device, uint32_t bindInfoCount,
                                                     const VkBindBufferMemoryInfo* pBindInfos, const RecordObject& record_obj) {
     ValidationStateTracker::PostCallRecordBindBufferMemory2(device, bindInfoCount, pBindInfos, record_obj);
+    ManualPostCallRecordBindBufferMemory2(device, bindInfoCount, pBindInfos, record_obj);
 
     if (record_obj.result < VK_SUCCESS) {
         LogErrorCode(record_obj);
@@ -756,6 +757,7 @@ void BestPractices::PostCallRecordBindBufferMemory2(VkDevice device, uint32_t bi
 void BestPractices::PostCallRecordBindImageMemory2(VkDevice device, uint32_t bindInfoCount, const VkBindImageMemoryInfo* pBindInfos,
                                                    const RecordObject& record_obj) {
     ValidationStateTracker::PostCallRecordBindImageMemory2(device, bindInfoCount, pBindInfos, record_obj);
+    ManualPostCallRecordBindImageMemory2(device, bindInfoCount, pBindInfos, record_obj);
 
     if (record_obj.result < VK_SUCCESS) {
         LogErrorCode(record_obj);
diff --git a/scripts/generators/best_practices_generator.py b/scripts/generators/best_practices_generator.py
@@ -66,6 +66,8 @@ def __init__(self):
             'vkGetPhysicalDeviceSurfaceFormats2KHR',
             'vkGetPhysicalDeviceDisplayPlanePropertiesKHR',
             'vkGetSwapchainImagesKHR',
+            'vkBindBufferMemory2',
+            'vkBindImageMemory2',
             # AMD tracked
             'vkCreateComputePipelines',
             'vkCmdPipelineBarrier',
diff --git a/tests/unit/memory.cpp b/tests/unit/memory.cpp
@@ -2298,4 +2298,41 @@ TEST_F(NegativeMemory, BadMemoryBindMemory2) {
     m_errorMonitor->SetDesiredError("VUID-VkBindImageMemoryInfo-pNext-01632");
     vk::BindImageMemory2KHR(device(), 1, &image_bind_info);
     m_errorMonitor->VerifyFound();
+}
+
+// TODO - Need a way to trigger Test ICD to fail call
+TEST_F(NegativeMemory, DISABLED_PartialBoundBuffer) {
+    TEST_DESCRIPTION("https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/5527");
+    AddRequiredExtensions(VK_KHR_BIND_MEMORY_2_EXTENSION_NAME);
+    RETURN_IF_SKIP(Init());
+
+    if (!IsPlatformMockICD()) {
+        GTEST_SKIP() << "Test needs to force a failure";
+    }
+
+    VkBufferCreateInfo buffer_create_info = vku::InitStructHelper();
+    buffer_create_info.size = 1024;
+    buffer_create_info.usage = VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT;
+    vkt::Buffer buffer_0(*m_device, buffer_create_info, vkt::no_mem);
+    vkt::Buffer buffer_1(*m_device, buffer_create_info, vkt::no_mem);
+
+    VkMemoryAllocateInfo alloc_info = vkt::DeviceMemory::get_resource_alloc_info(*m_device, buffer_0.memory_requirements(), 0);
+    vkt::DeviceMemory buffer_memory(*m_device, alloc_info);
+
+    VkBindBufferMemoryInfo bind_buffer_infos[2];
+    bind_buffer_infos[0] = vku::InitStructHelper();
+    bind_buffer_infos[0].buffer = buffer_0.handle();
+    bind_buffer_infos[0].memory = buffer_memory.handle();
+    bind_buffer_infos[1] = vku::InitStructHelper();
+    bind_buffer_infos[1].buffer = buffer_1.handle();
+    bind_buffer_infos[1].memory = buffer_memory.handle();
+
+    VkResult result = vk::BindBufferMemory2KHR(device(), 2, bind_buffer_infos);
+    if (result == VK_SUCCESS) {
+        GTEST_SKIP() << "Test needs a to fail call";
+    }
+
+    m_errorMonitor->SetDesiredError("VUID-vkBindBufferMemory-buffer-07459");
+    vk::BindBufferMemory(device(), buffer_0, buffer_memory, 0);
+    m_errorMonitor->VerifyFound();
 }
