diff --git a/BUILD.gn b/BUILD.gn index 28a2c0fc727..e1c967a9e86 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1,5 +1,5 @@ # Copyright (C) 2018-2021 The ANGLE Project Authors. -# Copyright (C) 2019-2021 LunarG, Inc. +# Copyright (C) 2019-2022 LunarG, Inc. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -82,6 +82,7 @@ config("vulkan_layer_config") { core_validation_sources = [ "layers/android_ndk_types.h", + "layers/base_node.cpp", "layers/base_node.h", "layers/buffer_state.cpp", "layers/buffer_state.h", diff --git a/build-android/cmake/layerlib/CMakeLists.txt b/build-android/cmake/layerlib/CMakeLists.txt index ac41b0e0954..883b24448f7 100644 --- a/build-android/cmake/layerlib/CMakeLists.txt +++ b/build-android/cmake/layerlib/CMakeLists.txt @@ -63,6 +63,7 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DVK_USE_PLATFORM_ANDROID_KHR \ -fvisibility=hidden") add_library(VkLayer_khronos_validation SHARED ${SRC_DIR}/layers/state_tracker.cpp + ${SRC_DIR}/layers/base_node.cpp ${SRC_DIR}/layers/buffer_state.cpp ${SRC_DIR}/layers/cmd_buffer_state.cpp ${SRC_DIR}/layers/device_memory_state.cpp diff --git a/build-android/jni/Android.mk b/build-android/jni/Android.mk index 2e1fb7c4a3c..ce28215df06 100644 --- a/build-android/jni/Android.mk +++ b/build-android/jni/Android.mk @@ -38,6 +38,7 @@ include $(CLEAR_VARS) LOCAL_MODULE := VkLayer_khronos_validation LOCAL_SRC_FILES += $(SRC_DIR)/layers/state_tracker.cpp LOCAL_SRC_FILES += $(SRC_DIR)/layers/device_memory_state.cpp +LOCAL_SRC_FILES += $(SRC_DIR)/layers/base_node.cpp LOCAL_SRC_FILES += $(SRC_DIR)/layers/buffer_state.cpp LOCAL_SRC_FILES += $(SRC_DIR)/layers/cmd_buffer_state.cpp LOCAL_SRC_FILES += $(SRC_DIR)/layers/image_state.cpp diff --git a/layers/CMakeLists.txt b/layers/CMakeLists.txt index e129a9a27a0..2c3338fc1af 100644 --- a/layers/CMakeLists.txt +++ b/layers/CMakeLists.txt @@ -1,6 +1,6 @@ # ~~~ -# Copyright (c) 2014-2021 Valve Corporation -# Copyright (c) 2014-2021 LunarG, Inc. +# Copyright (c) 2014-2022 Valve Corporation +# Copyright (c) 2014-2022 LunarG, Inc. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -162,6 +162,7 @@ set(CORE_VALIDATION_LIBRARY_FILES core_error_location.h core_error_location.cpp base_node.h + base_node.cpp device_memory_state.h device_memory_state.cpp buffer_state.h diff --git a/layers/base_node.cpp b/layers/base_node.cpp new file mode 100644 index 00000000000..f8b8960e625 --- /dev/null +++ b/layers/base_node.cpp @@ -0,0 +1,111 @@ +/* Copyright (c) 2015-2022 The Khronos Group Inc. + * Copyright (c) 2015-2022 Valve Corporation + * Copyright (c) 2015-2022 LunarG, Inc. + * Copyright (C) 2015-2022 Google Inc. + * Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * Author: Courtney Goeltzenleuchter + * Author: Tobin Ehlis + * Author: Chris Forbes + * Author: Mark Lobodzinski + * Author: Dave Houlton + * Author: John Zulauf + * Author: Tobias Hector + * Author: Jeremy Gebben + */ +#include "base_node.h" +#include "vk_layer_utils.h" + +BASE_NODE::~BASE_NODE() { Destroy(); } + +void BASE_NODE::Destroy() { + Invalidate(); + destroyed_ = true; +} + +bool BASE_NODE::InUse() const { + // NOTE: for performance reasons, this method calls up the tree + // with the read lock held. + auto guard = ReadLockTree(); + bool result = false; + for (auto& item : parent_nodes_) { + auto node = item.second.lock(); + if (!node) { + continue; + } + result |= node->InUse(); + if (result) { + break; + } + } + return result; +} + +bool BASE_NODE::AddParent(BASE_NODE *parent_node) { + auto guard = WriteLockTree(); + auto result = parent_nodes_.emplace(parent_node->Handle(), std::weak_ptr(parent_node->shared_from_this())); + return result.second; +} + +void BASE_NODE::RemoveParent(BASE_NODE *parent_node) { + assert(parent_node); + auto guard = WriteLockTree(); + parent_nodes_.erase(parent_node->Handle()); +} + +// copy the current set of parents so that we don't need to hold the lock +// while calling NotifyInvalidate on them, as that would lead to recursive locking. +BASE_NODE::NodeMap BASE_NODE::GetParentsForInvalidate(bool unlink) { + NodeMap result; + if (unlink) { + auto guard = WriteLockTree(); + result = std::move(parent_nodes_); + parent_nodes_.clear(); + } else { + auto guard = ReadLockTree(); + result = parent_nodes_; + } + return result; +} + +BASE_NODE::NodeMap BASE_NODE::ObjectBindings() const { + auto guard = ReadLockTree(); + return parent_nodes_; +} + +void BASE_NODE::Invalidate(bool unlink) { + NodeList empty; + // We do not want to call the virtual method here because any special handling + // in an overriden NotifyInvalidate() is for when a child node has become invalid. + // But calling Invalidate() indicates the current node is invalid. + // Calling the default implementation directly here avoids duplicating it inline. + BASE_NODE::NotifyInvalidate(empty, unlink); +} + +void BASE_NODE::NotifyInvalidate(const NodeList& invalid_nodes, bool unlink) { + auto current_parents = GetParentsForInvalidate(unlink); + if (current_parents.size() == 0) { + return; + } + + NodeList up_nodes = invalid_nodes; + up_nodes.emplace_back(shared_from_this()); + for (auto& item : current_parents) { + auto node = item.second.lock(); + if (node && !node->Destroyed()) { + node->NotifyInvalidate(up_nodes, unlink); + } + } +} diff --git a/layers/base_node.h b/layers/base_node.h index 2149938ae9c..f3acb796b76 100644 --- a/layers/base_node.h +++ b/layers/base_node.h @@ -1,7 +1,7 @@ -/* Copyright (c) 2015-2021 The Khronos Group Inc. - * Copyright (c) 2015-2021 Valve Corporation - * Copyright (c) 2015-2021 LunarG, Inc. - * Copyright (C) 2015-2021 Google Inc. +/* Copyright (c) 2015-2022 The Khronos Group Inc. + * Copyright (c) 2015-2022 Valve Corporation + * Copyright (c) 2015-2022 LunarG, Inc. + * Copyright (C) 2015-2022 Google Inc. * Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -31,6 +31,7 @@ #include "vk_object_types.h" #include "vk_layer_data.h" #include "vk_layer_logging.h" +#include "vk_layer_utils.h" #include @@ -45,82 +46,74 @@ struct hash { }; } // namespace std -class BASE_NODE { +// inheriting from enable_shared_from_this<> adds a method, shared_from_this(), which +// returns a shared_ptr version of the current object. It requires the object to +// be created with std::make_shared<> and it MUST NOT be used from the constructor +class BASE_NODE : public std::enable_shared_from_this { public: - using NodeSet = layer_data::unordered_set; - using NodeList = small_vector; + // Parent nodes are stored as weak_ptrs to avoid cyclic memory dependencies. + // Because weak_ptrs cannot safely be used as hash keys, the parents are stored + // in a map keyed by VulkanTypedHandle. This also allows looking for specific + // parent types without locking every weak_ptr. + using NodeMap = layer_data::unordered_map>; + using NodeList = small_vector, 4, uint32_t>; template BASE_NODE(Handle h, VulkanObjectType t) : handle_(h, t), destroyed_(false) {} - virtual ~BASE_NODE() { Destroy(); } + // because shared_from_this() does not work from the constructor, this 2nd phase + // constructor is where a state object should call AddParent() on its child nodes. + // It is called as part of ValidationStateTracker::Add() + virtual void LinkChildNodes() {} - virtual void Destroy() { - Invalidate(); - destroyed_ = true; - } + virtual ~BASE_NODE(); + + // Because state objects are reference counted, they may outlive the vulkan objects + // they represent. Destroy() is called when the vulkan object is destroyed, so that + // it can be cleaned up before all references are gone. It also triggers notifications + // to parent objects. + virtual void Destroy(); bool Destroyed() const { return destroyed_; } const VulkanTypedHandle &Handle() const { return handle_; } VulkanObjectType Type() const { return handle_.type; } - virtual bool InUse() const { - bool result = false; - for (auto& node: parent_nodes_) { - result |= node->InUse(); - if (result) { - break; - } - } - return result; - } - - virtual bool AddParent(BASE_NODE *parent_node) { - auto result = parent_nodes_.emplace(parent_node); - return result.second; - } - - virtual void RemoveParent(BASE_NODE *parent_node) { - parent_nodes_.erase(parent_node); - } - - void Invalidate(bool unlink = true) { - NodeList invalid_nodes; - invalid_nodes.emplace_back(this); - for (auto& node: parent_nodes_) { - node->NotifyInvalidate(invalid_nodes, unlink); - } - if (unlink) { - parent_nodes_.clear(); - } - } + virtual bool InUse() const; + + virtual bool AddParent(BASE_NODE *parent_node); + virtual void RemoveParent(BASE_NODE *parent_node); + + // Invalidate is called on a state object to inform its parents that it + // is being destroyed (unlink == true) or otherwise becoming invalid (unlink == false) + void Invalidate(bool unlink = true); + + // Helper to let objects examine their immediate parents without holding the tree lock. + NodeMap ObjectBindings() const; + protected: - // NOTE: the entries in invalid_nodes will likely be destroyed & deleted - // after the NotifyInvalidate() calls finish. - virtual void NotifyInvalidate(const NodeList &invalid_nodes, bool unlink) { - if (parent_nodes_.size() == 0) { - return; - } - NodeList up_nodes = invalid_nodes; - up_nodes.emplace_back(this); - for (auto& node: parent_nodes_) { - node->NotifyInvalidate(up_nodes, unlink); - } - if (unlink) { - parent_nodes_.clear(); - } - } + // Called recursively for every parent object of something that has become invalid + virtual void NotifyInvalidate(const NodeList &invalid_nodes, bool unlink); + + // returns a copy of the current set of parents so that they can be walked + // without the tree lock held. If unlink == true, parent_nodes_ is also cleared. + NodeMap GetParentsForInvalidate(bool unlink); VulkanTypedHandle handle_; // Set to true when the API-level object is destroyed, but this object may // hang around until its shared_ptr refcount goes to zero. - bool destroyed_; + std::atomic destroyed_; + + private: + ReadLockGuard ReadLockTree() const { return ReadLockGuard(tree_lock_); } + WriteLockGuard WriteLockTree() { return WriteLockGuard(tree_lock_); } // Set of immediate parent nodes for this object. For an in-use object, the // parent nodes should form a tree with the root being a command buffer. - NodeSet parent_nodes_; + NodeMap parent_nodes_; + // Lock guarding parent_nodes_, this lock MUST NOT be used for other purposes. + mutable ReadWriteLock tree_lock_; }; class REFCOUNTED_NODE : public BASE_NODE { diff --git a/layers/best_practices_utils.cpp b/layers/best_practices_utils.cpp index 71a1811d98b..36811d3da25 100644 --- a/layers/best_practices_utils.cpp +++ b/layers/best_practices_utils.cpp @@ -1,6 +1,6 @@ -/* Copyright (c) 2015-2021 The Khronos Group Inc. - * Copyright (c) 2015-2021 Valve Corporation - * Copyright (c) 2015-2021 LunarG, Inc. +/* Copyright (c) 2015-2022 The Khronos Group Inc. + * Copyright (c) 2015-2022 Valve Corporation + * Copyright (c) 2015-2022 LunarG, Inc. * Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -705,8 +705,8 @@ bool BestPractices::PreCallValidateFreeMemory(VkDevice device, VkDeviceMemory me const auto mem_info = Get(memory); - for (const auto& node: mem_info->ObjectBindings()) { - const auto& obj = node->Handle(); + for (const auto& item : mem_info->ObjectBindings()) { + const auto& obj = item.first; LogObjectList objlist(device); objlist.add(obj); objlist.add(mem_info->mem()); diff --git a/layers/buffer_state.h b/layers/buffer_state.h index f313c1ad273..ef5a5bc256a 100644 --- a/layers/buffer_state.h +++ b/layers/buffer_state.h @@ -54,10 +54,11 @@ class BUFFER_VIEW_STATE : public BASE_NODE { BUFFER_VIEW_STATE(const std::shared_ptr &bf, VkBufferView bv, const VkBufferViewCreateInfo *ci, VkFormatFeatureFlags2KHR ff) - : BASE_NODE(bv, kVulkanObjectTypeBufferView), create_info(*ci), buffer_state(bf), format_features(ff) { - if (buffer_state) { - buffer_state->AddParent(this); - } + : BASE_NODE(bv, kVulkanObjectTypeBufferView), create_info(*ci), buffer_state(bf), format_features(ff) {} + + void LinkChildNodes() override { + // Connect child node(s), which cannot safely be done in the constructor. + buffer_state->AddParent(this); } virtual ~BUFFER_VIEW_STATE() { if (!Destroyed()) { diff --git a/layers/cmd_buffer_state.cpp b/layers/cmd_buffer_state.cpp index e12e6dd4451..65c2f755771 100644 --- a/layers/cmd_buffer_state.cpp +++ b/layers/cmd_buffer_state.cpp @@ -252,14 +252,14 @@ const IMAGE_VIEW_STATE *CMD_BUFFER_STATE::GetActiveAttachmentImageViewState(uint return active_attachments->at(index); } -void CMD_BUFFER_STATE::AddChild(BASE_NODE *child_node) { +void CMD_BUFFER_STATE::AddChild(std::shared_ptr &child_node) { assert(child_node); if (child_node->AddParent(this)) { object_bindings.insert(child_node); } } -void CMD_BUFFER_STATE::RemoveChild(BASE_NODE *child_node) { +void CMD_BUFFER_STATE::RemoveChild(std::shared_ptr &child_node) { assert(child_node); child_node->RemoveParent(this); object_bindings.erase(child_node); @@ -357,12 +357,6 @@ void CMD_BUFFER_STATE::Reset() { transform_feedback_active = false; - // Remove object bindings - for (auto *base_obj : object_bindings) { - RemoveChild(base_obj); - } - object_bindings.clear(); - // Clean up the label data ResetCmdDebugUtilsLabel(dev_data->report_data, commandBuffer()); @@ -458,20 +452,20 @@ void CMD_BUFFER_STATE::NotifyInvalidate(const BASE_NODE::NodeList &invalid_nodes } assert(!invalid_nodes.empty()); LogObjectList log_list; - for (auto *obj : invalid_nodes) { + for (auto &obj : invalid_nodes) { log_list.object_list.emplace_back(obj->Handle()); } broken_bindings.emplace(invalid_nodes[0]->Handle(), log_list); if (unlink) { - for (auto *obj : invalid_nodes) { + for (auto &obj : invalid_nodes) { object_bindings.erase(obj); switch (obj->Type()) { case kVulkanObjectTypeCommandBuffer: - linkedCommandBuffers.erase(static_cast(obj)); + linkedCommandBuffers.erase(static_cast(obj.get())); break; case kVulkanObjectTypeImage: - image_layout_map.erase(static_cast(obj)); + image_layout_map.erase(static_cast(obj.get())); break; default: break; @@ -626,7 +620,7 @@ void CMD_BUFFER_STATE::BeginRenderPass(CMD_TYPE cmd_type, const VkRenderPassBegi // Connect this RP to cmdBuffer if (!dev_data->disabled[command_buffer_state] && activeRenderPass) { - AddChild(activeRenderPass.get()); + AddChild(activeRenderPass); } auto chained_device_group_struct = LvlFindInChain(pRenderPassBegin->pNext); @@ -652,7 +646,7 @@ void CMD_BUFFER_STATE::BeginRenderPass(CMD_TYPE cmd_type, const VkRenderPassBegi UpdateAttachmentsView(pRenderPassBegin); // Connect this framebuffer and its children to this cmdBuffer - AddChild(activeFramebuffer.get()); + AddChild(activeFramebuffer); } } @@ -783,7 +777,7 @@ void CMD_BUFFER_STATE::Begin(const VkCommandBufferBeginInfo *pBeginInfo) { // Connect this framebuffer and its children to this cmdBuffer if (!dev_data->disabled[command_buffer_state]) { - AddChild(activeFramebuffer.get()); + AddChild(activeFramebuffer); } } } @@ -853,7 +847,7 @@ void CMD_BUFFER_STATE::ExecuteCommands(uint32_t commandBuffersCount, const VkCom sub_cb_state->primaryCommandBuffer = commandBuffer(); linkedCommandBuffers.insert(sub_cb_state.get()); - AddChild(sub_cb_state.get()); + AddChild(sub_cb_state); for (auto &function : sub_cb_state->queryUpdates) { queryUpdates.push_back(function); } @@ -1178,10 +1172,10 @@ void CMD_BUFFER_STATE::RecordStateCmd(CMD_TYPE cmd_type, CBStatusFlags state_bit void CMD_BUFFER_STATE::RecordTransferCmd(CMD_TYPE cmd_type, std::shared_ptr &&buf1, std::shared_ptr &&buf2) { RecordCmd(cmd_type); if (buf1) { - AddChild(buf1.get()); + AddChild(buf1); } if (buf2) { - AddChild(buf2.get()); + AddChild(buf2); } } @@ -1195,7 +1189,7 @@ void CMD_BUFFER_STATE::RecordSetEvent(CMD_TYPE cmd_type, VkEvent event, VkPipeli if (!dev_data->disabled[command_buffer_state]) { auto event_state = dev_data->Get(event); if (event_state) { - AddChild(event_state.get()); + AddChild(event_state); } } events.push_back(event); @@ -1213,7 +1207,7 @@ void CMD_BUFFER_STATE::RecordResetEvent(CMD_TYPE cmd_type, VkEvent event, VkPipe if (!dev_data->disabled[command_buffer_state]) { auto event_state = dev_data->Get(event); if (event_state) { - AddChild(event_state.get()); + AddChild(event_state); } } events.push_back(event); @@ -1232,7 +1226,7 @@ void CMD_BUFFER_STATE::RecordWaitEvents(CMD_TYPE cmd_type, uint32_t eventCount, if (!dev_data->disabled[command_buffer_state]) { auto event_state = dev_data->Get(pEvents[i]); if (event_state) { - AddChild(event_state.get()); + AddChild(event_state); } } waitedEvents.insert(pEvents[i]); @@ -1248,13 +1242,13 @@ void CMD_BUFFER_STATE::RecordBarriers(uint32_t memoryBarrierCount, const VkMemor for (uint32_t i = 0; i < bufferMemoryBarrierCount; i++) { auto buffer_state = dev_data->Get(pBufferMemoryBarriers[i].buffer); if (buffer_state) { - AddChild(buffer_state.get()); + AddChild(buffer_state); } } for (uint32_t i = 0; i < imageMemoryBarrierCount; i++) { auto image_state = dev_data->Get(pImageMemoryBarriers[i].image); if (image_state) { - AddChild(image_state.get()); + AddChild(image_state); } } } @@ -1265,13 +1259,13 @@ void CMD_BUFFER_STATE::RecordBarriers(const VkDependencyInfoKHR &dep_info) { for (uint32_t i = 0; i < dep_info.bufferMemoryBarrierCount; i++) { auto buffer_state = dev_data->Get(dep_info.pBufferMemoryBarriers[i].buffer); if (buffer_state) { - AddChild(buffer_state.get()); + AddChild(buffer_state); } } for (uint32_t i = 0; i < dep_info.imageMemoryBarrierCount; i++) { auto image_state = dev_data->Get(dep_info.pImageMemoryBarriers[i].image); if (image_state) { - AddChild(image_state.get()); + AddChild(image_state); } } } @@ -1283,7 +1277,7 @@ void CMD_BUFFER_STATE::RecordWriteTimestamp(CMD_TYPE cmd_type, VkPipelineStageFl if (!dev_data->disabled[command_buffer_state]) { auto pool_state = dev_data->Get(queryPool); - AddChild(pool_state.get()); + AddChild(pool_state); } QueryObject query = {queryPool, slot}; EndQuery(query); diff --git a/layers/cmd_buffer_state.h b/layers/cmd_buffer_state.h index a89ee3a08ba..4589c7e5c87 100644 --- a/layers/cmd_buffer_state.h +++ b/layers/cmd_buffer_state.h @@ -258,7 +258,7 @@ class CMD_BUFFER_STATE : public REFCOUNTED_NODE { layer_data::unordered_set> framebuffers; // Unified data structs to track objects bound to this command buffer as well as object // dependencies that have been broken : either destroyed objects, or updated descriptor sets - BASE_NODE::NodeSet object_bindings; + layer_data::unordered_set> object_bindings; layer_data::unordered_map broken_bindings; QFOTransferBarrierSets qfo_transfer_buffer_barriers; @@ -331,14 +331,19 @@ class CMD_BUFFER_STATE : public REFCOUNTED_NODE { IMAGE_VIEW_STATE *GetActiveAttachmentImageViewState(uint32_t index); const IMAGE_VIEW_STATE *GetActiveAttachmentImageViewState(uint32_t index) const; - void AddChild(BASE_NODE *child_node); - + void AddChild(std::shared_ptr &base_node); template void AddChild(std::shared_ptr &child_node) { - AddChild(child_node.get()); + auto base = std::static_pointer_cast(child_node); + AddChild(base); } - void RemoveChild(BASE_NODE *child_node); + void RemoveChild(std::shared_ptr &base_node); + template + void RemoveChild(std::shared_ptr &child_node) { + auto base = std::static_pointer_cast(child_node); + RemoveChild(base); + } virtual void Reset(); diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index d7aa21f2421..05552ba70ad 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -3359,10 +3359,10 @@ bool CoreChecks::ValidateQueueFamilyIndices(const Location &loc, const CMD_BUFFE } // Ensure that any bound images or buffers created with SHARING_MODE_CONCURRENT have access to the current queue family - for (const auto *base_node : pCB->object_bindings) { + for (const auto &base_node : pCB->object_bindings) { switch (base_node->Type()) { case kVulkanObjectTypeImage: { - auto image_state = static_cast(base_node); + auto image_state = static_cast(base_node.get()); if (image_state && image_state->createInfo.sharingMode == VK_SHARING_MODE_CONCURRENT) { skip |= ValidImageBufferQueue(pCB, image_state->Handle(), queue_state->queueFamilyIndex, image_state->createInfo.queueFamilyIndexCount, @@ -3371,7 +3371,7 @@ bool CoreChecks::ValidateQueueFamilyIndices(const Location &loc, const CMD_BUFFE break; } case kVulkanObjectTypeBuffer: { - auto buffer_state = static_cast(base_node); + auto buffer_state = static_cast(base_node.get()); if (buffer_state && buffer_state->createInfo.sharingMode == VK_SHARING_MODE_CONCURRENT) { skip |= ValidImageBufferQueue(pCB, buffer_state->Handle(), queue_state->queueFamilyIndex, buffer_state->createInfo.queueFamilyIndexCount, diff --git a/layers/descriptor_sets.cpp b/layers/descriptor_sets.cpp index 7086bfc096b..64cdf408aee 100644 --- a/layers/descriptor_sets.cpp +++ b/layers/descriptor_sets.cpp @@ -832,7 +832,6 @@ cvdescriptorset::DescriptorSet::DescriptorSet(const VkDescriptorSet set, DESCRIP } else { descriptors_.emplace_back(new ((free_descriptor++)->Sampler()) SamplerDescriptor(state_data, nullptr)); } - descriptors_.back()->AddParent(this); } break; } @@ -847,7 +846,6 @@ cvdescriptorset::DescriptorSet::DescriptorSet(const VkDescriptorSet set, DESCRIP descriptors_.emplace_back(new ((free_descriptor++)->ImageSampler()) ImageSamplerDescriptor(state_data, nullptr)); } - descriptors_.back()->AddParent(this); } break; } @@ -857,27 +855,23 @@ cvdescriptorset::DescriptorSet::DescriptorSet(const VkDescriptorSet set, DESCRIP case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE: for (uint32_t di = 0; di < layout_->GetDescriptorCountFromIndex(i); ++di) { descriptors_.emplace_back(new ((free_descriptor++)->Image()) ImageDescriptor(type)); - descriptors_.back()->AddParent(this); } break; case VK_DESCRIPTOR_TYPE_UNIFORM_TEXEL_BUFFER: case VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER: for (uint32_t di = 0; di < layout_->GetDescriptorCountFromIndex(i); ++di) { descriptors_.emplace_back(new ((free_descriptor++)->Texel()) TexelDescriptor(type)); - descriptors_.back()->AddParent(this); } break; case VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER: case VK_DESCRIPTOR_TYPE_STORAGE_BUFFER: for (uint32_t di = 0; di < layout_->GetDescriptorCountFromIndex(i); ++di) { descriptors_.emplace_back(new ((free_descriptor++)->Buffer()) BufferDescriptor(type)); - descriptors_.back()->AddParent(this); } break; case VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT: for (uint32_t di = 0; di < layout_->GetDescriptorCountFromIndex(i); ++di) { descriptors_.emplace_back(new ((free_descriptor++)->InlineUniform()) InlineUniformDescriptor(type)); - descriptors_.back()->AddParent(this); } break; case VK_DESCRIPTOR_TYPE_ACCELERATION_STRUCTURE_NV: @@ -885,13 +879,11 @@ cvdescriptorset::DescriptorSet::DescriptorSet(const VkDescriptorSet set, DESCRIP for (uint32_t di = 0; di < layout_->GetDescriptorCountFromIndex(i); ++di) { descriptors_.emplace_back(new ((free_descriptor++)->AccelerationStructure()) AccelerationStructureDescriptor(type)); - descriptors_.back()->AddParent(this); } break; case VK_DESCRIPTOR_TYPE_MUTABLE_VALVE: for (uint32_t di = 0; di < layout_->GetDescriptorCountFromIndex(i); ++di) { descriptors_.emplace_back(new ((free_descriptor++)->Mutable()) MutableDescriptor()); - descriptors_.back()->AddParent(this); } break; default: @@ -899,7 +891,6 @@ cvdescriptorset::DescriptorSet::DescriptorSet(const VkDescriptorSet set, DESCRIP for (uint32_t di = 0; di < layout_->GetDescriptorCountFromIndex(i); ++di) { dynamic_offset_idx_to_descriptor_list_.push_back(descriptors_.size()); descriptors_.emplace_back(new ((free_descriptor++)->Buffer()) BufferDescriptor(type)); - descriptors_.back()->AddParent(this); } } else { assert(0); // Bad descriptor type specified @@ -909,6 +900,13 @@ cvdescriptorset::DescriptorSet::DescriptorSet(const VkDescriptorSet set, DESCRIP } } +void cvdescriptorset::DescriptorSet::LinkChildNodes() { + // Connect child node(s), which cannot safely be done in the constructor. + for (auto &desc : descriptors_) { + desc->AddParent(this); + } +} + void cvdescriptorset::DescriptorSet::Destroy() { for (auto &desc: descriptors_) { desc->RemoveParent(this); diff --git a/layers/descriptor_sets.h b/layers/descriptor_sets.h index 41ca4160239..d1f4a1ab935 100644 --- a/layers/descriptor_sets.h +++ b/layers/descriptor_sets.h @@ -739,6 +739,7 @@ class DescriptorSet : public BASE_NODE { using StateTracker = ValidationStateTracker; DescriptorSet(const VkDescriptorSet, DESCRIPTOR_POOL_STATE *, const std::shared_ptr &, uint32_t variable_count, const StateTracker *state_data_const); + void LinkChildNodes() override; ~DescriptorSet() { Destroy(); } // A number of common Get* functions that return data based on layout from which this set was created diff --git a/layers/device_memory_state.h b/layers/device_memory_state.h index d3775c70e39..b0a86caf60b 100644 --- a/layers/device_memory_state.h +++ b/layers/device_memory_state.h @@ -1,7 +1,7 @@ -/* Copyright (c) 2015-2021 The Khronos Group Inc. - * Copyright (c) 2015-2021 Valve Corporation - * Copyright (c) 2015-2021 LunarG, Inc. - * Copyright (C) 2015-2021 Google Inc. +/* Copyright (c) 2015-2022 The Khronos Group Inc. + * Copyright (c) 2015-2022 Valve Corporation + * Copyright (c) 2015-2022 LunarG, Inc. + * Copyright (C) 2015-2022 Google Inc. * Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -80,8 +80,6 @@ class DEVICE_MEMORY_STATE : public BASE_NODE { bool IsDedicatedImage() const { return dedicated && dedicated->handle.type == kVulkanObjectTypeImage; } VkDeviceMemory mem() const { return handle_.Cast(); } - - const NodeSet &ObjectBindings() const { return parent_nodes_; } }; // Generic memory binding struct to track objects bound to objects diff --git a/layers/image_state.cpp b/layers/image_state.cpp index ce7f8c7391b..9c945afe5ab 100644 --- a/layers/image_state.cpp +++ b/layers/image_state.cpp @@ -331,10 +331,13 @@ void IMAGE_STATE::AddAliasingImage(IMAGE_STATE *bound_image) { void IMAGE_STATE::SetMemBinding(std::shared_ptr &mem, VkDeviceSize memory_offset) { if ((createInfo.flags & VK_IMAGE_CREATE_ALIAS_BIT) != 0) { - for (auto *base_node : mem->ObjectBindings()) { - if (base_node->Type() == kVulkanObjectTypeImage) { - auto other_image = static_cast(base_node); - AddAliasingImage(other_image); + for (auto &entry : mem->ObjectBindings()) { + if (entry.first.type == kVulkanObjectTypeImage) { + auto base_node = entry.second.lock(); + if (base_node) { + auto other_image = static_cast(base_node.get()); + AddAliasingImage(other_image); + } } } } @@ -346,11 +349,14 @@ void IMAGE_STATE::SetSwapchain(std::shared_ptr &swapchain, uint3 bind_swapchain = swapchain; swapchain_image_index = swapchain_index; bind_swapchain->AddParent(this); - for (auto *base_node : swapchain->ObjectBindings()) { - if (base_node->Type() == kVulkanObjectTypeImage) { - auto other_image = static_cast(base_node); - if (swapchain_image_index == other_image->swapchain_image_index) { - AddAliasingImage(other_image); + for (auto &entry : swapchain->ObjectBindings()) { + if (entry.first.type == kVulkanObjectTypeImage) { + auto base_node = entry.second.lock(); + if (base_node) { + auto other_image = static_cast(base_node.get()); + if (swapchain_image_index == other_image->swapchain_image_index) { + AddAliasingImage(other_image); + } } } } @@ -435,9 +441,7 @@ IMAGE_VIEW_STATE::IMAGE_VIEW_STATE(const std::shared_ptr &im, VkIma min_lod(GetImageViewMinLod(ci)), format_features(ff), inherited_usage(GetInheritedUsage(ci, *im)), - image_state(im) { - image_state->AddParent(this); -} + image_state(im) {} void IMAGE_VIEW_STATE::Destroy() { if (image_state) { diff --git a/layers/image_state.h b/layers/image_state.h index 8bb4806391a..05947fb910e 100644 --- a/layers/image_state.h +++ b/layers/image_state.h @@ -212,9 +212,13 @@ class IMAGE_VIEW_STATE : public BASE_NODE { IMAGE_VIEW_STATE(const std::shared_ptr &image_state, VkImageView iv, const VkImageViewCreateInfo *ci, VkFormatFeatureFlags2KHR ff, const VkFilterCubicImageViewImageFormatPropertiesEXT &cubic_props); IMAGE_VIEW_STATE(const IMAGE_VIEW_STATE &rh_obj) = delete; - VkImageView image_view() const { return handle_.Cast(); } + void LinkChildNodes() override { + // Connect child node(s), which cannot safely be done in the constructor. + image_state->AddParent(this); + } + virtual ~IMAGE_VIEW_STATE() { if (!Destroyed()) { Destroy(); @@ -271,8 +275,6 @@ class SWAPCHAIN_NODE : public BASE_NODE { void Destroy() override; - const NodeSet &ObjectBindings() const { return parent_nodes_; } - protected: void NotifyInvalidate(const BASE_NODE::NodeList &invalid_nodes, bool unlink) override; }; diff --git a/layers/pipeline_state.cpp b/layers/pipeline_state.cpp index 5ec32cb8718..3e0fa427e54 100644 --- a/layers/pipeline_state.cpp +++ b/layers/pipeline_state.cpp @@ -439,7 +439,7 @@ void LAST_BOUND_STATE::UnbindAndResetPushDescriptorSet(CMD_BUFFER_STATE *cb_stat if (push_descriptor_set) { for (auto &ps : per_set) { if (ps.bound_descriptor_set == push_descriptor_set) { - cb_state->RemoveChild(ps.bound_descriptor_set.get()); + cb_state->RemoveChild(ps.bound_descriptor_set); ps.bound_descriptor_set.reset(); } } diff --git a/layers/render_pass_state.cpp b/layers/render_pass_state.cpp index 8ff49039799..755c1dc89e3 100644 --- a/layers/render_pass_state.cpp +++ b/layers/render_pass_state.cpp @@ -1,7 +1,7 @@ -/* Copyright (c) 2015-2021 The Khronos Group Inc. - * Copyright (c) 2015-2021 Valve Corporation - * Copyright (c) 2015-2021 LunarG, Inc. - * Copyright (C) 2015-2021 Google Inc. +/* Copyright (c) 2015-2022 The Khronos Group Inc. + * Copyright (c) 2015-2022 Valve Corporation + * Copyright (c) 2015-2022 LunarG, Inc. + * Copyright (C) 2015-2022 Google Inc. * Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -316,7 +316,10 @@ FRAMEBUFFER_STATE::FRAMEBUFFER_STATE(VkFramebuffer fb, const VkFramebufferCreate : BASE_NODE(fb, kVulkanObjectTypeFramebuffer), createInfo(pCreateInfo), rp_state(rpstate), - attachments_view_state(std::move(attachments)) { + attachments_view_state(std::move(attachments)) {} + +void FRAMEBUFFER_STATE::LinkChildNodes() { + // Connect child node(s), which cannot safely be done in the constructor. for (auto &a : attachments_view_state) { a->AddParent(this); } diff --git a/layers/render_pass_state.h b/layers/render_pass_state.h index 064aeb96941..4d0232333e3 100644 --- a/layers/render_pass_state.h +++ b/layers/render_pass_state.h @@ -1,7 +1,7 @@ -/* Copyright (c) 2015-2021 The Khronos Group Inc. - * Copyright (c) 2015-2021 Valve Corporation - * Copyright (c) 2015-2021 LunarG, Inc. - * Copyright (C) 2015-2021 Google Inc. +/* Copyright (c) 2015-2022 The Khronos Group Inc. + * Copyright (c) 2015-2022 Valve Corporation + * Copyright (c) 2015-2022 LunarG, Inc. + * Copyright (C) 2015-2022 Google Inc. * Modifications Copyright (C) 2020 Advanced Micro Devices, Inc. All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -131,6 +131,7 @@ class FRAMEBUFFER_STATE : public BASE_NODE { FRAMEBUFFER_STATE(VkFramebuffer fb, const VkFramebufferCreateInfo *pCreateInfo, std::shared_ptr &&rpstate, std::vector> &&attachments); + void LinkChildNodes() override; VkFramebuffer framebuffer() const { return handle_.Cast(); } diff --git a/layers/state_tracker.h b/layers/state_tracker.h index ed3157391a8..79ff65c45f1 100644 --- a/layers/state_tracker.h +++ b/layers/state_tracker.h @@ -280,7 +280,9 @@ class ValidationStateTracker : public ValidationObject { void Add(std::shared_ptr&& state_object) { auto& map = GetStateMap(); auto handle = state_object->Handle().template Cast::HandleType>(); - + // Finish setting up the object node tree, which cannot be done from the state object contructors + // due to use of shared_from_this() + state_object->LinkChildNodes(); map.insert_or_assign(handle, std::move(state_object)); }