Guard loader.instances access with mutex.

While the adding and removing of data from this global linked list
was guarded, GetInstanceProcAddr & GetDeviceProcAddr did not have
such guards. This results in race conditions that were detected with
thread sanitizer. This commit adds a mutex solely for the
loader.instances global variable.

diff --git a/loader/loader.c b/loader/loader.c
index 98054a4..ae819ce 100644 (file)
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -88,6 +88,7 @@ struct activated_layer_info {
 loader_platform_thread_mutex loader_lock;
 loader_platform_thread_mutex loader_json_lock;
 loader_platform_thread_mutex loader_preload_icd_lock;
+loader_platform_thread_mutex loader_global_instance_list_lock;
 
 // A list of ICDs that gets initialized when the loader does its global initialization. This list should never be used by anything
 // other than EnumerateInstanceExtensionProperties(), vkDestroyInstance, and loader_release(). This list does not change
@@ -1161,6 +1162,7 @@ out:
 }
 
 struct loader_icd_term *loader_get_icd_and_device(const void *device, struct loader_device **found_dev, uint32_t *icd_index) {
+    loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
     *found_dev = NULL;
     for (struct loader_instance *inst = loader.instances; inst; inst = inst->next) {
         uint32_t index = 0;
@@ -1174,11 +1176,13 @@ struct loader_icd_term *loader_get_icd_and_device(const void *device, struct loa
                     if (NULL != icd_index) {
                         *icd_index = index;
                     }
+                    loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
                     return icd_term;
                 }
             index++;
         }
     }
+    loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
     return NULL;
 }
 
@@ -1485,6 +1489,8 @@ void loader_initialize(void) {
     loader_platform_thread_create_mutex(&loader_lock);
     loader_platform_thread_create_mutex(&loader_json_lock);
     loader_platform_thread_create_mutex(&loader_preload_icd_lock);
+    loader_platform_thread_create_mutex(&loader_global_instance_list_lock);
+
     // initialize logging
     loader_debug_init();
 #if defined(_WIN32)
@@ -1507,6 +1513,7 @@ void loader_release() {
     loader_platform_thread_delete_mutex(&loader_lock);
     loader_platform_thread_delete_mutex(&loader_json_lock);
     loader_platform_thread_delete_mutex(&loader_preload_icd_lock);
+    loader_platform_thread_delete_mutex(&loader_global_instance_list_lock);
 }
 
 // Preload the ICD libraries that are likely to be needed so we don't repeatedly load/unload them later
@@ -3962,12 +3969,14 @@ struct loader_instance *loader_get_instance(const VkInstance instance) {
         return NULL;
     } else {
         disp = loader_get_instance_layer_dispatch(instance);
+        loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
         for (struct loader_instance *inst = loader.instances; inst; inst = inst->next) {
             if (&inst->disp->layer_inst_disp == disp) {
                 ptr_instance = inst;
                 break;
             }
         }
+        loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
     }
     return ptr_instance;
 }
