This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Support libc++ in MSVC
AcceptedPublic

Authored by phosek on Apr 28 2021, 11:27 AM.

Details

Summary

This implements support for using libc++ headers in MSVC toolchain.
We only support libc++ headers that are part of the toolchain, and
not headers installed elsewhere on the system.

Diff Detail

Event Timeline

phosek requested review of this revision.Apr 28 2021, 11:27 AM
phosek created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2021, 11:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

can you add a test for the linker invocation too?

rnk added a comment.Apr 29 2021, 11:43 AM

The test doesn't seem to pass on Jenkins. Maybe we need a REQUIRES line or a fake sysroot or something for the test.

clang/lib/Driver/ToolChains/MSVC.cpp
1348

I'm guessing this is copy-pasted code from the other toolchains, but this lambda makes less sense when its only called once. If you inline it, we don't need an extra SmallString or lambda.

phosek updated this revision to Diff 341652.Apr 29 2021, 2:44 PM
phosek marked an inline comment as done.
thakis added a comment.May 3 2021, 6:03 AM

How does this interact with /MT / /MD? Does this link the static or the dynamic libc++? If the former, does that do the right thing for DLLs? If the latter, are users expected to manually copy libc++.dll? (Sorry, not super up to speed on the static/shared lib status of libc++ on windows.)

How does this interact with /MT / /MD? Does this link the static or the dynamic libc++? If the former, does that do the right thing for DLLs? If the latter, are users expected to manually copy libc++.dll? (Sorry, not super up to speed on the static/shared lib status of libc++ on windows.)

Right now, libc++ headers mark everything dllimport by default, unless _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is defined. If you build a static-only libc++, libc++'s __config_site predefines _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS.

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

thakis added a comment.May 3 2021, 7:52 AM

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

Right, I meant more "how do users pick if they want a statically or dynamically linked libc++". Sounds like you get a dynamic libc++ by default. Since Windows doesn't have rpaths afaik, using -stdlib=libc++ means you'll get an executable that won't start, unless you know to copy the libc++ dll next to your executable with this patch as-is, yes?

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

Right, I meant more "how do users pick if they want a statically or dynamically linked libc++". Sounds like you get a dynamic libc++ by default. Since Windows doesn't have rpaths afaik, using -stdlib=libc++ means you'll get an executable that won't start, unless you know to copy the libc++ dll next to your executable with this patch as-is, yes?

Yes, that sounds correct.

As for how people choose - I guess unless you resort to trickery - you use whichever is default in the libc++ build that is used.

However, the libc++ headers inject linker directives to pull in the right form of the lib, matching the linkage of the headers. So as long as that isn't disabled, the clang driver shouldn't need to specify any lib to link against at all.

phosek added a comment.May 5 2021, 6:03 PM

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

Right, I meant more "how do users pick if they want a statically or dynamically linked libc++". Sounds like you get a dynamic libc++ by default. Since Windows doesn't have rpaths afaik, using -stdlib=libc++ means you'll get an executable that won't start, unless you know to copy the libc++ dll next to your executable with this patch as-is, yes?

Yes, that sounds correct.

As for how people choose - I guess unless you resort to trickery - you use whichever is default in the libc++ build that is used.

However, the libc++ headers inject linker directives to pull in the right form of the lib, matching the linkage of the headers. So as long as that isn't disabled, the clang driver shouldn't need to specify any lib to link against at all.

On other platforms the decision whether to use static or shared is controlled by -static-libstdc++, does CL have a similar flag or shall we support -static-libstdc++ in MSVC driver as well?

On other platforms the decision whether to use static or shared is controlled by -static-libstdc++, does CL have a similar flag or shall we support -static-libstdc++ in MSVC driver as well?

Hmm, well contrary to how -static-libstdc++ works otherwise (you only need it while linking), you'd need to define -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS everwhere when compiling, if libc++ defaults to DLL linkage but you'd want to use a static version instead.

Repeating the question - does the driver really need to know or care? Thanks to the autolinking directive in libc++ headers, the driver shouldn't need to explicitly link against the libc++ library at all?

On other platforms the decision whether to use static or shared is controlled by -static-libstdc++, does CL have a similar flag or shall we support -static-libstdc++ in MSVC driver as well?

Hmm, well contrary to how -static-libstdc++ works otherwise (you only need it while linking), you'd need to define -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS everwhere when compiling, if libc++ defaults to DLL linkage but you'd want to use a static version instead.

Repeating the question - does the driver really need to know or care? Thanks to the autolinking directive in libc++ headers, the driver shouldn't need to explicitly link against the libc++ library at all?

I'd be fine with that. Is that what CL does as well? Do we still need the -libpath: for the linker to find the library?

On other platforms the decision whether to use static or shared is controlled by -static-libstdc++, does CL have a similar flag or shall we support -static-libstdc++ in MSVC driver as well?

Hmm, well contrary to how -static-libstdc++ works otherwise (you only need it while linking), you'd need to define -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS everwhere when compiling, if libc++ defaults to DLL linkage but you'd want to use a static version instead.

Repeating the question - does the driver really need to know or care? Thanks to the autolinking directive in libc++ headers, the driver shouldn't need to explicitly link against the libc++ library at all?

