[CMake] Handle clang in MSVC mode in GetHostTriple #116701
Conversation
With this[1] build of clang out of the box cmake fails. On a Windows on Arm with just cmake, ninja, git, python and clang[1] can be built with[2] this change. check-clang doesn't work yet. [1] https://github.com/llvm/llvm-project/releases/download/llvmorg-19.1.2/LLVM-19.1.2-woa64.exe [2] cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang;lld" -DLLVM_ENABLE_RUNTIMES="compiler-rt" -DLLVM_TARGETS_TO_BUILD="AArch64" -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF ../llvm && ninja clang compiler-rt Change-Id: I8849489fc043e63efa57d2d45fb596d422d6baaf
| if( CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM64.*" ) | ||
| set( value "aarch64-pc-windows-msvc" ) | ||
| elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM.*" ) | ||
| set( value "armv7-pc-windows-msvc" ) | ||
| elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "x64" ) | ||
| set( value "x86_64-pc-windows-msvc" ) | ||
| elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "X86" ) | ||
| set( value "i686-pc-windows-msvc" ) | ||
| elseif( CMAKE_SIZEOF_VOID_P EQUAL 8 ) | ||
| set( value "x86_64-pc-windows-msvc" ) | ||
| else() | ||
| set( value "i686-pc-windows-msvc" ) |
There was a problem hiding this comment.
We usually don't use spaces around parens.
| if( CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM64.*" ) | |
| set( value "aarch64-pc-windows-msvc" ) | |
| elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM.*" ) | |
| set( value "armv7-pc-windows-msvc" ) | |
| elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "x64" ) | |
| set( value "x86_64-pc-windows-msvc" ) | |
| elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "X86" ) | |
| set( value "i686-pc-windows-msvc" ) | |
| elseif( CMAKE_SIZEOF_VOID_P EQUAL 8 ) | |
| set( value "x86_64-pc-windows-msvc" ) | |
| else() | |
| set( value "i686-pc-windows-msvc" ) | |
| if(CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM64.*") | |
| set(value "aarch64-pc-windows-msvc") | |
| elseif(CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM.*") | |
| set(value "armv7-pc-windows-msvc") | |
| elseif(CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "x64") | |
| set(value "x86_64-pc-windows-msvc") | |
| elseif(CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "X86") | |
| set(value "i686-pc-windows-msvc") | |
| elseif(CMAKE_SIZEOF_VOID_P EQUAL 8) | |
| set(value "x86_64-pc-windows-msvc") | |
| else() | |
| set(value "i686-pc-windows-msvc") |
| elseif( CMAKE_SIZEOF_VOID_P EQUAL 8 ) | ||
| set( value "x86_64-pc-windows-msvc" ) | ||
| else() | ||
| set( value "i686-pc-windows-msvc" ) |
There was a problem hiding this comment.
Is it safe to assume x86/x64 when CMAKE_C_COMPILER_ARCHITECTURE_ID isn't x86 or x64? Can we report an error instead?
mstorsjo
left a comment
mstorsjo
left a comment
There was a problem hiding this comment.
The PR subject/description reads a bit weird, can you rephrase it?
I guess "build clang with..." means "make sure that building clang works", but it feels a bit terse and off the point here, I'd rather focus on e.g. "fix GetHostTriple for clang in MSVC mode, with the GCC like command line driver mode" or something like that?
(Also, the second sentence in the PR description feels a bit off, that could also use a couple more words to be more readable.)
| endif() | ||
| else( MSVC ) | ||
| if(CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS) | ||
| if(CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS AND CMAKE_C_COMPILER_ID MATCHES "Clang" ) |
There was a problem hiding this comment.
Why is this needed here? When operating in MSVC mode, CMake usually sets the MSVC CMake variable, so this should hit the existing codepath at lines 5-18 above? And this is a bit weird, because CMake usually doesn't set COMPILER_ARCHITECTURE_ID for Clang (or GCC), but only for MSVC. I guess CMake might set COMPILER_ARCHITECTURE_ID for Clang-cl when operating in MSVC mode, but if we're invoking it as clang, not clang-cl so that the MSVC variable isn't set, it feels odd that CMake still would be setting COMPILER_ARCHITECTURE_ID.
Also I don't quite see why this needs to be as a separate step within the else() clause here, why can't this be a toplevel case like the other ones?
The condition, CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS also feels quite contrieved for this case. I guess what you want to detect is "clang, targeting windows, probably in msvc mode (as we're past a check for MINGW further up)", and we generally don't want to check for the host unless we need to.
The existing check for CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS is to pick out configurations where we can't run the generic detection shell script below.
Can't we just extend the existing MSVC case at the top, e.g. something like if (MSVC OR (CMAKE_C_COMPILER_ID MATCHES "Clang" AND WIN32 AND CMAKE_C_COMPILER_ARCHITECTURE_ID) or something along those lines, to specifically match the case we're looking for here?
Change-Id: I6e4d6336f639ca4a55829ccd5beab35d5abad339
|
|
||
| function( get_host_triple var ) | ||
| if( MSVC ) | ||
| if(MSVC OR (CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND CMAKE_C_COMPILER_ID MATCHES "Clang")) |
There was a problem hiding this comment.
This isn't about CMAKE_HOST_SYSTEM_NAME, it's about the target, so we should check CMAKE_SYSTEM_NAME instead - to make it work as expected and intended for cross compilation.
The existing check below which checks CMAKE_HOST_SYSTEM_NAME is for determining whether it will be possible to execute a shell script, where we do need to check the host environment.
In addition to the other request to clarify the wording of the commit message, I think it would be good to spell out the implicit details here; configuring like this, without explicitly telling CMake to use |
Change-Id: I998dae17e49db94e1b46aac1b35b68439e0861cb
DanielKristofKiss
changed the title
|
The commit message still reads very very weirdly to me - it talks a lot about goals and other stuff, instead of focusing on the actual CMake configuration. (All the other aspects on what tools are available and which sets of tests pass in this configuration, are entirely unrelated to this change.) May I suggest this instead, which focues on the thing at hand? Subject: [CMake] Handle clang in MSVC mode in GetHostTriple When configuring CMake with Clang in MSVC mode, Clang can either be invoked with the MSVC style driver (clang-cl) or the GNU style driver (clang). When using the MSVC style driver, CMake sets the CMake variable Even though CMake doesn't set the variable For this configuration, use the MSVC style, Doesn't that capture the thing that this patch actually deals with? If I read the code correctly, |
Change-Id: I93d2d9c5bb07f02309724ee1124d8ca7db29ad62
DanielKristofKiss
changed the title
|
|
||
| function( get_host_triple var ) | ||
| if( MSVC ) | ||
| if( MSVC OR (CMAKE_SYSTEM_NAME STREQUAL "Windows" AND CMAKE_C_COMPILER_ID MATCHES "Clang" AND NOT MINGW AND NOT MSYS)) |
There was a problem hiding this comment.
IMO, this if condition should be organized something like following:
if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
if(CMAKE_C_COMPILER_ID MATCHES "Clang")
if(MSVC)
message(STATUS "Clang is operating in MSVC mode (MSVC style driver: clang-cl).")
elseif(CMAKE_C_COMPILER_ARCHITECTURE_ID)
message(STATUS "Clang is operating in MSVC mode (GNU style driver).")
else()
message(STATUS "Clang MinGW mode")
endif()
elseif(MSVC)
message(STATUS "Compiler is MSVC (cl compiler).")
endif()There was a problem hiding this comment.
Yeah, something like that would certainly be cleaner - but that would probably also be a much larger refactoring of the cmake code here.
Anyway, I'll accept the change here as I think this version should function correctly, but I don't mind such a restructuring either (maybe ideally as a separate step?).
There was a problem hiding this comment.
I'll take care of that later, just first want to be sure all changes there to run all test, build everything fine. Not sure how much other changes needed... looking at it.
mstorsjo
left a comment
mstorsjo
left a comment
There was a problem hiding this comment.
LGTM, I think this version works as wanted.
|
|
||
| function( get_host_triple var ) | ||
| if( MSVC ) | ||
| if( MSVC OR (CMAKE_SYSTEM_NAME STREQUAL "Windows" AND CMAKE_C_COMPILER_ID MATCHES "Clang" AND NOT MINGW AND NOT MSYS)) |
There was a problem hiding this comment.
Yeah, something like that would certainly be cleaner - but that would probably also be a much larger refactoring of the cmake code here.
Anyway, I'll accept the change here as I think this version should function correctly, but I don't mind such a restructuring either (maybe ideally as a separate step?).
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/11246 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/4260 Here is the relevant piece of the build log for the reference |
Caused by #116701. Seems like a lot of arguments to repeat, so just use a plain endif().
|
Btw, there's another way of detecting Clang in MSVC mode, while using the GNU command line. Currently we do this as |
6 participants
When configuring CMake with Clang in MSVC mode, Clang can either be invoked with the MSVC style driver (clang-cl) or the GNU style driver (clang). When using the MSVC style driver, CMake sets the CMake variable MSVC (which indicates the kind of command line interface), but when using the GNU style driver, this variable isn't set, while Clang still operates in MSVC mode.
Even though CMake doesn't set the variable MSVC, it still does set CMAKE_C_COMPILER_ARCHITECTURE_ID, which it does set for MSVC and Clang in MSVC mode, but not for Clang in MinGW mode.
For this configuration, use the MSVC style, CMAKE_C_COMPILER_ARCHITECTURE_ID based GetHostTriple implementation.