This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Switch the CMP0091 policy (MSVC_RUNTIME_LIBRARY) to the new behaviour
ClosedPublic

Authored by mstorsjo on Jul 13 2023, 1:59 PM.

Details

Summary

With the new behaviour, the /MD or similar options aren't added to
e.g. CMAKE_CXX_FLAGS_RELEASE, but are added separately by CMake.
They can be changed by the cmake variable
CMAKE_MSVC_RUNTIME_LIBRARY or with the target property
MSVC_RUNTIME_LIBRARY.

LLVM has had its own custom CMake flags, e.g. LLVM_USE_CRT_RELEASE,
which affects which CRT is used for release mode builds. Deprecate
these and direct users to use CMAKE_MSVC_RUNTIME_LIBRARY directly
instead (and do a best effort attempt at setting CMAKE_MSVC_RUNTIME_LIBRARY
based on the existing LLVM_USE_CRT_ flags). This only handles the
simple cases, it doesn't handle multi-config generators with
different LLVM_USE_CRT_* variables for different configs though,
but that's probably fine - we should move over to the new upstream
CMake mechanism anyway, and push users towards that.

Change code in compiler-rt, that previously tried to override the
CRT choice to /MT, to set CMAKE_MSVC_RUNTIME_LIBRARY instead of
meddling in the old variables.

This resolves the policy issue in
https://github.com/llvm/llvm-project/issues/63286, and should
handle the issues that were observed originally when the
minimum CMake version was bumped, in
https://github.com/llvm/llvm-project/issues/62719 and
https://github.com/llvm/llvm-project/issues/62739.

I'm tagging people who were involved in the previous reported
issues, @hans, @glandium, @dschuff - I'd appreciate if you could
test this in your build configs (I've tried to test the cases
mentioned).

Also @sunho, this touches the ORC runtime library on Windows,
can you verify that it still works as intended in your setups?

Diff Detail

Event Timeline

mstorsjo created this revision.Jul 13 2023, 1:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 1:59 PM
mstorsjo requested review of this revision.Jul 13 2023, 1:59 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 13 2023, 1:59 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

I can confirm this doesn't seem to break our builds.

glandium resigned from this revision.Jul 13 2023, 6:10 PM
phosek accepted this revision.Jul 13 2023, 11:20 PM

LGTM

This revision is now accepted and ready to land.Jul 13 2023, 11:20 PM
hans added a comment.Jul 14 2023, 4:44 AM

Confirmed that Chromium's toolchain build works with this.

dschuff accepted this revision.Jul 14 2023, 11:37 AM

Works for me, thanks for the heads-up.

Thanks for testing! I'll go ahead and land this on Monday probably, and then follow up with a second patch to clean up the supposedly unnecessary messing around with the flags variables.

I didn't get the compiler-rt orc stuff tested yet though (the tests fail for me before this patch too - the tests end up looking for liborc_rt-x86_64.a while the build produced orc_rt-x86_64.a). But the changes in that area seem quite trivial.

sunho added a comment.Jul 16 2023, 2:31 AM

LGTM. For ORC side failure, I'll take a look and submit a patch. I believe that shouldn't block this patch, as it seems to be failing long time ago.

LGTM. For ORC side failure, I'll take a look and submit a patch. I believe that shouldn't block this patch, as it seems to be failing long time ago.

Thanks for having a look!

This revision was landed with ongoing or failed builds.Jul 17 2023, 12:24 AM
This revision was automatically updated to reflect the committed changes.
smeenai added inline comments.
compiler-rt/CMakeLists.txt
391–402

https://bugs.llvm.org/show_bug.cgi?id=20214 was marked as fixed back in 2014. I realized that we'd been building compiler-rt with /MD for years (our flags were set up in a way that wasn't caught by the flag replacement below, I guess), and it seems to be working fine. Should we make this user-configurable now?

mstorsjo added inline comments.Jul 18 2023, 12:08 AM
compiler-rt/CMakeLists.txt
391–402

Hmm, regarding ASAN, I think there's two separate concerns - which CRT the ASAN DLL itself is linked against (which is what this code here would affect) and what kind of CRT for the user code that ASAN intercepts. The referenced bug seems to be about the latter. So while the bug is fixed, it might still be the case that the sanitizer code needs to becompiled in one specific way. (But the comment indeed seems outdated.)

Curiosly, how did you pass /MD to the compiler so that it passed through unmodified here?

As for making it user-configurable - at least some parts of the compiler-rt libraries and/or their tests require this - see the original issue at https://github.com/llvm/llvm-project/issues/62719. If we can fix that issue at the root, to allow getting rid of this override here, that'd be nice. But I wouldn't be surprised if at least some of the sanitizer libraries require being compiled in a specific way.

smeenai added inline comments.Jul 18 2023, 10:01 AM
compiler-rt/CMakeLists.txt
391–402

It turns out to be kinda dumb :D In a (possibly misguided) attempt to minimize build worker disk usage, I'd only imported portions of the Windows SDK, and I omitted libcmt.lib since I didn't think it was needed anywhere. That meant that COMPILER_RT_HAS_MT_FLAG was false, which is why we kept the /MD. Seems like we've been skating on thin ice though and I should just import libcmt and go with the static linking.