From f57610da139e2a5acebb64db31edd728f9283db1 Mon Sep 17 00:00:00 2001 From: Charles Giessen Date: Fri, 21 Oct 2022 12:01:45 -0600 Subject: Fix linux 32 bit unknown function error handling The issue was that passing a string to loader_log wouldn't work, likely due to relocation issues. Workaround is to create a bespoke function that contained the format string embedded in it. Additionally, CMAKE_SYSTEM_PROCESSOR is incorrect in the case of x86 builds on x64. Checking for sizeof(void*) == 8 allows distinguishing building for 32 bit and 64 bit, and so the correct gen_defines.asm is created while cross compiling. Also rename the error to note that its a function which isn't supported, not an extension. --- loader/CMakeLists.txt | 13 +++++++++++-- loader/log.c | 5 +++++ loader/log.h | 5 +++++ loader/unknown_ext_chain.c | 2 +- loader/unknown_ext_chain_gas_aarch64.S | 2 +- loader/unknown_ext_chain_gas_x86.S | 7 +++---- loader/unknown_ext_chain_masm.asm | 2 +- scripts/parse_asm_values.py | 4 +++- tests/loader_unknown_ext_tests.cpp | 2 +- 9 files changed, 31 insertions(+), 11 deletions(-) diff --git a/loader/CMakeLists.txt b/loader/CMakeLists.txt index eebb83a33..62f8e2c40 100644 --- a/loader/CMakeLists.txt +++ b/loader/CMakeLists.txt @@ -222,6 +222,15 @@ else() # i.e.: Linux endif() endif() + # When compiling for x86 on x64, we can't use CMAKE_SYSTEM_PROCESSOR to determine which architecture to use, + # Instead, check the size of void* and if its 4, set ASM_OFFSET_SYSTEM_PROCESSOR to x86 + # Note - there is no 32 bit arm assembly code, so this only applies to x86 currently. + if("${CMAKE_SIZEOF_VOID_P}" EQUAL "8") + set(ASM_OFFSET_SYSTEM_PROCESSOR ${CMAKE_SYSTEM_PROCESSOR}) # x86_64 or aarch64 + else() + set(ASM_OFFSET_SYSTEM_PROCESSOR "x86") + endif() + if(ASSEMBLER_WORKS) add_executable(asm_offset asm_offset.c) target_link_libraries(asm_offset loader_specific_options) @@ -243,14 +252,14 @@ else() # i.e.: Linux # Run parse_asm_values.py on asm_offset's assembly file to generate the gen_defines.asm, which the asm code depends on add_custom_command(TARGET asm_offset POST_BUILD COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_SOURCE_DIR}/scripts/parse_asm_values.py "$/gen_defines.asm" - "${ASM_OFFSET_INTERMEDIATE_LOCATION}" "GAS" "${CMAKE_CXX_COMPILER_ID}" "${CMAKE_SYSTEM_PROCESSOR}" + "${ASM_OFFSET_INTERMEDIATE_LOCATION}" "GAS" "${CMAKE_CXX_COMPILER_ID}" "${ASM_OFFSET_SYSTEM_PROCESSOR}" BYPRODUCTS gen_defines.asm ) endif() add_custom_target(loader_asm_gen_files DEPENDS gen_defines.asm) else() if(USE_GAS) - message(WARNING "Could not find working ${CMAKE_SYSTEM_PROCESSOR} GAS assembler\n${ASM_FAILURE_MSG}") + message(WARNING "Could not find working ${ASM_OFFSET_SYSTEM_PROCESSOR} GAS assembler\n${ASM_FAILURE_MSG}") else() message(WARNING "Assembly sources have been disabled\n${ASM_FAILURE_MSG}") endif() diff --git a/loader/log.c b/loader/log.c index 6b68bd0dc..ab64f6336 100644 --- a/loader/log.c +++ b/loader/log.c @@ -229,3 +229,8 @@ void loader_log(const struct loader_instance *inst, VkFlags msg_type, int32_t ms assert(false); } } + +void loader_log_asm_function_not_supported(const struct loader_instance *inst, VkFlags msg_type, int32_t msg_code, + const char *func_name) { + loader_log(inst, msg_type, msg_code, "Function %s not supported for this physical device", func_name); +} diff --git a/loader/log.h b/loader/log.h index 84bf6e199..b45a26494 100644 --- a/loader/log.h +++ b/loader/log.h @@ -49,3 +49,8 @@ uint32_t loader_get_debug_level(void); // Logs a message to stderr // May output to DebugUtils if the instance isn't null and the extension is enabled. void loader_log(const struct loader_instance *inst, VkFlags msg_type, int32_t msg_code, const char *format, ...); + +// Used for the assembly code to emit an specific error message +// This is a work around for linux 32 bit error handling not passing relocatable strings correctly +void loader_log_asm_function_not_supported(const struct loader_instance *inst, VkFlags msg_type, int32_t msg_code, + const char *func_name); diff --git a/loader/unknown_ext_chain.c b/loader/unknown_ext_chain.c index 3ce7f3b1c..93d71bfb2 100644 --- a/loader/unknown_ext_chain.c +++ b/loader/unknown_ext_chain.c @@ -48,7 +48,7 @@ struct loader_icd_term *icd_term = phys_dev_term->this_icd_term; \ struct loader_instance *inst = (struct loader_instance *)icd_term->this_instance; \ if (NULL == icd_term->phys_dev_ext[num]) { \ - loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0, "Extension %s not supported for this physical device", \ + loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0, "Function %s not supported for this physical device", \ inst->phys_dev_ext_disp_functions[num]); \ } \ icd_term->phys_dev_ext[num](phys_dev_term->phys_dev); \ diff --git a/loader/unknown_ext_chain_gas_aarch64.S b/loader/unknown_ext_chain_gas_aarch64.S index 5a7d98a07..b2c65ee77 100644 --- a/loader/unknown_ext_chain_gas_aarch64.S +++ b/loader/unknown_ext_chain_gas_aarch64.S @@ -76,7 +76,7 @@ vkdev_ext\num: .data termin_error_string: -.string "Extension %s not supported for this physical device" +.string "Function %s not supported for this physical device" .text diff --git a/loader/unknown_ext_chain_gas_x86.S b/loader/unknown_ext_chain_gas_x86.S index 496208ddf..759375c23 100644 --- a/loader/unknown_ext_chain_gas_x86.S +++ b/loader/unknown_ext_chain_gas_x86.S @@ -100,12 +100,11 @@ vkPhysDevExtTermin\num: jmp [eax + (DISPATCH_OFFSET_ICD_TERM + (PTR_SIZE * \num))] # Jump to the next function in the chain terminError\num: mov eax, dword ptr [eax + INSTANCE_OFFSET_ICD_TERM] # Load the loader_instance into eax - push dword ptr [eax + (FUNCTION_OFFSET_INSTANCE + (CHAR_PTR_SIZE * \num))] # Push the func name (fifth arg) - push offset termin_error_string@GOT # Push the error string (fourth arg) + push dword ptr [eax + (FUNCTION_OFFSET_INSTANCE + (CHAR_PTR_SIZE * \num))] # Push the func name (fourth arg) push 0 # Push zero (third arg) push VULKAN_LOADER_ERROR_BIT # Push the error logging bit (second arg) push eax # Push the loader_instance (first arg) - call loader_log # Log the error message before we crash + call loader_log_asm_function_not_supported # Log the error message before we crash add esp, 20 # Clean up the args mov eax, 0 jmp eax # Crash intentionally by jumping to address zero @@ -129,7 +128,7 @@ vkdev_ext\num: .data termin_error_string: -.string "Extension %s not supported for this physical device" +.string "Function %s not supported for this physical device" .text diff --git a/loader/unknown_ext_chain_masm.asm b/loader/unknown_ext_chain_masm.asm index e33858aeb..46d13de47 100644 --- a/loader/unknown_ext_chain_masm.asm +++ b/loader/unknown_ext_chain_masm.asm @@ -119,7 +119,7 @@ endm ENDIF .const - termin_error_string db 'Extension %s not supported for this physical device', 0 + termin_error_string db 'Function %s not supported for this physical device', 0 .code diff --git a/scripts/parse_asm_values.py b/scripts/parse_asm_values.py index bff263d2a..df7a0460c 100644 --- a/scripts/parse_asm_values.py +++ b/scripts/parse_asm_values.py @@ -35,7 +35,8 @@ source_asm_file = sys.argv[2] assembler_type = sys.argv[3] # Whether we are using gcc, clang, or msvc compiler = sys.argv[4] -# taken from CMAKE_SYSTEM_PROCESSOR - x86_64 or aarch64 +# taken from CMAKE_SYSTEM_PROCESSOR - x86_64, aarch64, or x86 +# Only used with GAS - MASM doesn't need this, as it has its own way to determine x86 vs x64 arch = sys.argv[5] if destination_file is None or source_asm_file is None or assembler_type is None or compiler is None or arch is None: @@ -71,6 +72,7 @@ with open(destination_file, "w", encoding="utf-8") as dest: dest.write(".set X86_64, 1\n") elif arch == "aarch64": dest.write(".set AARCH_64, 1\n") + # Nothing to write in the x86 case for d in defines: match = None diff --git a/tests/loader_unknown_ext_tests.cpp b/tests/loader_unknown_ext_tests.cpp index 294251afe..bdce9b68c 100644 --- a/tests/loader_unknown_ext_tests.cpp +++ b/tests/loader_unknown_ext_tests.cpp @@ -445,7 +445,7 @@ TEST(UnknownFunctionDeathTests, PhysicalDeviceFunctionErrorPath) { decltype(custom_physical_device_functions::func_zero)* returned_func_i = env.vulkan_functions.load(inst.inst, function_names.at(0).c_str()); ASSERT_NE(returned_func_i, nullptr); - ASSERT_DEATH(returned_func_i(phys_dev_to_use, 0), "Extension vkNotIntRealFuncTEST_0 not supported for this physical device"); + ASSERT_DEATH(returned_func_i(phys_dev_to_use, 0), "Function vkNotIntRealFuncTEST_0 not supported for this physical device"); } TEST(UnknownFunction, PhysicalDeviceFunctionWithImplicitLayerImplementation) { -- cgit v1.2.3