layers: Add BASE_NODE tree locking

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",

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

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

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

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 <[email protected]>
+ * Author: Tobin Ehlis <[email protected]>
+ * Author: Chris Forbes <[email protected]>
+ * Author: Mark Lobodzinski <[email protected]>
+ * Author: Dave Houlton <[email protected]>
+ * Author: John Zulauf <[email protected]>
+ * Author: Tobias Hector <[email protected]>
+ * Author: Jeremy Gebben <[email protected]>
+ */
+#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<BASE_NODE>(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);
+        }
+    }
+}

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 <atomic>
 
@@ -45,82 +46,74 @@ struct hash<VulkanTypedHandle> {
 };
 }  // 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<BASE_NODE> {
   public:
-    using NodeSet = layer_data::unordered_set<BASE_NODE *>;
-    using NodeList = small_vector<BASE_NODE *, 4>;
+    // 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<VulkanTypedHandle, std::weak_ptr<BASE_NODE>>;
+    using NodeList = small_vector<std::shared_ptr<BASE_NODE>, 4, uint32_t>;
 
     template <typename Handle>
     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<bool> 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 {

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<DEVICE_MEMORY_STATE>(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());

layers/buffer_state.h

@@ -54,10 +54,11 @@ class BUFFER_VIEW_STATE : public BASE_NODE {
 
     BUFFER_VIEW_STATE(const std::shared_ptr<BUFFER_STATE> &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()) {