clang-cl: Invent a /winsysroot concept

[thakis]

Summary

On non-Windows platforms, --sysroot can be used to make the compiler use
a single, hermetic directory for all header and library files.

This is useful, but difficult to do on Windows. After D95472 it's
possible to achieve this with two flags:

out/gn/bin/clang-cl win.c -fuse-ld=lld \
    /vctoolsdir path/to/VC/Tools/MSVC/14.26.28801 \
    /winsdkdir path/to/win_sdk

But that's still cumbersome: It requires two flags instead of one, and
it requires writing down the (changing) VC/Tools/MSVC version.

This adds a new /winsysroot <dir> flag that's effectively an alias to
these two flags. With this, building against a hermetic Windows
toolchain only needs:

out/gn/bin/clang-cl win.c -fuse-ld=lld /winsysroot path

/winsysroot <dir> is the same as adding

/vctoolsdir <dir>/VC/Tools/MSVC/<vctoolsver>
/winsdkdir <dir>/Windows Kits/<winsdkmajorversion>

<vctoolsver> is taken from /vctoolsversion if passed, or else it's the
name of the directory in <dir>/VC/Tools/MSVC that's the highest numeric
tuple.

<winsdkmajorversion> is the major version in /winsdkversion if passed,
else it's the name of the directory in <dir>/Windows Kits that's
the highest number.

So /winsysroot <path> requires this subfolder structure:

path/
  VC/
    Tools/
      MSVC/
        14.26.28801  (or another number)
          include/
          ...
  Windows Kits/
    10/
      Include/
        10.0.19041.0/ (or another number)
          um/
          ...
      Lib/
        10.0.19041.0/ (or another number)
          um/
            x64/
            ...
          ...

[thakis]

