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 c98d37cfecfe5d4844d0d419928ae2c3ee1d1c1c..37ae18053dcd4ce54f2e7dea928685f90fd1ac80 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 5e5adb22e76bad13c54eb738503ae39c63563437..d1b770fb5d363710b030ed51be42e0912ab9dd48 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 2e8fd74fcbaa284e4ac7ff0a748726d39305c7b7..11e64f7a45f39aec826db567e5f0bcf5cad2ea3c 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 78577e3109e05c1612c6934c6420c2d551989ea8..6043250d7c8d54516510656408371bf7d9767fcc 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 ed0cffb6af95359119bc8df50aa6031001512350..0f1d436f273eaa62e023c0451b9a0ce2e74ae273 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));