Address feedback

Author: jpr42

loader/asm_offset.c

@@ -171,5 +171,10 @@ int main(int argc, char **argv) {
         }
     }
     // NOLINTEND(cert-err33-c)
+    if (ferror(file)) {
+        fclose(file);
+        fprintf(stderr, "Error: failed to write gen_defines.asm\n");
+        return 1;
+    }
     return fclose(file);
 }

loader/dev_ext_trampoline.c

@@ -286,7 +286,6 @@ VKAPI_ATTR void VKAPI_CALL vkdev_ext248(VkDevice device) ASM_NAME("vkdev_ext248"
 VKAPI_ATTR void VKAPI_CALL vkdev_ext249(VkDevice device) ASM_NAME("vkdev_ext249");
 
 void *loader_get_dev_ext_trampoline(uint32_t index) {
-    // NOLINTNEXTLINE(bugprone-switch-missing-default-case) - falls through to the return NULL below
     switch (index) {
 #define CASE_HANDLE(num) case num: return vkdev_ext##num
         CASE_HANDLE(0);
@@ -539,7 +538,6 @@ void *loader_get_dev_ext_trampoline(uint32_t index) {
         CASE_HANDLE(247);
         CASE_HANDLE(248);
         CASE_HANDLE(249);
+        default: return NULL;
     }
-
-    return NULL;
 }

loader/loader.c

@@ -5805,24 +5805,23 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateInstance(const VkInstanceCreateI
 
     struct loader_instance *ptr_instance = (struct loader_instance *)*pInstance;
     if (NULL == ptr_instance) {
-        loader_log(ptr_instance, VULKAN_LOADER_WARN_BIT, 0,
+        loader_log(NULL, VULKAN_LOADER_ERROR_BIT, 0,
                    "terminator_CreateInstance: Loader instance pointer null encountered.  Possibly set by active layer. (Policy "
                    "#LLP_LAYER_21)");
+        abort();
     } else if (LOADER_MAGIC_NUMBER != ptr_instance->magic) {
-        loader_log(ptr_instance, VULKAN_LOADER_WARN_BIT, 0,
+        loader_log(ptr_instance, VULKAN_LOADER_ERROR_BIT, 0,
                    "terminator_CreateInstance: Instance pointer (%p) has invalid MAGIC value 0x%08" PRIx64
                    ". Instance value possibly "
                    "corrupted by active layer (Policy #LLP_LAYER_21).  ",
                    ptr_instance, ptr_instance->magic);
+        abort();
     }
 
     // Save the application version if it has been modified - layers sometimes needs features in newer API versions than
     // what the application requested, and thus will increase the instance version to a level that suites their needs.
     if (pCreateInfo->pApplicationInfo && pCreateInfo->pApplicationInfo->apiVersion) {
         loader_api_version altered_version = loader_make_version(pCreateInfo->pApplicationInfo->apiVersion);
-        // per LLP_LAYER_21 (docs/LoaderLayerInterface.md), a layer that clobbers pInstance before calling down is
-        // documented to crash the loader; this is not recoverable here.
-        // NOLINTNEXTLINE(clang-analyzer-core.NullDereference)
         if (altered_version.major != ptr_instance->app_api_version.major ||
             altered_version.minor != ptr_instance->app_api_version.minor) {
             ptr_instance->app_api_version = altered_version;
@@ -5857,14 +5856,12 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateInstance(const VkInstanceCreateI
     } else {
         for (uint32_t j = 0; j < pCreateInfo->enabledExtensionCount; j++) {
             if (!strcmp(pCreateInfo->ppEnabledExtensionNames[j], VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME)) {
-                // NOLINTNEXTLINE(clang-analyzer-core.NullDereference) - see LLP_LAYER_21 note above
                 ptr_instance->supports_get_dev_prop_2 = true;
                 break;
             }
         }
     }
 
-    // NOLINTNEXTLINE(clang-analyzer-core.NullDereference) - see LLP_LAYER_21 note above
     for (uint32_t i = 0; i < ptr_instance->icd_tramp_list.count; i++) {
         icd_term = loader_icd_add(ptr_instance, &ptr_instance->icd_tramp_list.scanned_list[i]);
         if (NULL == icd_term) {
@@ -6220,21 +6217,20 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice(VkPhysicalDevice physical
     VkDeviceGroupDeviceCreateInfo *caller_dgci = NULL;
 
     if (NULL == dev) {
-        loader_log(icd_term->this_instance, VULKAN_LOADER_WARN_BIT, 0,
+        loader_log(icd_term->this_instance, VULKAN_LOADER_ERROR_BIT, 0,
                    "terminator_CreateDevice: Loader device pointer null encountered.  Possibly set by active layer. (Policy "
                    "#LLP_LAYER_22)");
+        abort();
     } else if (DEVICE_DISP_TABLE_MAGIC_NUMBER != dev->loader_dispatch.core_dispatch.magic) {
-        loader_log(icd_term->this_instance, VULKAN_LOADER_WARN_BIT, 0,
+        loader_log(icd_term->this_instance, VULKAN_LOADER_ERROR_BIT, 0,
                    "terminator_CreateDevice: Device pointer (%p) has invalid MAGIC value 0x%08" PRIx64
                    ". The expected value is "
                    "0x10ADED040410ADED. Device value possibly "
                    "corrupted by active layer (Policy #LLP_LAYER_22).  ",
                    dev, dev->loader_dispatch.core_dispatch.magic);
+        abort();
     }
 
-    // per LLP_LAYER_22 (docs/LoaderLayerInterface.md), a layer that clobbers pDevice before calling down is
-    // documented to crash the loader; this is not recoverable here.
-    // NOLINTNEXTLINE(clang-analyzer-core.NullDereference)
     dev->phys_dev_term = phys_dev_term;
 
     icd_exts.list = NULL;

loader/loader_environment.c

@@ -519,36 +519,32 @@ VkResult loader_add_environment_layers(struct loader_instance *inst, const char
             continue;
         }
 
-        // We found a layer we're interested in, but has it been disabled...
-        bool adding;
+        // A layer matching the enable filter is force-enabled even if it also matches the disable filter - this mirrors
+        // VK_INSTANCE_LAYERS, which VK_LOADER_LAYERS_ENABLE is a generalization of, and which overrides disables.
+        // Also make sure the layer isn't already in the output_list, skip adding it if it is.
+        bool force_enabled = check_name_matches_filter_environment_var(source_prop->info.layerName, &filters->enable_filter) &&
+                             !loader_find_layer_name_in_list(source_prop->info.layerName, target_list);
+
         bool is_implicit = (0 == (source_prop->type_flags & VK_LAYER_TYPE_FLAG_EXPLICIT_LAYER));
         bool disabled_by_type =
             (is_implicit) ? (filters->disable_filter.disable_all_implicit) : (filters->disable_filter.disable_all_explicit);
-        if ((filters->disable_filter.disable_all || disabled_by_type ||
+        if (!force_enabled &&
+            (filters->disable_filter.disable_all || disabled_by_type ||
              check_name_matches_filter_environment_var(source_prop->info.layerName, &filters->disable_filter.additional_filters)) &&
             !check_name_matches_filter_environment_var(source_prop->info.layerName, &filters->allow_filter)) {
             loader_log(inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0,
                        "Layer \"%s\" ignored because it has been disabled by env var \'%s\'", source_prop->info.layerName,
                        VK_LAYERS_DISABLE_ENV_VAR);
+            continue;
         }
 
-        // Whether a layer is force-enabled only depends on the check below - the disable check above exists purely to log
-        // why a layer was skipped, since a layer is only ever added by this function via VK_LOADER_LAYERS_ENABLE.
-        // Also make sure the layer isn't already in the output_list, skip adding it if it is.
-        if (check_name_matches_filter_environment_var(source_prop->info.layerName, &filters->enable_filter) &&
-            !loader_find_layer_name_in_list(source_prop->info.layerName, target_list)) {
-            adding = true;
-            // Only way is_substring is true is if there are enable variables.  If that's the case, and we're past the
-            // above, we should indicate that it was forced on in this way.
-            loader_log(inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0,
-                       "Layer \"%s\" forced enabled due to env var \'%s\'", source_prop->info.layerName, VK_LAYERS_ENABLE_ENV_VAR);
-        } else {
-            adding = false;
-        }
-
-        if (!adding) {
+        if (!force_enabled) {
             continue;
         }
+        // Only way is_substring is true is if there are enable variables.  If that's the case, and we're past the
+        // above, we should indicate that it was forced on in this way.
+        loader_log(inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0, "Layer \"%s\" forced enabled due to env var \'%s\'",
+                   source_prop->info.layerName, VK_LAYERS_ENABLE_ENV_VAR);
 
         // If not a meta-layer, simply add it.
         if (0 == (source_prop->type_flags & VK_LAYER_TYPE_FLAG_META_LAYER)) {

loader/phys_dev_ext.c

@@ -549,7 +549,6 @@ VKAPI_ATTR void VKAPI_CALL vkPhysDevExtTermin249(VkPhysicalDevice) ASM_NAME("vkP
 
 
 void *loader_get_phys_dev_ext_tramp(uint32_t index) {
-    // NOLINTNEXTLINE(bugprone-switch-missing-default-case) - falls through to the return NULL below
     switch (index) {
 #define TRAMP_CASE_HANDLE(num) case num: return vkPhysDevExtTramp##num
         TRAMP_CASE_HANDLE(0);
@@ -802,12 +801,11 @@ void *loader_get_phys_dev_ext_tramp(uint32_t index) {
         TRAMP_CASE_HANDLE(247);
         TRAMP_CASE_HANDLE(248);
         TRAMP_CASE_HANDLE(249);
+        default: return NULL;
     }
-    return NULL;
 }
 
 void *loader_get_phys_dev_ext_termin(uint32_t index) {
-    // NOLINTNEXTLINE(bugprone-switch-missing-default-case) - falls through to the return NULL below
     switch (index) {
 #define TERM_CASE_HANDLE(num) case num: return vkPhysDevExtTermin##num
         TERM_CASE_HANDLE(0);
@@ -1060,6 +1058,6 @@ void *loader_get_phys_dev_ext_termin(uint32_t index) {
         TERM_CASE_HANDLE(247);
         TERM_CASE_HANDLE(248);
         TERM_CASE_HANDLE(249);
+        default: return NULL;
     }
-    return NULL;
 }

tests/loader_layer_tests.cpp

@@ -4191,6 +4191,9 @@ TEST(TestLayers, EnvironLayerEnableDisableExplicitLayer) {
     ASSERT_TRUE(env.debug_log.find_prefix_then_postfix("Insert instance layer", explicit_layer_name_3));
     ASSERT_TRUE(env.debug_log.find_prefix_then_postfix(explicit_layer_name_3, "forced enabled due to env var"));
     ASSERT_FALSE(env.debug_log.find_prefix_then_postfix(explicit_layer_name_3, "disabled because name matches filter of env var"));
+    // layer_3 also matches VK_LOADER_LAYERS_DISABLE ("*Second*"), but VK_LOADER_LAYERS_ENABLE overrides it - it should not
+    // also log that it was ignored as disabled.
+    ASSERT_FALSE(env.debug_log.find_prefix_then_postfix(explicit_layer_name_3, "ignored because it has been disabled"));
 
     // Disable all but enable 2
     // ------------------------------------------