-
Notifications
You must be signed in to change notification settings - Fork 15k
[compiler-rt] Fix building on OpenBSD/amd64 #165086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
OpenBSD/amd64 does not use multi-lib. Enabling the CET support on amd64 exposed that CMake was using a multi-lib build on amd64. OpenBSD 64-bit platforms do not support multi-lib.
4545594 to
56feddb
Compare
|
@vitalybuka Ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this.
If the pointer size conditions really are necessary, then adding the OPENBSD definition seems like a better approach, although perhaps a better solution would be to work out why test_target_arch is not working on OBSD, and fixing that test macro.
If the problem with the test macro are resolvable, then it might be ok to retain the (NOT STREQUAL) version, but I would be perfectly ok with adding an OPENBSD definition and using that, though open to dissent from others :D
| add_default_target_arch(${COMPILER_RT_DEFAULT_TARGET_ARCH}) | ||
| elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "i[2-6]86|x86|amd64") | ||
| if(NOT MSVC) | ||
| if(NOT MSVC AND NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "OpenBSD") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this OpenBSD specific, or BSD in general and simply hasn't been hit on them yet?
If is OpenBSD specific is there a central config file that is included in all the cmake scripts where you could add something like
# I'm blindly assuming `DEFINED` is an operator and not a special form
# of `if`, if the latter this would become if () else() ...set ... endif()
if (NOT (DEFINED OPENBSD))
# I also do not know if CMake allows this, because I'm unsure what
# things are special forms vs operators, because I'm not a cmake person
set(OPENBSD ("${CMAKE_SYSTEM_NAME}" STREQUAL "OpenBSD"))
endif()Then OPENBSD and NOT OPENBSD become available and the script would read better.
Or maybe OPEN_BSD if that seems more appropriate.
I have the existence check because it seems possible that cmake might add a builtin like this in the future?
| elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "powerpc") | ||
| test_target_arch(powerpc "" "-m32") | ||
| test_target_arch(powerpc64 "" "-m64") | ||
| if(NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "OpenBSD") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these needed, my assumption is that test_target_arch should be handling this automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(edit: I had misremembered the x86 path when I wrote the original comment)
| elseif("${COMPILER_RT_DEFAULT_TARGET_ARCH}" MATCHES "sparc") | ||
| test_target_arch(sparc "" "-m32") | ||
| test_target_arch(sparcv9 "" "-m64") | ||
| if(NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "OpenBSD") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
OpenBSD/amd64 does not use multi-lib. Enabling the CET support on amd64
exposed that CMake was using a multi-lib build on amd64.
OpenBSD 64-bit platforms do not support multi-lib.