libclc: Fix LLVM library linking on Windows

[daniels]

Summary

CMake requires library lists on Windows to be split by semi-colons,
rather than the spaces we get from llvm-config. Fix this by a
substitution on Windows.

[jvesely]

I think the first part should use separate_arguments the same way as LLVM_CXXFLAGS does. (you can also add the following change while at it:

-set( LLVM_CXX_FLAGS ${LLVM_CXX_FLAGS} -fno-rtti -fno-exceptions )
+list( APPEND LLVM_CXX_FLAGS -fno-rtti -fno-exceptions )

I don't see what the second part is trying to address. Does visual studio not detect the static libraries on the command line? I presume the dynamic versions are not present.
If anything it should check llvm-config --shared-mode if available instead of checking for win32. Do all visual studio/clang/mingw use the same flag?

[daniels]

Thanks for the review!

In D77162#1954860, @jvesely wrote:

I think the first part should use separate_arguments the same way as LLVM_CXXFLAGS does. (you can also add the following change while at it:

-set( LLVM_CXX_FLAGS ${LLVM_CXX_FLAGS} -fno-rtti -fno-exceptions )
+list( APPEND LLVM_CXX_FLAGS -fno-rtti -fno-exceptions )

Sure, I didn't know about these. Done and will push later this evening.

I don't see what the second part is trying to address. Does visual studio not detect the static libraries on the command line? I presume the dynamic versions are not present.
If anything it should check llvm-config --shared-mode if available instead of checking for win32. Do all visual studio/clang/mingw use the same flag?

Yes, clang runs as clang-cl which accepts MSVC-type arguments, though you can also invoke clang to pass UNIX-style arguments. For the linker, however, there is no option; lld-link only accepts MSVC-type arguments on Windows and nothing else. I don't know what MinGW does. Is there an easy way to test that? Do you currently build on MinGW?

I could test llvm-config --shared-mode == static && WIN32 to apply /MT, as I tried various other methods of convincing Windows to link statically and not dynamically with no luck. On UNIX this isn't an issue as you can happily mix the two modes.

[jvesely]

In D77162#1957318, @daniels wrote:

I don't see what the second part is trying to address. Does visual studio not detect the static libraries on the command line? I presume the dynamic versions are not present.
If anything it should check llvm-config --shared-mode if available instead of checking for win32. Do all visual studio/clang/mingw use the same flag?

Yes, clang runs as clang-cl which accepts MSVC-type arguments, though you can also invoke clang to pass UNIX-style arguments. For the linker, however, there is no option; lld-link only accepts MSVC-type arguments on Windows and nothing else. I don't know what MinGW does. Is there an easy way to test that? Do you currently build on MinGW?

I could test llvm-config --shared-mode == static && WIN32 to apply /MT, as I tried various other methods of convincing Windows to link statically and not dynamically with no luck. On UNIX this isn't an issue as you can happily mix the two modes.

I'm not sure I fully understand. windows doesn't allow mixing .lib a and .dll libraries?
a superficial search suggests that /MT instructs the linker to use the static-multithreaded version of runtime libs [0]. Other than using the same threaded runtime we should be able to mix static and dynamic libraries, no?

is the /MT flag included in LLVM_LINKER_FLAGS?
maybe we just need set_target_link_options( prepare_builtins ${LLVM_LINKER_FLAGS) or set_target_properties( prepare_bultins LINK_FLAGS ${LLVM_LINKER_FLAGS} )

[0] https://docs.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=vs-2019

[daniels]

In D77162#1958296, @jvesely wrote:

In D77162#1957318, @daniels wrote:

I could test llvm-config --shared-mode == static && WIN32 to apply /MT, as I tried various other methods of convincing Windows to link statically and not dynamically with no luck. On UNIX this isn't an issue as you can happily mix the two modes.

I'm not sure I fully understand. windows doesn't allow mixing .lib a and .dll libraries?
a superficial search suggests that /MT instructs the linker to use the static-multithreaded version of runtime libs [0]. Other than using the same threaded runtime we should be able to mix static and dynamic libraries, no?

is the /MT flag included in LLVM_LINKER_FLAGS?
maybe we just need set_target_link_options( prepare_builtins ${LLVM_LINKER_FLAGS) or set_target_properties( prepare_bultins LINK_FLAGS ${LLVM_LINKER_FLAGS} )

No, you're totally right, I was misled by the error message. We can just drop this anyway, and if the user specifies that they want LLVM to link with /MT, they can pass -DCMAKE_POLICY_DEFAULT_CMP0091=NEW -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded to the libclc build. (I missed this as libclc can't build with the Visual Studio CMake backend, only the Ninja backend ...)

I'll just drop this hunk entirely, and the patch becomes a lot smaller. :)

[daniels]

Now only splits by semicolons, and uses list syntax for compiler args

[jvesely]

LGTM. thanks

libclc/CMakeLists.txt

if( LIBCLC_TARGETS_TO_BUILD STREQUAL "all" )
   set( LIBCLC_TARGETS_TO_BUILD ${LIBCLC_TARGETS_ALL} )
 endif()
 
 list( SORT LIBCLC_TARGETS_TO_BUILD )
 
 execute_process( COMMAND ${LLVM_CONFIG} "--system-libs"
   OUTPUT_VARIABLE LLVM_SYSTEM_LIBS
   OUTPUT_STRIP_TRAILING_WHITESPACE )
+separate_arguments( LLVM_SYSTEM_LIBS )
 execute_process( COMMAND ${LLVM_CONFIG} "--libs" "core" "bitreader" "bitwriter"
   OUTPUT_VARIABLE LLVM_LIBS
   OUTPUT_STRIP_TRAILING_WHITESPACE )
+separate_arguments( LLVM_LIBS )
 execute_process( COMMAND ${LLVM_CONFIG} "--libdir"
   OUTPUT_VARIABLE LLVM_LIBDIR
   OUTPUT_STRIP_TRAILING_WHITESPACE )
 execute_process( COMMAND ${LLVM_CONFIG} "--ldflags"
   OUTPUT_VARIABLE LLVM_LD_FLAGS
   OUTPUT_STRIP_TRAILING_WHITESPACE )
 execute_process( COMMAND ${LLVM_CONFIG} "--cxxflags"
   OUTPUT_VARIABLE LLVM_CXX_FLAGS
   OUTPUT_STRIP_TRAILING_WHITESPACE )
 separate_arguments( LLVM_CXX_FLAGS )
 execute_process( COMMAND ${LLVM_CONFIG} "--bindir"
   OUTPUT_VARIABLE LLVM_BINDIR
   OUTPUT_STRIP_TRAILING_WHITESPACE )
 
 # These were not properly reported in early LLVM and we don't need them
-set( LLVM_CXX_FLAGS ${LLVM_CXX_FLAGS} -fno-rtti -fno-exceptions )
+list( APPEND LLVM_CXX_FLAGS -fno-rtti -fno-exceptions )
 
 # Print LLVM variables
 message( "LLVM system libs: ${LLVM_SYSTEM_LIBS}" )
 message( "LLVM libs: ${LLVM_LIBS}" )
 message( "LLVM libdir: ${LLVM_LIBDIR}" )
 message( "LLVM bindir: ${LLVM_BINDIR}" )
 message( "LLVM ld flags: ${LLVM_LD_FLAGS}" )
 message( "LLVM cxx flags: ${LLVM_CXX_FLAGS}" )