layers: Unify invalid command buffer handling

Created a single vector to track any cmd buffer dependencies that are destroyed
and thereby cause the cmd buffer to become invalid. Currently just tracking
the previous object types of descriptor sets and framebuffers, but will update
the tracking to include all cmd buffer dependencies in future commits.

4 files changed, 41 insertions, 72 deletions

diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp
index 6100e005..e8d8e748 100644
--- a/layers/core_validation.cpp
+++ b/layers/core_validation.cpp
@@ -607,6 +607,10 @@ static const char *object_type_to_string(VkDebugReportObjectTypeEXT type) {
         return "buffer";
     case VK_DEBUG_REPORT_OBJECT_TYPE_SWAPCHAIN_KHR_EXT:
         return "swapchain";
+    case VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT:
+        return "descriptor set";
+    case VK_DEBUG_REPORT_OBJECT_TYPE_FRAMEBUFFER_EXT:
+        return "buffer";
     default:
         return "unknown";
     }
@@ -3746,9 +3750,7 @@ static void resetCB(layer_data *dev_data, const VkCommandBuffer cb) {
         pCB->activeRenderPass = nullptr;
         pCB->activeSubpassContents = VK_SUBPASS_CONTENTS_INLINE;
         pCB->activeSubpass = 0;
-        pCB->destroyedSets.clear();
-        pCB->updatedSets.clear();
-        pCB->destroyedFramebuffers.clear();
+        pCB->broken_bindings.clear();
         pCB->waitedEvents.clear();
         pCB->events.clear();
         pCB->writeEventsBeforeWait.clear();
@@ -4438,58 +4440,18 @@ static bool validateCommandBufferState(layer_data *dev_data, GLOBAL_CB_NODE *pCB
     if (CB_RECORDED != pCB->state) {
         if (CB_INVALID == pCB->state) {
             // Inform app of reason CB invalid
-            bool causeReported = false;
-            if (!pCB->destroyedSets.empty()) {
-                std::stringstream set_string;
-                for (auto set : pCB->destroyedSets)
-                    set_string << " " << set;
-
-                skipCall |=
-                    log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT,
-                            (uint64_t)(pCB->commandBuffer), __LINE__, DRAWSTATE_INVALID_COMMAND_BUFFER, "DS",
-                            "You are submitting command buffer 0x%" PRIxLEAST64
-                            " that is invalid because it had the following bound descriptor set(s) destroyed: %s",
-                            (uint64_t)(pCB->commandBuffer), set_string.str().c_str());
-                causeReported = true;
-            }
-            if (!pCB->updatedSets.empty()) {
-                std::stringstream set_string;
-                for (auto set : pCB->updatedSets)
-                    set_string << " " << set;
-
-                skipCall |=
-                    log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT,
-                            (uint64_t)(pCB->commandBuffer), __LINE__, DRAWSTATE_INVALID_COMMAND_BUFFER, "DS",
-                            "You are submitting command buffer 0x%" PRIxLEAST64
-                            " that is invalid because it had the following bound descriptor set(s) updated: %s",
-                            (uint64_t)(pCB->commandBuffer), set_string.str().c_str());
-                causeReported = true;
-            }
-            if (!pCB->destroyedFramebuffers.empty()) {
-                std::stringstream fb_string;
-                for (auto fb : pCB->destroyedFramebuffers)
-                    fb_string << " " << fb;
+            for (auto obj : pCB->broken_bindings) {
+                const char *type_str = object_type_to_string(obj.type);
+                // Descriptor sets are a special case that can be either destroyed or updated to invalidated a CB
+                const char *cause_str =
+                    (obj.type == VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT) ? "destroyed or updated" : "destroyed";
 
                 skipCall |=
                     log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT,
                             reinterpret_cast<uint64_t &>(pCB->commandBuffer), __LINE__, DRAWSTATE_INVALID_COMMAND_BUFFER, "DS",
-                            "You are submitting command buffer 0x%" PRIxLEAST64 " that is invalid because it had the following "
-                            "referenced framebuffers destroyed: %s",
-                            reinterpret_cast<uint64_t &>(pCB->commandBuffer), fb_string.str().c_str());
-                causeReported = true;
-            }
-            // TODO : This is defensive programming to make sure an error is
-            //  flagged if we hit this INVALID cmd buffer case and none of the
-            //  above cases are hit. As the number of INVALID cases grows, this
-            //  code should be updated to seemlessly handle all the cases.
-            if (!causeReported) {
-                skipCall |= log_msg(
-                    dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_COMMAND_BUFFER_EXT,
-                    reinterpret_cast<uint64_t &>(pCB->commandBuffer), __LINE__, DRAWSTATE_INVALID_COMMAND_BUFFER, "DS",
-                    "You are submitting command buffer 0x%" PRIxLEAST64 " that is invalid due to an unknown cause. Validation "
-                    "should "
-                    "be improved to report the exact cause.",
-                    reinterpret_cast<uint64_t &>(pCB->commandBuffer));
+                            "You are submitting command buffer 0x%" PRIxLEAST64 " that is invalid because bound %s 0x%" PRIxLEAST64
+                            " was %s.",
+                            reinterpret_cast<uint64_t &>(pCB->commandBuffer), type_str, obj.handle, cause_str);
             }
         } else { // Flag error for using CB w/o vkEndCommandBuffer() called
             skipCall |=
@@ -5523,17 +5485,22 @@ VKAPI_ATTR VkResult VKAPI_CALL ResetFences(VkDevice device, uint32_t fenceCount,
     return result;
 }
 
+// For given cb_nodes, invalidate them and track object causing invalidation
+void invalidateCommandBuffers(std::unordered_set<GLOBAL_CB_NODE *> cb_nodes, VK_OBJECT obj) {
+    for (auto cb_node : cb_nodes) {
+        cb_node->state = CB_INVALID;
+        cb_node->broken_bindings.push_back(obj);
+    }
+}
+
 VKAPI_ATTR void VKAPI_CALL
 DestroyFramebuffer(VkDevice device, VkFramebuffer framebuffer, const VkAllocationCallbacks *pAllocator) {
     layer_data *dev_data = get_my_data_ptr(get_dispatch_key(device), layer_data_map);
     std::unique_lock<std::mutex> lock(global_lock);
     auto fb_node = getFramebuffer(dev_data, framebuffer);
     if (fb_node) {
-        for (auto cb_node : fb_node->cb_bindings) {
-            // Set CB as invalid and record destroyed framebuffer
-            cb_node->state = CB_INVALID;
-            cb_node->destroyedFramebuffers.insert(framebuffer);
-        }
+        invalidateCommandBuffers(fb_node->cb_bindings,
+                                 {reinterpret_cast<uint64_t &>(fb_node->framebuffer), VK_DEBUG_REPORT_OBJECT_TYPE_FRAMEBUFFER_EXT});
         dev_data->frameBufferMap.erase(fb_node->framebuffer);
     }
     lock.unlock();
diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h
index 18c86757..036ed1f7 100644
--- a/layers/core_validation_types.h
+++ b/layers/core_validation_types.h
@@ -71,6 +71,12 @@ class BASE_NODE {
     std::unordered_set<GLOBAL_CB_NODE *> cb_bindings;
 };
 
+// Generic wrapper for vulkan objects
+struct VK_OBJECT {
+    uint64_t handle;
+    VkDebugReportObjectTypeEXT type;
+};
+
 struct DESCRIPTOR_POOL_NODE {
     VkDescriptorPool pool;
     uint32_t maxSets;       // Max descriptor sets allowed in this pool
@@ -462,13 +468,9 @@ struct GLOBAL_CB_NODE : public BASE_NODE {
     uint32_t activeSubpass;
     VkFramebuffer activeFramebuffer;
     std::unordered_set<VkFramebuffer> framebuffers;
-    // Track descriptor sets that are destroyed or updated while bound to CB
-    // TODO : These data structures relate to tracking resources that invalidate
-    //  a cmd buffer that references them. Need to unify how we handle these
-    //  cases so we don't have different tracking data for each type.
-    std::unordered_set<cvdescriptorset::DescriptorSet *> destroyedSets;
-    std::unordered_set<cvdescriptorset::DescriptorSet *> updatedSets;
-    std::unordered_set<VkFramebuffer> destroyedFramebuffers;
+    // Unified data struct to track dependencies that have been broken
+    //  These are either destroyed objects, or updated descriptor sets
+    std::vector<VK_OBJECT> broken_bindings;
     std::unordered_set<VkEvent> waitedEvents;
     std::vector<VkEvent> writeEventsBeforeWait;
     std::vector<VkEvent> events;
@@ -519,6 +521,7 @@ SAMPLER_NODE *getSamplerNode(const layer_data *, VkSampler);
 VkImageViewCreateInfo *getImageViewData(const layer_data *, VkImageView);
 VkSwapchainKHR getSwapchainFromImage(const layer_data *, VkImage);
 SWAPCHAIN_NODE *getSwapchainNode(const layer_data *, VkSwapchainKHR);
+void invalidateCommandBuffers(std::unordered_set<GLOBAL_CB_NODE *>, VK_OBJECT);
 }
 
 #endif // CORE_VALIDATION_TYPES_H_
diff --git a/layers/descriptor_sets.cpp b/layers/descriptor_sets.cpp
index 1f72d447..3767f78d 100644
--- a/layers/descriptor_sets.cpp
+++ b/layers/descriptor_sets.cpp
@@ -321,7 +321,7 @@ cvdescriptorset::DescriptorSet::DescriptorSet(const VkDescriptorSet set, const D
 cvdescriptorset::DescriptorSet::~DescriptorSet() {
     InvalidateBoundCmdBuffers();
     // Remove link to any cmd buffers
-    for (auto cb : bound_cmd_buffers_) {
+    for (auto cb : cb_bindings) {
         for (uint32_t i=0; i<VK_PIPELINE_BIND_POINT_RANGE_SIZE; ++i) {
             cb->lastBound[i].uniqueBoundSets.erase(this);
         }
@@ -460,9 +460,8 @@ uint32_t cvdescriptorset::DescriptorSet::GetStorageUpdates(const std::unordered_
 }
 // Set is being deleted or updates so invalidate all bound cmd buffers
 void cvdescriptorset::DescriptorSet::InvalidateBoundCmdBuffers() {
-    for (auto cb_node : bound_cmd_buffers_) {
-        cb_node->state = CB_INVALID;
-    }
+    core_validation::invalidateCommandBuffers(cb_bindings,
+                                              {reinterpret_cast<uint64_t &>(set_), VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT});
 }
 // Perform write update in given update struct
 void cvdescriptorset::DescriptorSet::PerformWriteUpdate(const VkWriteDescriptorSet *update) {
diff --git a/layers/descriptor_sets.h b/layers/descriptor_sets.h
index 92824597..609f5d0a 100644
--- a/layers/descriptor_sets.h
+++ b/layers/descriptor_sets.h
@@ -286,6 +286,7 @@ void PerformAllocateDescriptorSets(const VkDescriptorSetAllocateInfo *, const Vk
 class DescriptorSet : public BASE_NODE {
   public:
     using BASE_NODE::in_use;
+    using BASE_NODE::cb_bindings;
     DescriptorSet(const VkDescriptorSet, const DescriptorSetLayout *, const core_validation::layer_data *);
     ~DescriptorSet();
     // A number of common Get* functions that return data based on layout from which this set was created
@@ -331,11 +332,11 @@ class DescriptorSet : public BASE_NODE {
     const DescriptorSetLayout *GetLayout() const { return p_layout_; };
     VkDescriptorSet GetSet() const { return set_; };
     // Return unordered_set of all command buffers that this set is bound to
-    std::unordered_set<GLOBAL_CB_NODE *> GetBoundCmdBuffers() const { return bound_cmd_buffers_; }
+    std::unordered_set<GLOBAL_CB_NODE *> GetBoundCmdBuffers() const { return cb_bindings; }
     // Bind given cmd_buffer to this descriptor set
-    void BindCommandBuffer(GLOBAL_CB_NODE *cb_node) { bound_cmd_buffers_.insert(cb_node); }
-    // If given cmd_buffer is in the bound_cmd_buffers_ set, remove it
-    void RemoveBoundCommandBuffer(GLOBAL_CB_NODE *cb_node) { bound_cmd_buffers_.erase(cb_node); }
+    void BindCommandBuffer(GLOBAL_CB_NODE *cb_node) { cb_bindings.insert(cb_node); }
+    // If given cmd_buffer is in the cb_bindings set, remove it
+    void RemoveBoundCommandBuffer(GLOBAL_CB_NODE *cb_node) { cb_bindings.erase(cb_node); }
     VkSampler const *GetImmutableSamplerPtrFromBinding(const uint32_t index) const {
         return p_layout_->GetImmutableSamplerPtrFromBinding(index);
     };
@@ -359,7 +360,6 @@ class DescriptorSet : public BASE_NODE {
     bool some_update_; // has any part of the set ever been updated?
     VkDescriptorSet set_;
     const DescriptorSetLayout *p_layout_;
-    std::unordered_set<GLOBAL_CB_NODE *> bound_cmd_buffers_;
     std::vector<std::unique_ptr<Descriptor>> descriptors_;
     // Ptr to device data used for various data look-ups
     const core_validation::layer_data *device_data_;