Warn user when using device layers field #1086

The device layer fields in VkDeviceCreateInfo are deprecated and have
been for years. Users should not be adding anything to them (and if
they do, it should match the layers added in VkInstanceCreateInfo).

This would be a warning that 'device layers are deprecated, they do
nothing' in case a user is trying to enable validation layers using
them. Charles has seen multiple users not knowing that they are
deprecated and get burned by the API due to not having validation
layers enabled.

Since there may be applications in the wild which currently do provide
a list of layers to device create info, this warning should only be
issued if the list differs from instance creation. That way no spurious
warnings are created.

diff --git a/loader/loader.c b/loader/loader.c
index c98d37c..37ae180 100644 (file)
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -4710,6 +4710,20 @@ VkResult loader_create_instance_chain(const VkInstanceCreateInfo *pCreateInfo, c
 
     memcpy(&loader_create_info, pCreateInfo, sizeof(VkInstanceCreateInfo));
 
+    if (pCreateInfo->enabledLayerCount > 0 && pCreateInfo->ppEnabledLayerNames != NULL) {
+        inst->enabled_layer_count = pCreateInfo->enabledLayerCount;
+
+        inst->enabled_layer_names = (char **)loader_instance_heap_alloc(inst, sizeof(char *) * pCreateInfo->enabledLayerCount,
+                                                                        VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE);
+
+        for (uint32_t i = 0, n = inst->enabled_layer_count; i < n; ++i) {
+            size_t size = strlen(pCreateInfo->ppEnabledLayerNames[i]) + 1;
+            inst->enabled_layer_names[i] = (char *)loader_instance_heap_alloc(inst, sizeof(char) * size, VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE);
+            memset(inst->enabled_layer_names[i], '\0', sizeof(char) * size);
+            strcpy(inst->enabled_layer_names[i], pCreateInfo->ppEnabledLayerNames[i]);
+        }
+    }
+
     if (inst->expanded_activated_layer_list.count > 0) {
         chain_info.u.pLayerInfo = NULL;
         chain_info.pNext = pCreateInfo->pNext;
@@ -5004,6 +5018,34 @@ VkResult loader_create_device_chain(const VkPhysicalDevice pd, const VkDeviceCre
 
     memcpy(&loader_create_info, pCreateInfo, sizeof(VkDeviceCreateInfo));
 
+    if (loader_create_info.enabledLayerCount > 0 && loader_create_info.ppEnabledLayerNames != NULL) {
+        bool invalid_device_layer_usage = false;
+
+        if (loader_create_info.enabledLayerCount != inst->enabled_layer_count && loader_create_info.enabledLayerCount > 0) {
+            invalid_device_layer_usage = true;
+        } else if (loader_create_info.enabledLayerCount > 0 && loader_create_info.ppEnabledLayerNames == NULL) {
+            invalid_device_layer_usage = true;
+        } else if (loader_create_info.enabledLayerCount == 0 && loader_create_info.ppEnabledLayerNames != NULL) {
+            invalid_device_layer_usage = true;
+        } else if (inst->enabled_layer_names != NULL) {
+            for (uint32_t i = 0; i < loader_create_info.enabledLayerCount; i++) {
+                const char *device_layer_names = loader_create_info.ppEnabledLayerNames[i];
+
+                if (strcmp(device_layer_names, inst->enabled_layer_names[i]) != 0) {
+                    invalid_device_layer_usage = true;
+                    break;
+                }
+            }
+        }
+
+        if (invalid_device_layer_usage) {
+            loader_log(
+                inst, VULKAN_LOADER_WARN_BIT, 0,
+                "loader_create_device_chain: Using deprecated and ignored 'ppEnabledLayerNames' member of 'VkDeviceCreateInfo' "
+                "when creating a Vulkan device.");
+        }
+    }
+
     // Before we continue, we need to find out if the KHR_device_group extension is in the enabled list.  If it is, we then
     // need to look for the corresponding VkDeviceGroupDeviceCreateInfoKHR struct in the device list.  This is because we
     // need to replace all the incoming physical device values (which are really loader trampoline physical device values)
@@ -5809,6 +5851,15 @@ VKAPI_ATTR void VKAPI_CALL terminator_DestroyInstance(VkInstance instance, const
     }
     loader_free_dev_ext_table(ptr_instance);
     loader_free_phys_dev_ext_table(ptr_instance);
+
+    for (uint32_t i = 0, n = ptr_instance->enabled_layer_count; i < n; ++i) {
+        loader_instance_heap_free(ptr_instance, ptr_instance->enabled_layer_names[i]);
+    }
+
+    if (ptr_instance->enabled_layer_count > 0) {
+        loader_instance_heap_free(ptr_instance, ptr_instance->enabled_layer_names);
+        memset(&ptr_instance->enabled_layer_names, 0, sizeof(ptr_instance->enabled_layer_names));
+    }
 }
 
 VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice(VkPhysicalDevice physicalDevice, const VkDeviceCreateInfo *pCreateInfo,

diff --git a/loader/loader_common.h b/loader/loader_common.h
index 5e5adb2..d1b770f 100644 (file)
--- a/loader/loader_common.h
+++ b/loader/loader_common.h
@@ -266,6 +266,9 @@ struct loader_instance {
 
     struct loader_msg_callback_map_entry *icd_msg_callback_map;
 
+    uint32_t enabled_layer_count;
+    char **enabled_layer_names;
+
     struct loader_layer_list instance_layer_list;
     bool override_layer_present;
 

diff --git a/tests/framework/test_environment.cpp b/tests/framework/test_environment.cpp
index 2e8fd74..11e64f7 100644 (file)
--- a/tests/framework/test_environment.cpp
+++ b/tests/framework/test_environment.cpp
@@ -194,6 +194,11 @@ void InstWrapper::CheckCreate(VkResult result_to_check) {
     ASSERT_EQ(result_to_check, functions->vkCreateInstance(create_info.get(), callbacks, &inst));
 }
 
+void InstWrapper::CheckCreateWithInfo(InstanceCreateInfo& create_info, VkResult result_to_check) {
+    ASSERT_EQ(result_to_check, functions->vkCreateInstance(create_info.get(), callbacks, &inst));
+}
+
+
 std::vector<VkPhysicalDevice> InstWrapper::GetPhysDevs(uint32_t phys_dev_count, VkResult result_to_check) {
     uint32_t physical_count = phys_dev_count;
     std::vector<VkPhysicalDevice> physical_devices;

diff --git a/tests/framework/test_environment.h b/tests/framework/test_environment.h
index 78577e3..6043250 100644 (file)
--- a/tests/framework/test_environment.h
+++ b/tests/framework/test_environment.h
@@ -272,6 +272,7 @@ struct InstWrapper {
 
     // Construct this VkInstance using googletest to assert if it succeeded
     void CheckCreate(VkResult result_to_check = VK_SUCCESS);
+    void CheckCreateWithInfo(InstanceCreateInfo& create_info, VkResult result_to_check = VK_SUCCESS);
 
     // Convenience
     operator VkInstance() { return inst; }

diff --git a/tests/loader_regression_tests.cpp b/tests/loader_regression_tests.cpp
index ed0cffb..0f1d436 100644 (file)
--- a/tests/loader_regression_tests.cpp
+++ b/tests/loader_regression_tests.cpp
@@ -969,6 +969,95 @@ TEST(CreateDevice, LayersNotPresent) {
     dev.CheckCreate(phys_dev);
 }
 
+// Device layers are deprecated.
+// Ensure that no error occur if instance and device are created with the same list of layers.
+// https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#extendingvulkan-layers-devicelayerdeprecation
+TEST(CreateDevice, MatchInstanceAndDeviceLayers) {
+    FrameworkEnvironment env{};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2));
+    env.get_test_icd().physical_devices.emplace_back("physical_device_0");
+
+    const char* layer_name = "TestLayer";
+    env.add_explicit_layer(
+        ManifestLayer{}.add_layer(
+            ManifestLayer::LayerDescription{}.set_name(layer_name).set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)),
+        "test_layer.json");
+
+    InstWrapper inst{env.vulkan_functions};
+    inst.create_info.add_layer(layer_name);
+    inst.CheckCreate();
+
+    VkPhysicalDevice phys_dev = inst.GetPhysDev();
+
+    DeviceWrapper dev{inst};
+    dev.create_info.add_layer(layer_name).add_device_queue(DeviceQueueCreateInfo{}.add_priority(0.0f));
+
+    dev.CheckCreate(phys_dev);
+}
+
+// Device layers are deprecated.
+// Ensure that a message is generated when instance and device are created with different list of layers.
+// At best , the user can list only instance layers in the device layer list
+// https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#extendingvulkan-layers-devicelayerdeprecation
+TEST(CreateDevice, UnmatchInstanceAndDeviceLayers) {
+    FrameworkEnvironment env{};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2));
+    env.get_test_icd().physical_devices.emplace_back("physical_device_0");
+
+    const char* layer_name = "TestLayer";
+    env.add_explicit_layer(
+        ManifestLayer{}.add_layer(
+            ManifestLayer::LayerDescription{}.set_name(layer_name).set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)),
+        "test_layer.json");
+
+    DebugUtilsLogger debug_log{VK_DEBUG_UTILS_MESSAGE_SEVERITY_WARNING_BIT_EXT};
+    InstWrapper inst{env.vulkan_functions};
+    FillDebugUtilsCreateDetails(inst.create_info, debug_log);
+    inst.CheckCreate();
+
+    DebugUtilsWrapper log{inst, VK_DEBUG_UTILS_MESSAGE_SEVERITY_WARNING_BIT_EXT};
+    CreateDebugUtilsMessenger(log);
+
+    VkPhysicalDevice phys_dev = inst.GetPhysDev();
+
+    DeviceWrapper dev{inst};
+    dev.create_info.add_layer(layer_name).add_device_queue(DeviceQueueCreateInfo{}.add_priority(0.0f));
+
+    dev.CheckCreate(phys_dev);
+
+    ASSERT_TRUE(log.find("loader_create_device_chain: Using deprecated and ignored 'ppEnabledLayerNames' member of 'VkDeviceCreateInfo' when creating a Vulkan device."));
+}
+
+// Device layers are deprecated.
+// Ensure that when VkInstanceCreateInfo is deleted, the check of the instance layer lists is running correctly during VkDevice creation
+// https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#extendingvulkan-layers-devicelayerdeprecation
+TEST(CreateDevice, CheckCopyOfInstanceLayerNames) {
+    FrameworkEnvironment env{};
+    env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2));
+    env.get_test_icd().physical_devices.emplace_back("physical_device_0");
+
+    const char* layer_name = "TestLayer";
+    env.add_explicit_layer(
+        ManifestLayer{}.add_layer(
+            ManifestLayer::LayerDescription{}.set_name(layer_name).set_lib_path(TEST_LAYER_PATH_EXPORT_VERSION_2)),
+        "test_layer.json");
+
+    InstWrapper inst{env.vulkan_functions};
+    {
+        // We intentionally create a local InstanceCreateInfo that goes out of scope at the } so that when dev.CheckCreate is called the layer name pointers are no longer valid
+        InstanceCreateInfo create_info{};
+        create_info.add_layer(layer_name);
+        inst.CheckCreateWithInfo(create_info);
+    }
+
+    VkPhysicalDevice phys_dev = inst.GetPhysDev();
+
+    DeviceWrapper dev{inst};
+    dev.create_info.add_layer(layer_name).add_device_queue(DeviceQueueCreateInfo{}.add_priority(0.0f));
+
+    dev.CheckCreate(phys_dev);
+}
+
 TEST(CreateDevice, ConsecutiveCreate) {
     FrameworkEnvironment env{};
     env.add_icd(TestICDDetails(TEST_ICD_PATH_VERSION_2));