[Driver] Support libc++ in MSVC

[phosek]

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.

[thakis]

can you add a test for the linker invocation too?

[rnk]

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

1332

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.

[thakis]

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.)

[mstorsjo]

In D101479#2733303, @thakis wrote:

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]

In D101479#2733354, @mstorsjo wrote:

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?

[mstorsjo]

In D101479#2733434, @thakis wrote:

In D101479#2733354, @mstorsjo wrote:

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]

In D101479#2733622, @mstorsjo wrote:

In D101479#2733434, @thakis wrote:

In D101479#2733354, @mstorsjo wrote:

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?

[mstorsjo]

In D101479#2740895, @phosek wrote:

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?

[phosek]

In D101479#2741202, @mstorsjo wrote:

In D101479#2740895, @phosek wrote:

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?

[mstorsjo]

In D101479#2741223, @phosek wrote:

In D101479#2741202, @mstorsjo wrote:

In D101479#2740895, @phosek wrote:

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.

[amccarth]

In D101479#2733354, @mstorsjo wrote:

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?

[mstorsjo]

In D101479#2748189, @amccarth wrote:

In D101479#2733354, @mstorsjo wrote:

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.

[amccarth]

In D101479#2748475, @mstorsjo wrote:

In D101479#2748189, @amccarth wrote:

In D101479#2733354, @mstorsjo wrote:

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

[mstorsjo]

In D101479#2750765, @amccarth wrote:

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.

[phosek]

Ping, is this OK to land?

[rnk]

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

[mstorsjo]

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.)

[phosek]

In D101479#2813724, @mstorsjo wrote:

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.

[mstorsjo]

In D101479#2813797, @phosek wrote:

In D101479#2813724, @mstorsjo wrote:

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.

[hofst]

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?

clang/lib/Driver/ToolChains/MSVC.cpp

const StringRef Paths[] = {
     "C:/Program Files/Microsoft Visual Studio 8/VC/PlatformSDK/Include"
   };
   addSystemIncludes(DriverArgs, CC1Args, Paths);
 #endif
 }
 
 void MSVCToolChain::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
                                                  ArgStringList &CC1Args) const {
-  // FIXME: There should probably be logic here to find libc++ on Windows.
+  if (DriverArgs.hasArg(options::OPT_nostdlibinc) ||
+      DriverArgs.hasArg(options::OPT_nostdincxx))
+    return;
+
+  const Driver &D = getDriver();
+
+  auto AddLibcxxIncludePath = [&](StringRef Path) {
+    std::string Version = detectLibcxxVersion(Path);
+    if (Version.empty())
+      return;
+
+    SmallString<128> Dir(Path);
+    llvm::sys::path::append(Dir, "c++", Version);
+    addSystemInclude(DriverArgs, CC1Args, Dir);
+  };
+
+  switch (GetCXXStdlibType(DriverArgs)) {
+  case ToolChain::CST_Libcxx: {
+    SmallString<128> P(D.Dir);
+    llvm::sys::path::append(P, "..", "include");
+    AddLibcxxIncludePath(P);
+    break;
+  }
+
+  default:
+    // TODO: Shall we report an error for other C++ standard libraries?
+    break;
+  }
 }
 
 VersionTuple MSVCToolChain::computeMSVCVersion(const Driver *D,
                                                const ArgList &Args) const {
   bool IsWindowsMSVC = getTriple().isWindowsMSVCEnvironment();
   VersionTuple MSVT = ToolChain::computeMSVCVersion(D, Args);
   if (MSVT.empty())
     MSVT = getMSVCVersionFromTriple(getTriple());

clang/test/Driver/msvc-libcxx.cpp

+// RUN: %clangxx -target x86_64-pc-windows-msvc --stdlib=libc++ -### %s 2>&1 | FileCheck %s -check-prefix MSVC-LIBCXX
+
+// MSVC-LIBCXX: "-internal-isystem" "{{.*[/\\]}}include{{/|\\\\}}c++{{/|\\\\}}v1"