Maybe we should just go with `Windows Kits/10" -- the space is only seen at the driver->cc1 boundary and it works fine there, and the 10 can be inferred from /winsdkversion.

On the other hand, thinking about how to create such a sysroot directory, saying "just copy %vctoolsinstalldir% to somedir/vctools and %windowssdkdir% to somedir/win_sdk" would be nice too. On the other hand, it'd break relative paths from vc\tools\msvc\XXX to (say) vc\tools\llvm. I don't know if anything needs such relative paths.

[mstorsjo]

"Windows Kits/10" has the drawback that it contains a space and yet another number. Ideally the contents of that directory would be available under some short name, so I went with /winsdkdir <dir>/win_sdk. I'm not married to this though. Maybe <dir>/WinSdk looks move like VC/Tools/MSVC?

FWIW, for my own cross-MSVC setups, I use <dir>/vc/tools/msvc and <dir>/kits/10 - kinda preserving the "Windows Kits" directory, but in a friendlier form - but this proposal is clearly workable too.

[thakis]

Seems like there are no strong opinions, so now with actual code. I went with "VC/Tools/MSVC/XXX" and "Windows Kits/10" since that seems like the obvious thing.

[thakis]

add test

[hans]

clang/lib/Driver/ToolChains/MSVC.cpp

75

for being defensive, would it be safer to check !EC before comparing the iterators, in case DirIt is in a bad state because of an error?

88

Since the success of the function is directly tied to whether the string is empty, maybe just return the string directly to simplify the API?

Oh I see, this comes from getWindows10SDKVersionFromPath(). Either way is fine I suppose.

1111

EC is unused now

clang/test/Driver/cl-sysroot.cpp

2 ↗

(On Diff #319938)

fancy :)

4 ↗

(On Diff #319938)

Driver/ tests don't usually run the actual compiler. Any reason not to just check the -### output?

[thakis]

address comments

[thakis]

clang/lib/Driver/ToolChains/MSVC.cpp

75

Done (but this code is just moved up from below)

88

Thanks, that's much nicer.

clang/test/Driver/cl-sysroot.cpp

4 ↗

(On Diff #319938)

I had that at first, but it felt a bit brittle to me. But sure, moved back to that.

[hans]

lgtm (with test nit)

clang/test/Driver/cl-sysroot.cpp

10 ↗

(On Diff #319949)

could the slashes be backslashes when this test runs on a windows machine?

4 ↗

(On Diff #319938)

Thanks! Running the compiler here could be brittle too, since it will run for all backends, and some could get confused by seeing windows as part of the target triple.

[thakis]

clang/test/Driver/cl-sysroot.cpp

10 ↗

(On Diff #319949)

Yes, thanks. Even worse, the whole -DFOO=%t approach doesn't work because clang escapes %t for the driver, which double all the slashes etc. Instead gave the toolsdir a number that no real msvc install should use and then captured the root dir via filecheck from there.

[rnk]

+People who have spent time on cross-compilation Windows sysroots in the past: @smeenai @compnerd

[smeenai]

I saw this on LLVM Weekly, and I like it a lot :)

Now if only Windows could case-correct their SDKs so we didn't need VFS overlays and symlinks for the linker...

[aganea]

In D95534#2534510, @smeenai wrote:

Now if only Windows could case-correct their SDKs so we didn't need VFS overlays and symlinks for the linker...

I opened a ticket but it was closed shortly after. Microsoft could easily fix the #includes but the #pragma comment(lib, ..) would require an additionnal layer of OLDNAMES.lib aka /ALTERNATENAME directives when dealing with external libraries, since they already have the (old) #pragma comment(lib, ..) directives already stamped in. Even after making these changes manually for a PoC, we still needed clang-cl ... -Xclang -fixit -Wnonportable-include-path -Wnonportable-system-include-path to fix some paths in third-party libraries - and discovered along the way that -fixit didn't work with PCH files in this specific case. All just a massive pain.

We ended up mounting a 'casefold' filesystem and now it cross-compiles like a charm! :-) (and incidentally it is a bit faster to build than on Windows, on the same hardware)

[thakis]

In D95534#2534510, @smeenai wrote:

I saw this on LLVM Weekly, and I like it a lot :)

Now if only Windows could case-correct their SDKs so we didn't need VFS overlays and symlinks for the linker...

We use ciopfs for this, https://source.chromium.org/chromium/chromium/src/+/master:build/vs_toolchain.py;l=485?q=ciopfs That code has worked without changes for the last 2.5 years.

[thakis]

We use ciopfs for this, https://source.chromium.org/chromium/chromium/src/+/master:build/vs_toolchain.py;l=485?q=ciopfs That code has worked without changes for the last 2.5 years.

(ciopfs is user-space, so it can be set up transparently by a normal post-pull hook and this doesn't need any manual setup by devs or bots.)

[aganea]

In D95534#2550998, @thakis wrote:

In D95534#2534510, @smeenai wrote:

I saw this on LLVM Weekly, and I like it a lot :)

Now if only Windows could case-correct their SDKs so we didn't need VFS overlays and symlinks for the linker...

We use ciopfs for this, https://source.chromium.org/chromium/chromium/src/+/master:build/vs_toolchain.py;l=485?q=ciopfs That code has worked without changes for the last 2.5 years.

(sorry a bit off topic for LLVM)
Hello @thakis ! What is the best way to report Chrome dev issues? I wanted to try ciopfs, so I followed the guide here, but clang-cl then fails with "Too many files open", pointing to files in chromium/src/third_party/depot_tools/win_toolchain/vs_files/. I've removed the ciopfs mount point and instead created a 'casefold' folder at the same location (ie. chattr +F vs_files), unzipped the SDKs again, and now the build compiles fine. Raising ulimit -n 250000 and fs.file-max = 1000000 didn't help the ciopfs issue in the first place. I'm running on a 48C/96HT machine on Ubuntu 20.04. There seems to be either a leak of FDs or caching inside ciopfs? Any suggestions?

clang/include/clang/Driver/Options.td

 def _SLASH_Tc : CLCompileJoinedOrSeparate<"Tc">,
   HelpText<"Treat <file> as C source file">, MetaVarName<"<file>">;
 def _SLASH_TC : CLCompileFlag<"TC">, HelpText<"Treat all source files as C">;
 def _SLASH_Tp : CLCompileJoinedOrSeparate<"Tp">,
   HelpText<"Treat <file> as C++ source file">, MetaVarName<"<file>">;
 def _SLASH_TP : CLCompileFlag<"TP">, HelpText<"Treat all source files as C++">;
 def _SLASH_vctoolsdir : CLJoinedOrSeparate<"vctoolsdir">,
   HelpText<"Path to the VCToolChain">, MetaVarName<"<dir>">;
+def _SLASH_vctoolsversion : CLJoinedOrSeparate<"vctoolsversion">,
+  HelpText<"For use with /winsysroot, defaults to newest found">;
 def _SLASH_winsdkdir : CLJoinedOrSeparate<"winsdkdir">,
   HelpText<"Path to the Windows SDK">, MetaVarName<"<dir>">;
 def _SLASH_winsdkversion : CLJoinedOrSeparate<"winsdkversion">,
-  HelpText<"Full version of the Windows SDK">;
+  HelpText<"Full version of the Windows SDK, defaults to newest found">;
+def _SLASH_winsysroot : CLJoinedOrSeparate<"winsysroot">,
+  HelpText<"Same as /vctoolsdir <dir>/VC/Tools/MSVC/<vctoolsversion> /winsdkdir <dir>/Windows Kits/10">,
+  MetaVarName<"<dir>">;
 def _SLASH_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>,
   Group<_SLASH_volatile_Group>, Flags<[CLOption, NoXarchOption]>,
   HelpText<"Volatile loads and stores have standard semantics">;
 def _SLASH_vmb : CLFlag<"vmb">,
   HelpText<"Use a best-case representation method for member pointers">;
 def _SLASH_vmg : CLFlag<"vmg">,
   HelpText<"Use a most-general representation for member pointers">;
 def _SLASH_vms : CLFlag<"vms">,

clang/lib/Driver/ToolChains/MSVC.cpp

 using namespace clang;
 using namespace llvm::opt;
 
 // Defined below.
 // Forward declare this so there aren't too many things above the constructor.
 static bool getSystemRegistryString(const char *keyPath, const char *valueName,
                                     std::string &value, std::string *phValue);
 
+static bool getHighestNumericTupleInDirectory(StringRef Directory, std::string &Highest) {
+  Highest.clear();
+  llvm::VersionTuple HighestTuple;
+
+  std::error_code EC;
+  for (llvm::sys::fs::directory_iterator DirIt(Directory, EC), DirEnd;
+       DirIt != DirEnd && !EC; DirIt.increment(EC)) {
+    if (!llvm::sys::fs::is_directory(DirIt->path()))
+      continue;
+    StringRef CandidateName = llvm::sys::path::filename(DirIt->path());
+    llvm::VersionTuple Tuple;
+    if (Tuple.tryParse(CandidateName)) // tryParse() returns true on error.
+      continue;
+    if (Tuple > HighestTuple) {
+      HighestTuple = Tuple;
+      Highest = CandidateName.str();
+    }
+  }
+
+  return !Highest.empty();
+}
+
 // Check command line arguments to try and find a toolchain.
 static bool
 findVCToolChainViaCommandLine(const ArgList &Args, std::string &Path,
                               MSVCToolChain::ToolsetLayout &VSLayout) {
   // Don't validate the input; trust the value supplied by the user.
   // The primary motivation is to prevent unnecessary file and registry access.
-  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir,
+                               options::OPT__SLASH_winsysroot)) {
+    if (A->getOption().getID() == options::OPT__SLASH_winsysroot) {
+      llvm::SmallString<128> ToolsPath(A->getValue());
+      llvm::sys::path::append(ToolsPath, "VC", "Tools", "MSVC");
+      std::string VCToolsVersion;
+      if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsversion))
+        VCToolsVersion = A->getValue();
+      else
+        getHighestNumericTupleInDirectory(ToolsPath, VCToolsVersion);
+      llvm::sys::path::append(ToolsPath, VCToolsVersion);
+      Path = std::string(ToolsPath.str());
+    } else {
       Path = A->getValue();
+    }
     VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
     return true;
   }
   return false;
 }
 
 // Check various environment variables to try and find a toolchain.
 static bool
void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   }
 
   // If the VC environment hasn't been configured (perhaps because the user
   // did not run vcvarsall), try to build a consistent link environment.  If
   // the environment variable is set however, assume the user knows what
   // they're doing. If the user passes /vctoolsdir or /winsdkdir, trust that
   // over env vars.
   if (!llvm::sys::Process::GetEnv("LIB") ||
-      Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+      Args.getLastArg(options::OPT__SLASH_vctoolsdir,
+                      options::OPT__SLASH_winsysroot)) {
     CmdArgs.push_back(Args.MakeArgString(
         Twine("-libpath:") +
         TC.getSubDirectoryPath(
             toolchains::MSVCToolChain::SubDirectoryType::Lib)));
     CmdArgs.push_back(Args.MakeArgString(
         Twine("-libpath:") +
         TC.getSubDirectoryPath(toolchains::MSVCToolChain::SubDirectoryType::Lib,
                                "atlmfc")));
   }
   if (!llvm::sys::Process::GetEnv("LIB") ||
-      Args.getLastArg(options::OPT__SLASH_winsdkdir)) {
+      Args.getLastArg(options::OPT__SLASH_winsdkdir,
+                      options::OPT__SLASH_winsysroot)) {
     if (TC.useUniversalCRT()) {
       std::string UniversalCRTLibPath;
       if (TC.getUniversalCRTLibraryPath(Args, UniversalCRTLibPath))
         CmdArgs.push_back(
             Args.MakeArgString(Twine("-libpath:") + UniversalCRTLibPath));
     }
     std::string WindowsSdkLibPath;
     if (TC.getWindowsSDKLibraryPath(Args, WindowsSdkLibPath))

 // Find the most recent version of Universal CRT or Windows 10 SDK.
 // vcvarsqueryregistry.bat from Visual Studio 2015 sorts entries in the include
 // directory by name and uses the last one of the list.
 // So we compare entry names lexicographically to find the greatest one.
 static bool getWindows10SDKVersionFromPath(const std::string &SDKPath,
                                            std::string &SDKVersion) {
   SDKVersion.clear();
 
   std::error_code EC;
   llvm::SmallString<128> IncludePath(SDKPath);
   llvm::sys::path::append(IncludePath, "Include");
-  for (llvm::sys::fs::directory_iterator DirIt(IncludePath, EC), DirEnd;
-       DirIt != DirEnd && !EC; DirIt.increment(EC)) {
-    if (!llvm::sys::fs::is_directory(DirIt->path()))
-      continue;
-    StringRef CandidateName = llvm::sys::path::filename(DirIt->path());
-    // If WDK is installed, there could be subfolders like "wdf" in the
-    // "Include" directory.
-    // Allow only directories which names start with "10.".
-    if (!CandidateName.startswith("10."))
-      continue;
-    if (CandidateName > SDKVersion)
-      SDKVersion = std::string(CandidateName);
-  }
-
-  return !SDKVersion.empty();
+  return getHighestNumericTupleInDirectory(IncludePath, SDKVersion);
 }
 
 static bool getWindowsSDKDirViaCommandLine(const ArgList &Args,
                                            std::string &Path, int &Major,
                                            std::string &Version) {
-  if (Arg *A = Args.getLastArg(options::OPT__SLASH_winsdkdir)) {
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_winsdkdir,
+                               options::OPT__SLASH_winsysroot)) {
     // Don't validate the input; trust the value supplied by the user.
     // The motivation is to prevent unnecessary file and registry access.
+    llvm::VersionTuple SDKVersion;
+    if (Arg *A = Args.getLastArg(options::OPT__SLASH_winsdkversion))
+      SDKVersion.tryParse(A->getValue());
+
+    if (A->getOption().getID() == options::OPT__SLASH_winsysroot) {
+      llvm::SmallString<128> SDKPath(A->getValue());
+      llvm::sys::path::append(SDKPath, "Windows Kits");
+      if (!SDKVersion.empty()) {
+        llvm::sys::path::append(SDKPath, Twine(SDKVersion.getMajor()));
+      } else {
+        std::string SDKVersion;
+        if (getHighestNumericTupleInDirectory(SDKPath, SDKVersion))
+          llvm::sys::path::append(SDKPath, SDKVersion);
+      }
+      Path = std::string(SDKPath.str());
+    } else {
       Path = A->getValue();
-    if (Arg *A = Args.getLastArg(options::OPT__SLASH_winsdkversion)) {
-      StringRef WinSdkVersion = A->getValue();
-      Version = WinSdkVersion.str();
-      if (WinSdkVersion.consumeInteger(10, Major))
-        return false;
-      if (!(WinSdkVersion.empty() || WinSdkVersion.startswith(".")))
-        return false;
+    }
+
+    if (!SDKVersion.empty()) {
+      Major = SDKVersion.getMajor();
+      Version = SDKVersion.getAsString();
     } else if (getWindows10SDKVersionFromPath(Path, Version)) {
       Major = 10;
     }
     return true;
   }
   return false;
 }
 
void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   for (const auto &Path : DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc))
     addSystemInclude(DriverArgs, CC1Args, Path);
 
   if (DriverArgs.hasArg(options::OPT_nostdlibinc))
     return;
 
   // Honor %INCLUDE%. It should know essential search paths with vcvarsall.bat.
   // Skip if the user expressly set a vctoolsdir
-  if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+  if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
+                             options::OPT__SLASH_winsysroot)) {
     if (llvm::Optional<std::string> cl_include_dir =
             llvm::sys::Process::GetEnv("INCLUDE")) {
       SmallVector<StringRef, 8> Dirs;
       StringRef(*cl_include_dir)
           .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
       for (StringRef Dir : Dirs)
         addSystemInclude(DriverArgs, CC1Args, Dir);
       if (!Dirs.empty())