Page MenuHomePhabricator

libclc: Fix LLVM library linking on Windows
ClosedPublic

Authored by daniels on Mar 31 2020, 12:27 PM.

Details

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.

Diff Detail

Event Timeline

daniels created this revision.Mar 31 2020, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 12:27 PM

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?

Thanks for the review!

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.

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

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 updated this revision to Diff 255483.Apr 6 2020, 2:06 PM

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

jvesely accepted this revision.Apr 6 2020, 7:09 PM

LGTM. thanks

This revision is now accepted and ready to land.Apr 6 2020, 7:09 PM
daniels edited the summary of this revision. (Show Details)Apr 13 2020, 3:40 AM
This revision was automatically updated to reflect the committed changes.