Fix linux 32 bit unknown function error handling #1036
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CI Vulkan-Loader build # 1556 running. |
|
CI Vulkan-Loader build # 1556 passed. |
|
Hi no, now get a fail build the ASM code https://gist.github.com/sl1pkn07/6923bf64f1cdf663c7258f36f77907b2 PS: the environmet ASFLAGS is gone. no one refenced in the loader/CMakeLists.txt, but is still in the BUILD.md greetints |
Line 135:
Should it be? My understanding from those docs is that ASFLAGS is something the user must configure to set 32 bit builds. The CMakelists doesn't need to know about the flags for 32 bit builds to work. |
|
Hi ASFLAGS is was used by this commit c34b5ae i'm not sure since when, but now that part of code is gone. I have not worried about it before because working, but now fails. (seems now have/use other method to get the work) also is referenced on this #225 when I have a ASM build issues. but now, with the flag gone, i'm lost for get what happen now for reference. my last sucess build dated on 5 sep, so is this commit fdfdef6 i'm goin tomorrow do more test greetings |
|
seems clang is more descriptive |
|
That error looks like it the 64 bit code is trying to be used in 32 bit mode? |
|
seems yes. as note, i'm trying, all time, build 32bits code on 64bits machine. so seems the asm code detect the 32bit compillation as 64 bits code |
|
Using your build code, I was able to build 32 bit in 64 bit OS, and I 'know' it was using the 32 bit asm code because that was where the compiler error was. What you just posted indicates the error happening in the 64 bit code, which shouldn't happen. Is |
mmmmm |
|
That code should be running on windows, not linux. However further down is the corresponding command that uses very similar syntax. Vulkan-Loader/loader/CMakeLists.txt Line 231 in ae54ca0
|
x86_64 |
|
That would be it, now to figure out 'how to make it not be that'. I'm not at my linux box atm so I can't check what I get when I do the 32 bit build. |
|
in my "sed" things, I've change i will try this |
|
something similar like this diff --git a/loader/CMakeLists.txt b/loader/CMakeLists.txt
index 0be1c5df4..a7a1f168f 100644
--- a/loader/CMakeLists.txt
+++ b/loader/CMakeLists.txt
@@ -228,15 +228,28 @@ else() # i.e.: Linux
find_package(PythonInterp REQUIRED)
# Run parse_asm_values.py on asm_offset's assembly file to generate the gen_defines.asm, which the asm code depends on
+ if("${CMAKE_SIZEOF_VOID_P}" EQUAL "8")
+ # If build on 64bits host
+ set(_target_asm ${CMAKE_SYSTEM_PROCESSOR})
+ else()
+ # Cross if build 32bits on 64bits host
+ if(${CMAKE_SYSTEM_PROCESSOR} STREQUAL "x86_64" OR ${CMAKE_SYSTEM_PROCESSOR} STREQUAL "amd64")
+ set(_target_asm x86)
+ endif()
+ # If build on 32bits host
+ if(${CMAKE_SYSTEM_PROCESSOR} STREQUAL "x86" OR ${CMAKE_SYSTEM_PROCESSOR} MATCHES "^i.86$")
+ set(_target_asm ${CMAKE_SYSTEM_PROCESSOR})
+ endif()
+ endif()
add_custom_command(TARGET asm_offset POST_BUILD
COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_SOURCE_DIR}/scripts/parse_asm_values.py "$<TARGET_FILE_DIR:asm_offset>/gen_defines.asm"
- "${ASM_OFFSET_INTERMEDIATE_LOCATION}" "GAS" "${CMAKE_CXX_COMPILER_ID}" "${CMAKE_SYSTEM_PROCESSOR}"
+ "${ASM_OFFSET_INTERMEDIATE_LOCATION}" "GAS" "${CMAKE_CXX_COMPILER_ID}" "${_target_asm}"
BYPRODUCTS gen_defines.asm
)
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 ${_target_asm} GAS assembler\n${ASM_FAILURE_MSG}")
else()
message(WARNING "Assembly sources have been disabled\n${ASM_FAILURE_MSG}")
endif() |
|
tested with the patch posted avobe remain 32bits test passed except 406 as note.the test 11 fails, in 64 and 32 bits (both have the same output) greetings |
|
Thank you for working with me. I figured out why the parse_asm_values.py doesn't work (while the old asm_offset.exe does). The asm_offset.exe checks I'll fix up this PR to include checking the CMAKE_SYSTEM_PROCESSOR and make parse_asm_values.py print the correct header (eg, include X86_64 or not). |
|
Hi. thanks for all (offtopic) i need a little info about this: Archlinux sets build with LTO by default. if i understand good, i need disable the LTO when build the project in 32bits mode when use 64bits host? greetings |
charles-lunarg
force-pushed
the
fix_linux_32bit_unknown_function_error_path
branch
from
afdd0df
to
7f063c1
Compare
|
CI Vulkan-Loader build queued with queue ID 18502. |
|
Hi, I just pushed up the 'fixed' version. Please check it out and make sure it actually works. To answer your LTO question, you should be able to use LTO while building 32 bit on x64 because unless CMAKE_CROSSCOMPILNG is TRUE, the build will execute asm_offset.exe. This removes the issues that Edit: To be clear, from what I can tell CMAKE_CROSSCOMPILNG is FALSE when doing 32 bit builds on an x64 machine. So this means the cross compilation fallback path is not used (and this is the path that can't handle LTO). |
|
CI Vulkan-Loader build # 1561 passed. |
|
thanks a lot fot the explanation the updated #1036 have the same output. failed test 406 output with ctest --verbose test 11 greetings |
|
gdb 11 full bt gdb 406 have 'no stack' |
charles-lunarg
force-pushed
the
fix_linux_32bit_unknown_function_error_path
branch
from
7f063c1
to
651da0c
Compare
|
CI Vulkan-Loader build queued with queue ID 20246. |
|
CI Vulkan-Loader build # 1566 running. |
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.
charles-lunarg
force-pushed
the
fix_linux_32bit_unknown_function_error_path
branch
from
651da0c
to
6c97b39
Compare
|
CI Vulkan-Loader build queued with queue ID 20260. |
|
CI Vulkan-Loader build # 1567 running. |
|
CI Vulkan-Loader build # 1567 passed. |
|
Hi i found "strange"? behavior is normal the test 406 (32bits) passed with |
|
I was able to build & run the github actions script you wrote up (with some modifications) and found that test 406 failed in debug mode, with a similar error to you. Failing in release vs debug mode isn't good, so I definitely need to check that out. This PR definitely isn't ready to merge, as the change doesn't seem to fix the unknown function path. |
|
On further inspection, the test only fails when address sanitizer is active. I suspect that there is a bug in it but I have no proof. Because I can get the code to behave like I expect without ASAN active, I'm going to note it as a 'known issue' rather than hold up this fix. |
charles-lunarg
deleted the
fix_linux_32bit_unknown_function_error_path
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Also rename the error to note that its a function which isn't supported, not an extension.
@sl1pkn07 - can you confirm this solves your issue? From what I can tell, the 32 bit unknown function handling worked except in the error handling path. This resolves that last issue.
Fixes #877