I'd be fine with that. Is that what CL does as well?

AFAIK yes. And it's header-based, so clang-cl does the same - if you include and use the MSVC C++ standard library headers, it'll implicitly link in that library too.

Do we still need the -libpath: for the linker to find the library?

Ah, yes, that will be needed.

phosek updated this revision to Diff 343507.May 6 2021, 3:08 PM

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

I don't understand this. When would someone want to use /MD and not get the DLL version of the run-time libraries?

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

I don't understand this. When would someone want to use /MD and not get the DLL version of the run-time libraries?

Whether one wants to link against the CRT statically or dynamically, and libc++ statically or dynamically, are two entirely separate things. I would e.g. expect that Chrome is built with a statically linked libc++ but linked against the dynamic CRT.

In any case, as the libc++ headers supply autolinking directives that match the declarations (whether they're doing dllimport or not), the driver shouldn't really need to decide, but it's all up to the libc++ installation.

Not sure if we want the desicion between static and shared libc++ be coupled with /MT and /MD, as one can quite plausibly want to use e.g. a static libc++ with /MD.

I don't understand this. When would someone want to use /MD and not get the DLL version of the run-time libraries?

Whether one wants to link against the CRT statically or dynamically, and libc++ statically or dynamically, are two entirely separate things. I would e.g. expect that Chrome is built with a statically linked libc++ but linked against the dynamic CRT.

Ah, thanks for explaining that! In the VC++ stack, /MD and /MT make the DLL/static choice for the CRT, the C++ standard library, and the compiler runtime.[1] It never occurred to me that someone might want to select these independently.

[1]: https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-library-features?view=msvc-160

Ah, thanks for explaining that! In the VC++ stack, /MD and /MT make the DLL/static choice for the CRT, the C++ standard library, and the compiler runtime.[1] It never occurred to me that someone might want to select these independently.

Yup. As those components come from a different vendor than libc++, and have different distribution strategies/policies (and since there's some amount of shared global state in the CRT, which further affect how one can/should use it) one may want to link them in different ways.

Ping, is this OK to land?

rnk accepted this revision.May 20 2021, 10:22 AM

lgtm, sorry for the delay, this fell off the end.

This revision is now accepted and ready to land.May 20 2021, 10:22 AM
This revision was automatically updated to reflect the committed changes.
phosek reopened this revision.May 22 2021, 3:51 PM
This revision is now accepted and ready to land.May 22 2021, 3:51 PM
This revision was automatically updated to reflect the committed changes.
phosek reopened this revision.Jun 11 2021, 12:48 AM
This revision is now accepted and ready to land.Jun 11 2021, 12:48 AM

Couldn’t this commit have been kept in, and just reverting the one for using it in the fuchsia cmake cache? (I’m not using this particular commit myself, just observing.)

Couldn’t this commit have been kept in, and just reverting the one for using it in the fuchsia cmake cache? (I’m not using this particular commit myself, just observing.)

The problem is that there's no way to configure the default libc++ on a per-target basis. We want to use libc++ as a default for all targets that support it, so for example even if you're compiling on Windows but targeting Fuchsia or Linux. Unfortunately, with this change that triggers D103947 when compiling compiler-rt and I don't know how to work around it.

I'd point out that we have the same problem for other defaults, for example if you set lld as the default linker on Darwin, it causes issues because Clang tries to use lld.ld64 which is not yet usable. We still want to use lld for all other targets, but there's no way to specify that in CMake today which is something we may want to address.

Couldn’t this commit have been kept in, and just reverting the one for using it in the fuchsia cmake cache? (I’m not using this particular commit myself, just observing.)

The problem is that there's no way to configure the default libc++ on a per-target basis. We want to use libc++ as a default for all targets that support it, so for example even if you're compiling on Windows but targeting Fuchsia or Linux. Unfortunately, with this change that triggers D103947 when compiling compiler-rt and I don't know how to work around it.

I'd point out that we have the same problem for other defaults, for example if you set lld as the default linker on Darwin, it causes issues because Clang tries to use lld.ld64 which is not yet usable. We still want to use lld for all other targets, but there's no way to specify that in CMake today which is something we may want to address.

Ah, I see, thanks for explaining - the link between those wasn’t obvious to me.

Yes, that’s indeed a problem with the mechanism of overriding the default choices. (FWIW, in llvm-mingw I work around this issue by having small wrapper scripts, like <triple>-clang, that set the cross target and the defaults I when invoking clang - but that’s not very elegant either.)

Clang does have the concept of a config file, named the same a the target triple plus “.cfg”, iirc, located next to the compiler binary, that might work for setting different defaults per cross target. I haven’t really used that much though.

thieta added a subscriber: thieta.Aug 9 2022, 4:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 4:48 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
This revision was automatically updated to reflect the committed changes.
phosek reopened this revision.Aug 18 2022, 1:23 AM
This revision is now accepted and ready to land.Aug 18 2022, 1:23 AM
hofst added a subscriber: hofst.Feb 21 2023, 4:47 AM
hofst added a comment.Feb 21 2023, 4:54 AM

I was looking forward to this change in clang 16 but just noticed that you had to revert the change again at August 18 2022.
@phosek Do you still plan to work on this change? Is there any way I could help?