take loader_lock when adding unknown functions

.github/workflows/build.yml

@@ -112,7 +112,7 @@ jobs:
                 -D CMAKE_CXX_COMPILER=clang++
             - run: cmake --build build
             - run: cmake --install build --prefix /tmp
-            - run: ctest --parallel --output-on-failure -E UnknownFunction --test-dir build/
+            - run: ctest --parallel --output-on-failure -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/
 
     linux-32:
         needs: codegen
@@ -198,7 +198,7 @@ jobs:
                 # https://gitlab.kitware.com/cmake/cmake/-/issues/25317
                 PKG_CONFIG_PATH: /usr/lib/i386-linux-gnu/pkgconfig
             - run: cmake --build build
-            - run: ctest --parallel --output-on-failure -E UnknownFunction --test-dir build/
+            - run: ctest --parallel --output-on-failure -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/
 
     linux-arm:
         needs: codegen
@@ -310,7 +310,7 @@ jobs:
                 -A ${{ matrix.arch }} `
                 -D BUILD_WERROR=ON
             - run: cmake --build build/ --config Release
-            - run: ctest --parallel --output-on-failure -C Release -E UnknownFunction --test-dir build/
+            - run: ctest --parallel --output-on-failure -C Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/
 
     windows_arm:
         # Native Windows on Arm: covers the arm64 and arm64ec MARMASM paths the x64 runners can't.
@@ -531,7 +531,7 @@ jobs:
               env:
                 LDFLAGS: -Wl,-fatal_warnings
             - run: cmake --build build --config Release
-            - run: ctest --parallel --output-on-failure --build-config Release -E UnknownFunction --test-dir build/
+            - run: ctest --parallel --output-on-failure --build-config Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/
             - run: cmake --install build --config Release --prefix /tmp
             - name: Verify Universal Binary
               if: ${{ matrix.static == 'OFF' }}
@@ -568,7 +568,7 @@ jobs:
               env:
                 LDFLAGS: -Wl,-fatal_warnings
             - run: cmake --build build --config Release
-            - run: ctest --parallel --output-on-failure --build-config Release -E UnknownFunction --test-dir build/
+            - run: ctest --parallel --output-on-failure --build-config Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/
             - run: cmake --install build --config Release --prefix /tmp
             - name: Verify Universal Binary
               if: ${{ matrix.static == 'OFF' }}

loader/unknown_function_handling.c

@@ -63,6 +63,7 @@ void loader_free_phys_dev_ext_table(struct loader_instance *inst) { (void)inst;
 #else
 
 #include "allocation.h"
+#include "loader.h"
 #include "log.h"
 
 // Forward declarations
@@ -210,12 +211,22 @@ void *loader_dev_ext_gpa_impl(struct loader_instance *inst, const char *funcName
     return out_function;
 }
 
+// Main interface functions. Both hold loader_lock while registering a function: the implementation appends to the
+// instance's unknown function name table and walks the driver and logical device lists, all of which
+// vkCreateDevice/vkDestroyDevice/vkEnumeratePhysicalDevices mutate under that lock, while vkGetInstanceProcAddr takes
+// no lock of its own. loader_lock is recursive, so callers which already hold it are unaffected.
 void *loader_dev_ext_gpa_tramp(struct loader_instance *inst, const char *funcName) {
-    return loader_dev_ext_gpa_impl(inst, funcName, true);
+    loader_platform_thread_lock_mutex(&loader_lock);
+    void *addr = loader_dev_ext_gpa_impl(inst, funcName, true);
+    loader_platform_thread_unlock_mutex(&loader_lock);
+    return addr;
 }
 
 void *loader_dev_ext_gpa_term(struct loader_instance *inst, const char *funcName) {
-    return loader_dev_ext_gpa_impl(inst, funcName, false);
+    loader_platform_thread_lock_mutex(&loader_lock);
+    void *addr = loader_dev_ext_gpa_impl(inst, funcName, false);
+    loader_platform_thread_unlock_mutex(&loader_lock);
+    return addr;
 }
 
 // Physical Device function handling
@@ -367,12 +378,19 @@ void *loader_phys_dev_ext_gpa_impl(struct loader_instance *inst, const char *fun
     }
     return loader_get_phys_dev_ext_termin(new_function_index);
 }
-// Main interface functions, makes it clear whether it is getting a terminator or trampoline
+// Main interface functions, makes it clear whether it is getting a terminator or trampoline. Both hold loader_lock for
+// the same reason as the device variants above.
 void *loader_phys_dev_ext_gpa_tramp(struct loader_instance *inst, const char *funcName) {
-    return loader_phys_dev_ext_gpa_impl(inst, funcName, true);
+    loader_platform_thread_lock_mutex(&loader_lock);
+    void *addr = loader_phys_dev_ext_gpa_impl(inst, funcName, true);
+    loader_platform_thread_unlock_mutex(&loader_lock);
+    return addr;
 }
 void *loader_phys_dev_ext_gpa_term(struct loader_instance *inst, const char *funcName) {
-    return loader_phys_dev_ext_gpa_impl(inst, funcName, false);
+    loader_platform_thread_lock_mutex(&loader_lock);
+    void *addr = loader_phys_dev_ext_gpa_impl(inst, funcName, false);
+    loader_platform_thread_unlock_mutex(&loader_lock);
+    return addr;
 }
 
 #endif

tests/loader_threading_tests.cpp

@@ -199,3 +199,87 @@ TEST(Threading, SurfaceCreateDestroyLoop) {
         surface_threads[i].join();
     }
 }
+
+// The driver reports these through vk_icdGetPhysicalDeviceProcAddr, so vkGetInstanceProcAddr has to append each one to
+// the loader's instance wide unknown function tables before it can hand back a trampoline.
+VKAPI_ATTR uint32_t VKAPI_CALL test_unknown_phys_dev_function(VkPhysicalDevice, uint32_t foo) { return foo; }
+
+void get_unknown_function_loop(InstWrapper* inst, std::vector<std::string> const* func_names) {
+    for (auto const& name : *func_names) {
+        PFN_vkVoidFunction func = inst->load(name.c_str());
+        ASSERT_NE(func, nullptr);
+    }
+}
+
+TEST(Threading, GetUnknownPhysicalDeviceFunctionLoop) {
+    // Capped so that thread_count * funcs_per_thread stays under MAX_NUM_UNKNOWN_EXTS on a machine with many cores
+    uint32_t thread_count = std::thread::hardware_concurrency();
+    if (thread_count < 2) thread_count = 2;
+    if (thread_count > 8) thread_count = 8;
+    const uint32_t funcs_per_thread = 16;
+
+    FrameworkEnvironment env{FrameworkSettings{}.set_log_filter("")};
+    auto& phys_dev = env.add_icd(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA).add_and_get_physical_device({});
+
+    // Each thread queries its own set of names, so every query appends a new entry rather than finding an existing one
+    std::vector<std::vector<std::string>> func_names{thread_count};
+    for (uint32_t i = 0; i < thread_count; i++) {
+        for (uint32_t j = 0; j < funcs_per_thread; j++) {
+            func_names[i].push_back("vkNotRealFuncTEST_" + std::to_string(i) + "_" + std::to_string(j));
+            phys_dev.custom_physical_device_functions.push_back(
+                VulkanFunction{func_names[i].back(), to_vkVoidFunction(test_unknown_phys_dev_function)});
+        }
+    }
+
+    InstWrapper inst{env.vulkan_functions};
+    inst.CheckCreate();
+
+    std::vector<std::thread> function_query_threads;
+    for (uint32_t i = 0; i < thread_count; i++) {
+        function_query_threads.emplace_back(get_unknown_function_loop, &inst, &func_names[i]);
+    }
+    for (uint32_t i = 0; i < thread_count; i++) {
+        function_query_threads[i].join();
+    }
+}
+
+// Reported through the driver's vkGetInstanceProcAddr, which puts them on the unknown device function path. Registering
+// one walks every driver's logical device list, which vkCreateDevice and vkDestroyDevice are editing in the other threads.
+VKAPI_ATTR uint32_t VKAPI_CALL test_unknown_device_function(VkDevice, uint32_t foo) { return foo; }
+
+void create_destroy_device_only_loop(InstWrapper* inst, uint32_t num_loops) {
+    for (uint32_t i = 0; i < num_loops; i++) {
+        DeviceWrapper dev{*inst};
+        dev.CheckCreate(inst->GetPhysDev());
+    }
+}
+
+TEST(Threading, GetUnknownDeviceFunctionWhileCreatingDevices) {
+    const uint32_t thread_count = 4;
+    const uint32_t funcs_per_thread = 16;
+    const uint32_t num_loops_create_destroy_device = 50;
+
+    FrameworkEnvironment env{FrameworkSettings{}.set_log_filter("")};
+    auto& phys_dev = env.add_icd(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA).add_and_get_physical_device({});
+
+    std::vector<std::vector<std::string>> func_names{thread_count};
+    for (uint32_t i = 0; i < thread_count; i++) {
+        for (uint32_t j = 0; j < funcs_per_thread; j++) {
+            func_names[i].push_back("vkNotRealDeviceFuncTEST_" + std::to_string(i) + "_" + std::to_string(j));
+            phys_dev.known_device_functions.push_back(
+                VulkanFunction{func_names[i].back(), to_vkVoidFunction(test_unknown_device_function)});
+        }
+    }
+
+    InstWrapper inst{env.vulkan_functions};
+    inst.CheckCreate();
+
+    std::vector<std::thread> threads;
+    for (uint32_t i = 0; i < thread_count; i++) {
+        threads.emplace_back(get_unknown_function_loop, &inst, &func_names[i]);
+        threads.emplace_back(create_destroy_device_only_loop, &inst, num_loops_create_destroy_device);
+    }
+    for (auto& thread : threads) {
+        thread.join();
+    }
+}