@@ -5331,6 +5340,7 @@ VKAPI_ATTR void VKAPI_CALL terminator_DestroyInstance(VkInstance instance, const
 
     // Remove this instance from the list of instances:
     struct loader_instance *prev = NULL;
+    loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
     struct loader_instance *next = loader.instances;
     while (next != NULL) {
         if (next == ptr_instance) {
@@ -5344,6 +5354,7 @@ VKAPI_ATTR void VKAPI_CALL terminator_DestroyInstance(VkInstance instance, const
         prev = next;
         next = next->next;
     }
+    loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
 
     while (NULL != icd_terms) {
         if (icd_terms->instance) {

diff --git a/loader/loader.h b/loader/loader.h
index 3beb8b6..3022096 100644 (file)
--- a/loader/loader.h
+++ b/loader/loader.h
@@ -82,6 +82,7 @@ extern struct loader_struct loader;
 extern loader_platform_thread_mutex loader_lock;
 extern loader_platform_thread_mutex loader_json_lock;
 extern loader_platform_thread_mutex loader_preload_icd_lock;
+extern loader_platform_thread_mutex loader_global_instance_list_lock;
 
 bool compare_vk_extension_properties(const VkExtensionProperties *op1, const VkExtensionProperties *op2);
 

diff --git a/loader/trampoline.c b/loader/trampoline.c
index 12f7755..fa07218 100644 (file)
--- a/loader/trampoline.c
+++ b/loader/trampoline.c
@@ -572,8 +572,10 @@ LOADER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateInstance(const VkInstanceCr
     }
     memcpy(&ptr_instance->disp->layer_inst_disp, &instance_disp, sizeof(instance_disp));
 
+    loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
     ptr_instance->next = loader.instances;
     loader.instances = ptr_instance;
+    loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
 
     // Activate any layers on instance chain
     res = loader_enable_instance_layers(ptr_instance, &ici, &ptr_instance->instance_layer_list);
@@ -611,10 +613,13 @@ out:
 
     if (NULL != ptr_instance) {
         if (res != VK_SUCCESS) {
+            loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
             // error path, should clean everything up
             if (loader.instances == ptr_instance) {
                 loader.instances = ptr_instance->next;
             }
+            loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
+
             loader_instance_heap_free(ptr_instance, ptr_instance->disp);
             // Remove any created VK_EXT_debug_report or VK_EXT_debug_utils items
             destroy_debug_callbacks_chain(ptr_instance, pAllocator);
@@ -646,6 +651,7 @@ out:
             ptr_instance->InstanceCreationDeletionDebugFunctionHead = ptr_instance->DbgFunctionHead;
             ptr_instance->DbgFunctionHead = NULL;
         }
+        // Only unlock when ptr_instance isn't NULL, as if it is, the above code didn't make it to when loader_lock was locked.
         loader_platform_thread_unlock_mutex(&loader_lock);
     }
 

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 9de446f..f9677bc 100644 (file)
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -43,6 +43,8 @@ target_link_libraries(test_regression PUBLIC testing_dependencies)
 set_target_properties(test_regression ${LOADER_STANDARD_CXX_PROPERTIES})
 target_compile_definitions(test_regression PUBLIC VK_NO_PROTOTYPES)
 
+# Threading tests live in separate executabe just for threading tests as it'll need support
+# in the test harness to enable in CI, as thread sanitizer doesn't work with address sanitizer enabled.
 add_executable(
     test_threading
         loader_testing_main.cpp

diff --git a/tests/framework/CMakeLists.txt b/tests/framework/CMakeLists.txt
index 75bbf49..1ecc82c 100644 (file)
--- a/tests/framework/CMakeLists.txt
+++ b/tests/framework/CMakeLists.txt
@@ -41,6 +41,8 @@ if (UNIX)
         target_link_options(testing_framework_util PUBLIC -fsanitize=thread)
         target_compile_options(vulkan PUBLIC -fsanitize=thread)
         target_link_options(vulkan PUBLIC -fsanitize=thread)
+        target_compile_options(gtest PUBLIC -fsanitize=thread)
+        target_link_options(gtest PUBLIC -fsanitize=thread)
     endif()
 endif()
 

diff --git a/tests/framework/icd/test_icd.cpp b/tests/framework/icd/test_icd.cpp
index 41790db..75ccb81 100644 (file)
--- a/tests/framework/icd/test_icd.cpp
+++ b/tests/framework/icd/test_icd.cpp
@@ -1378,7 +1378,6 @@ extern FRAMEWORK_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vk_icdNegotiateLoaderICDI
 
 #if TEST_ICD_EXPORT_ICD_GPDPA
 FRAMEWORK_EXPORT VKAPI_ATTR PFN_vkVoidFunction VKAPI_CALL vk_icdGetPhysicalDeviceProcAddr(VkInstance instance, const char* pName) {
-    // std::cout << "icdGetPhysicalDeviceProcAddr: " << pName << "\n";
     return get_physical_device_func(instance, pName);
 }
 #endif  // TEST_ICD_EXPORT_ICD_GPDPA

diff --git a/tests/framework/test_environment.cpp b/tests/framework/test_environment.cpp
index 5d058ab..8431b24 100644 (file)
--- a/tests/framework/test_environment.cpp
+++ b/tests/framework/test_environment.cpp
@@ -136,7 +136,7 @@ void FillDebugUtilsCreateDetails(InstanceCreateInfo& create_info, DebugUtilsWrap
     create_info.instance_info.pNext = wrapper.get();
 }
 
-PlatformShimWrapper::PlatformShimWrapper(std::vector<fs::FolderManager>* folders) noexcept {
+PlatformShimWrapper::PlatformShimWrapper(std::vector<fs::FolderManager>* folders, bool enable_log) noexcept {
 #if defined(WIN32) || defined(__APPLE__)
     shim_library = LibraryWrapper(SHIM_LIBRARY_NAME);
     PFN_get_platform_shim get_platform_shim_func = shim_library.get_symbol(GET_PLATFORM_SHIM_STR);
@@ -147,8 +147,9 @@ PlatformShimWrapper::PlatformShimWrapper(std::vector<fs::FolderManager>* folders
 #endif
     platform_shim->reset();
 
-    // leave it permanently on at full blast
-    set_env_var("VK_LOADER_DEBUG", "all");
+    if (enable_log) {
+        set_env_var("VK_LOADER_DEBUG", "all");
+    }
 }
 PlatformShimWrapper::~PlatformShimWrapper() noexcept { platform_shim->reset(); }
 
@@ -184,7 +185,8 @@ TestLayer& TestLayerHandle::reset_layer() noexcept {
 fs::path TestLayerHandle::get_layer_full_path() noexcept { return layer_library.lib_path; }
 fs::path TestLayerHandle::get_layer_manifest_path() noexcept { return manifest_path; }
 
-FrameworkEnvironment::FrameworkEnvironment() noexcept : platform_shim(&folders), vulkan_functions() {
+FrameworkEnvironment::FrameworkEnvironment() noexcept : FrameworkEnvironment(true) {}
+FrameworkEnvironment::FrameworkEnvironment(bool enable_log) noexcept : platform_shim(&folders, enable_log), vulkan_functions() {
     // This order is important, it matches the enum ManifestLocation, used to index the folders vector
     folders.emplace_back(FRAMEWORK_BUILD_DIRECTORY, std::string("null_dir"));
     folders.emplace_back(FRAMEWORK_BUILD_DIRECTORY, std::string("icd_manifests"));
@@ -508,3 +510,9 @@ VkSurfaceKHR create_surface(InstWrapper& inst, const char* api_selection) {
 
     return surface;
 }
+
+extern "C" {
+void __ubsan_on_report() { FAIL() << "Encountered an undefined behavior sanitizer error"; }
+void __asan_on_error() { FAIL() << "Encountered an address sanitizer error"; }
+void __tsan_on_report() { FAIL() << "Encountered a thread sanitizer error"; }
+}  // extern "C"

diff --git a/tests/framework/test_environment.h b/tests/framework/test_environment.h
index dbd9d71..20a2530 100644 (file)
--- a/tests/framework/test_environment.h
+++ b/tests/framework/test_environment.h
@@ -262,7 +262,7 @@ void FillDebugUtilsCreateDetails(InstanceCreateInfo& create_info, DebugUtilsWrap
 struct FrameworkEnvironment;  // forward declaration
 
 struct PlatformShimWrapper {
-    PlatformShimWrapper(std::vector<fs::FolderManager>* folders) noexcept;
+    PlatformShimWrapper(std::vector<fs::FolderManager>* folders, bool enable_log) noexcept;
     ~PlatformShimWrapper() noexcept;
     PlatformShimWrapper(PlatformShimWrapper const&) = delete;
     PlatformShimWrapper& operator=(PlatformShimWrapper const&) = delete;
@@ -345,7 +345,8 @@ enum class ManifestLocation {
 };
 
 struct FrameworkEnvironment {
-    FrameworkEnvironment() noexcept;
+    FrameworkEnvironment() noexcept;  // default is to enable VK_LOADER_DEBUG=all
+    FrameworkEnvironment(bool enable_log) noexcept;
 
     void add_icd(TestICDDetails icd_details) noexcept;
     void add_implicit_layer(ManifestLayer layer_manifest, const std::string& json_name) noexcept;

diff --git a/tests/loader_alloc_callback_tests.cpp b/tests/loader_alloc_callback_tests.cpp
index 56a20dd..4f3ea16 100644 (file)
--- a/tests/loader_alloc_callback_tests.cpp
+++ b/tests/loader_alloc_callback_tests.cpp
@@ -468,6 +468,7 @@ TEST(Allocation, DriverEnvVarIntentionalAllocFail) {
         ASSERT_TRUE(tracker.empty());
         fail_index++;
     }
+    remove_env_var("VK_DRIVER_FILES");
 }
 
 // Test failure during vkCreateDevice to make sure we don't leak memory if
@@ -484,7 +485,7 @@ TEST(Allocation, CreateDeviceIntentionalAllocFail) {
     driver.physical_devices.emplace_back("physical_device_1");
     driver.physical_devices[1].add_queue_family_properties({{VK_QUEUE_GRAPHICS_BIT, 1, 0, {1, 1, 1}}, false});
 
-    const char* layer_name = "VkLayerImplicit0";
+    const char* layer_name = "VK_LAYER_VkLayerImplicit0";
     env.add_implicit_layer(ManifestLayer{}.add_layer(ManifestLayer::LayerDescription{}
                                                          .set_name(layer_name)
                                                          .set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)

diff --git a/tests/loader_threading_tests.cpp b/tests/loader_threading_tests.cpp
index 78999a0..83b17da 100644 (file)
--- a/tests/loader_threading_tests.cpp
+++ b/tests/loader_threading_tests.cpp
@@ -29,29 +29,52 @@
 
 #include <mutex>
 #include <thread>
+#include <atomic>
 
-void create_destroy_device_loop(uint32_t num_loops_create_destroy_device, uint32_t num_loops_try_get_proc_addr, InstWrapper* inst,
-                                VkPhysicalDevice phys_dev) {
+void create_destroy_instance_loop_with_function_queries(FrameworkEnvironment* env, uint32_t num_loops_create_destroy_instance,
+                                                        uint32_t num_loops_try_get_instance_proc_addr,
+                                                        uint32_t num_loops_try_get_device_proc_addr) {
+    for (uint32_t i = 0; i < num_loops_create_destroy_instance; i++) {
+        InstWrapper inst{env->vulkan_functions};
+        inst.CheckCreate();
+        PFN_vkEnumeratePhysicalDevices enum_pd = nullptr;
+        for (uint32_t j = 0; j < num_loops_try_get_instance_proc_addr; j++) {
+            enum_pd = inst.load("vkEnumeratePhysicalDevices");
+            ASSERT_NE(enum_pd, nullptr);
+        }
+        VkPhysicalDevice phys_dev = inst.GetPhysDev();
+
+        DeviceWrapper dev{inst};
+        dev.create_info.add_device_queue(DeviceQueueCreateInfo{}.add_priority(1.0));
+        dev.CheckCreate(phys_dev);
+        for (uint32_t j = 0; j < num_loops_try_get_device_proc_addr; j++) {
+            PFN_vkCmdBindPipeline p = dev.load("vkCmdBindPipeline");
+            p(VK_NULL_HANDLE, VkPipelineBindPoint::VK_PIPELINE_BIND_POINT_GRAPHICS, VK_NULL_HANDLE);
+        }
+    }
+}
+
+void create_destroy_device_loop(FrameworkEnvironment* env, uint32_t num_loops_create_destroy_device,
+                                uint32_t num_loops_try_get_proc_addr) {
+    InstWrapper inst{env->vulkan_functions};
+    inst.CheckCreate();
+    VkPhysicalDevice phys_dev = inst.GetPhysDev();
     for (uint32_t i = 0; i < num_loops_create_destroy_device; i++) {
-        DeviceWrapper dev{*inst};
+        DeviceWrapper dev{inst};
         dev.create_info.add_device_queue(DeviceQueueCreateInfo{}.add_priority(1.0));
         dev.CheckCreate(phys_dev);
 
         for (uint32_t j = 0; j < num_loops_try_get_proc_addr; j++) {
-            PFN_vkCmdBindPipeline p =
-                reinterpret_cast<PFN_vkCmdBindPipeline>(dev->vkGetDeviceProcAddr(dev.dev, "vkCmdBindPipeline"));
-            ASSERT_NE(p, nullptr);
-            PFN_vkCmdBindDescriptorSets d =
-                reinterpret_cast<PFN_vkCmdBindDescriptorSets>(dev->vkGetDeviceProcAddr(dev.dev, "vkCmdBindDescriptorSets"));
-            ASSERT_NE(d, nullptr);
-            PFN_vkCmdBindVertexBuffers vb =
-                reinterpret_cast<PFN_vkCmdBindVertexBuffers>(dev->vkGetDeviceProcAddr(dev.dev, "vkCmdBindVertexBuffers"));
-            ASSERT_NE(vb, nullptr);
-            PFN_vkCmdBindIndexBuffer ib =
-                reinterpret_cast<PFN_vkCmdBindIndexBuffer>(dev->vkGetDeviceProcAddr(dev.dev, "vkCmdBindIndexBuffer"));
-            ASSERT_NE(ib, nullptr);
-            PFN_vkCmdDraw c = reinterpret_cast<PFN_vkCmdDraw>(dev->vkGetDeviceProcAddr(dev.dev, "vkCmdDraw"));
-            ASSERT_NE(c, nullptr);
+            PFN_vkCmdBindPipeline p = dev.load("vkCmdBindPipeline");
+            PFN_vkCmdBindDescriptorSets d = dev.load("vkCmdBindDescriptorSets");
+            PFN_vkCmdBindVertexBuffers vb = dev.load("vkCmdBindVertexBuffers");
+            PFN_vkCmdBindIndexBuffer ib = dev.load("vkCmdBindIndexBuffer");
+            PFN_vkCmdDraw c = dev.load("vkCmdDraw");
+            p(VK_NULL_HANDLE, VkPipelineBindPoint::VK_PIPELINE_BIND_POINT_GRAPHICS, VK_NULL_HANDLE);
+            d(VK_NULL_HANDLE, VkPipelineBindPoint::VK_PIPELINE_BIND_POINT_GRAPHICS, VK_NULL_HANDLE, 0, 0, nullptr, 0, nullptr);
+            vb(VK_NULL_HANDLE, 0, 0, nullptr, nullptr);
+            ib(VK_NULL_HANDLE, 0, 0, VkIndexType::VK_INDEX_TYPE_UINT16);
+            c(VK_NULL_HANDLE, 0, 0, 0, 0);
         }
     }
 }
@@ -67,12 +90,40 @@ VKAPI_ATTR void VKAPI_CALL test_vkCmdBindIndexBuffer(VkCommandBuffer cmd_buf, ui
                                                      const VkBuffer* pBuffers, const VkDeviceSize* pOffsets) {}
 VKAPI_ATTR void VKAPI_CALL test_vkCmdDraw(VkCommandBuffer cmd_buf, uint32_t vertexCount, uint32_t instanceCount,
                                           uint32_t firstVertex, uint32_t firstInstance) {}
-TEST(ThreadingTests, ConcurentGetDeviceProcAddr) {
-    FrameworkEnvironment env{};
+TEST(Threading, InstanceCreateDestroyLoop) {
+    const auto processor_count = std::thread::hardware_concurrency();
+
+    FrameworkEnvironment env{false};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA));
+    uint32_t num_loops_create_destroy_instance = 500;
+    uint32_t num_loops_try_get_instance_proc_addr = 5;
+    uint32_t num_loops_try_get_device_proc_addr = 100;
+    auto& driver = env.get_test_icd();
+
+    driver.physical_devices.emplace_back("physical_device_0");
+    driver.physical_devices.back().known_device_functions.push_back(
+        {"vkCmdBindPipeline", to_vkVoidFunction(test_vkCmdBindPipeline)});
+
+    std::vector<std::thread> instance_creation_threads;
+    std::vector<std::thread> function_query_threads;
+    for (uint32_t i = 0; i < processor_count; i++) {
+        instance_creation_threads.emplace_back(create_destroy_instance_loop_with_function_queries, &env,
+                                               num_loops_create_destroy_instance, num_loops_try_get_instance_proc_addr,
+                                               num_loops_try_get_device_proc_addr);
+    }
+    for (uint32_t i = 0; i < processor_count; i++) {
+        instance_creation_threads[i].join();
+    }
+}
+
+TEST(Threading, DeviceCreateDestroyLoop) {
+    const auto processor_count = std::thread::hardware_concurrency();
+
+    FrameworkEnvironment env{false};
     env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA));
-    uint32_t num_threads = 100;
-    uint32_t num_loops_create_destroy_device = 10;
-    uint32_t num_loops_try_get_proc_addr = 100;
+
+    uint32_t num_loops_create_destroy_device = 1000;
+    uint32_t num_loops_try_get_device_proc_addr = 5;
     auto& driver = env.get_test_icd();
 
     driver.physical_devices.emplace_back("physical_device_0");
@@ -86,21 +137,13 @@ TEST(ThreadingTests, ConcurentGetDeviceProcAddr) {
         {"vkCmdBindIndexBuffer", to_vkVoidFunction(test_vkCmdBindIndexBuffer)});
     driver.physical_devices.back().known_device_functions.push_back({"vkCmdDraw", to_vkVoidFunction(test_vkCmdDraw)});
 
-    InstWrapper inst{env.vulkan_functions};
-    inst.CheckCreate();
-
-    VkPhysicalDevice phys_dev = inst.GetPhysDev();
-
-    DeviceWrapper dev{inst};
-    dev.create_info.add_device_queue(DeviceQueueCreateInfo{}.add_priority(1.0));
-    dev.CheckCreate(phys_dev);
+    std::vector<std::thread> device_creation_threads;
 
-    std::vector<std::thread> threads;
-    for (uint32_t i = 0; i < num_threads; i++) {
-        threads.emplace_back(create_destroy_device_loop, num_loops_create_destroy_device, num_loops_try_get_proc_addr, &inst,
-                             phys_dev);
+    for (uint32_t i = 0; i < processor_count; i++) {
+        device_creation_threads.emplace_back(create_destroy_device_loop, &env, num_loops_create_destroy_device,
+                                             num_loops_try_get_device_proc_addr);
     }
-    for (uint32_t i = 0; i < num_threads; i++) {
-        threads[i].join();
+    for (uint32_t i = 0; i < processor_count; i++) {
+        device_creation_threads[i].join();
     }
 }