Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

[zahen]

Summary

Add an option to directly specify where the msvc toolchain lives for clang-cl and avoid unwanted file and registry probes.

[zahen]

Fix formatting

[zahen]

Once accepted, someone else will need to submit on my behalf.

clang/lib/Driver/ToolChains/MSVC.cpp

74

Deliberately not validating the input. The primary motivation is to prevent unnecessary file and registry access.

[aganea]

Could you please explain a little bit more what motivated this change? You can achieve almost the same with env VCToolsInstallDir=F:\Your_Path\ clang-cl .... You would also want to pass -fmsc-version=... to avoid extracting the version from cl.exe. If you're seeing many file and registry accesses, then clang-cl probably runs without a call to vcvarsall.bat before?

[zahen]

The build system strives to be deterministic so all file probes need to be accounted for; environment variables are also problematic. Our builds deliberately don't run from a Visual Studio command prompt. In addition, we'd like to avoid coupling clang tools that don't perform codegen to link.exe. We do use -fmsc-version= during the build.

[aganea]

In D85998#2231001, @zahen wrote:

The build system strives to be deterministic

When you say build system, you mean MSBuild? Other? For example, Fastbuild purposely calls CreateProcess with an empty environment block (not null) to avoid determinism issues. Is that your case? We are also using -nostdinc to avoid any compiler-generated include paths, even in cases where you need to repro something in a VS cmd shell.

The patch looks good otherwise, modulo the comments. + @amccarth @dblaikie for more opinions.

clang/lib/Driver/ToolChains/MSVC.cpp

74

I would add what you said above as a comment. It is interesting for future readers of this code, and avoids digging the history for intent.

clang/test/Driver/cl-options.c

686

Check that we're not detecting a local installation, and that we fallback to the default triple 19.11, ie.

// RUN: %clang_cl ... -vctoolsdir "" -### | FileCheck %s --check-prefix VCTOOLSDIR
// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"

[zahen]

We use BuildXL. Thanks for the comments. I'll make the requested changes and get a new rev posted.

[aganea]

clang/test/Driver/cl-options.c

685

One more thing: I would avoid long lines, and try to stay as much as possible within the 80 char per line limit. There's no hard rule for the tests as you can see in the file above, but it's a nice to have.

[zahen]

Updates as requested

[aganea]

Thanks, one more minor thing and it should be good to go.

clang/lib/Driver/ToolChains/MSVC.cpp

763–764

Please update these comments, and maybe state that the order of execution is important (for the reason you mention above)

[hans]

In D85998#2231001, @zahen wrote:

The build system strives to be deterministic so all file probes need to be accounted for; environment variables are also problematic. Our builds deliberately don't run from a Visual Studio command prompt. In addition, we'd like to avoid coupling clang tools that don't perform codegen to link.exe. We do use -fmsc-version= during the build.

How does the build system work with regular cl.exe if environment variables are problematic?

clang/lib/Driver/ToolChains/MSVC.cpp

73

What will the error look like if the user sets this but gets it wrong? I'm sympathetic to not wanting to validate it, but if it's wrong it would be nice if the error isn't too cryptic.

clang/test/Driver/cl-options.c

686

Maybe add a comment explaining the purpose of the test.

And would it be possible to have a test where -vctoolsdir is set to something, to see that it's picked up correctly?

[zahen]

Good call @aganea on these comments. Updated

[zahen]

@hans the explicit path must be declared, all exes and dlls. The unexpected probes for link.exe when invoking clang-cl.exe when it isn't actually going to be invoked are what we want to avoid.

clang/lib/Driver/ToolChains/MSVC.cpp

73

D:\>clang-cl -vctoolsdir fake main.cpp
clang-cl: error: unable to execute command: program not executable
clang-cl: error: linker command failed with exit code 1 (use -v to see invocation)

clang/test/Driver/cl-options.c

686

What's the expectation on the test boxes? I can author such a test but it would be very brittle unless we have a spec for how VS should be installed.

[hans]

clang/lib/Driver/ToolChains/MSVC.cpp

73

I realize this is not because of your patch, but it would be nice if this error could be more clear. Ideally it would at least print the path to what's failing to execute. Would you be willing to add that?

clang/test/Driver/cl-options.c

686

I'd suggest passing -vctoolsdir "/foo" and check that some /foo dir is getting passed as a system include dir to cc1, and that the link.exe invocation is under /foo.

I don't think it would be brittle.

[aganea]

clang/lib/Driver/ToolChains/MSVC.cpp

73

@hans The flag proposed here behaves like env VCToolsInstallDir="" clang-cl ..., and in that case there's no validation. Do you think the path to -vctoolsdir should be validated and throw an error otherwise? I find it nice to completly disable the toolchain auto-detection if passing in a empty dir, without needing an additional flag, ie clang-cl -vctoolsdir "".

[zahen]

clang/lib/Driver/ToolChains/MSVC.cpp

73

It's printed when using -v as directed. I don't feel strongly against adding the command line in all failure cases, but did want to point that out. Still think it's worth it?

clang/test/Driver/cl-options.c

686

Clever idea! I'll do it that way and key off the "ignored directory" warning that's emitted.

[hans]

clang/lib/Driver/ToolChains/MSVC.cpp

73

No, I'm not asking for it to be validated, but if e.g. execution of link.exe fails, it would be nice if the error message at least printed the path it was trying to execute so the user has a chance to see what's wrong.

[zahen]

clang/lib/Driver/ToolChains/MSVC.cpp

73

and the invocation with a bogus directory is as follows. No explicit path provided

"link.exe" ...

[zahen]

clang/test/Driver/cl-options.c

686

Did further digging and the path test would depend on other environment variables. If the %INCLUDE% variable is set, as it is by vcvars.bat, those paths are adopted and VCToolChainPath is not used. I'm hesitant to change that logic without stronger motivation.

Other ideas for a test that would work run both from a vc command prompt and a vanilla command prompt? I don't see anything else in the full verbose invocation to key off of:

D:\repos\llvm\llvm-project\build\Debug\bin>clang-cl -vctoolsdir "/fake" -v main.cpp
clang version 12.0.0 (https://github.com/llvm/llvm-project 537f5483fe4e09c39ffd7ac837c00b50dd13d676)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: D:\repos\llvm\llvm-project\build\Debug\bin
 "D:\\repos\\llvm\\llvm-project\\build\\Debug\\bin\\clang-cl.exe" -cc1 -triple x86_64-pc-windows-msvc19.11.0 -emit-obj -mrelax-all -mincremental-linker-compatible --mrelax-relocations -disable-free -main-file-name main.cpp -mrelocation-model pic -pic-level 2 -mframe-pointer=none -relaxed-aliasing -fmath-errno -fno-rounding-math -mconstructor-aliases -munwind-tables -target-cpu x86-64 -mllvm -x86-asm-syntax=intel -D_MT -flto-visibility-public-std --dependent-lib=libcmt --dependent-lib=oldnames -stack-protector 2 -fms-volatile -fdiagnostics-format msvc -v -resource-dir "D:\\repos\\llvm\\llvm-project\\build\\Debug\\lib\\clang\\12.0.0" -internal-isystem "D:\\repos\\llvm\\llvm-project\\build\\Debug\\lib\\clang\\12.0.0\\include" -internal-isystem "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Enterprise\\VC\\Tools\\MSVC\\14.27.29110\\ATLMFC\\include" -internal-isystem "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Enterprise\\VC\\Tools\\MSVC\\14.27.29110\\include" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\NETFXSDK\\4.8\\include\\um" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\ucrt" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\shared" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\um" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\winrt" -internal-isystem "C:\\Program Files (x86)\\Windows Kits\\10\\include\\10.0.18362.0\\cppwinrt" -fdeprecated-macro -fdebug-compilation-dir "D:\\repos\\llvm\\llvm-project\\build\\Debug\\bin" -ferror-limit 19 -fmessage-length=121 -fno-use-cxa-atexit -fms-extensions -fms-compatibility -fms-compatibility-version=19.11 -std=c++14 -fdelayed-template-parsing -fcolor-diagnostics -faddrsig -o "C:\\Users\\zahen\\AppData\\Local\\Temp\\main-8f329a.obj" -x c++ main.cpp
clang -cc1 version 12.0.0 based upon LLVM 12.0.0git default target x86_64-pc-windows-msvc
#include "..." search starts here:
#include <...> search starts here:
 D:\repos\llvm\llvm-project\build\Debug\lib\clang\12.0.0\include
 C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\ATLMFC\include
 C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include
 C:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\include\um
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\ucrt
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\shared
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\um
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\winrt
 C:\Program Files (x86)\Windows Kits\10\include\10.0.18362.0\cppwinrt
End of search list.
 "link.exe" -out:main.exe -nologo "C:\\Users\\zahen\\AppData\\Local\\Temp\\main-8f329a.obj"
clang-cl: error: unable to execute command: program not executable
clang-cl: error: linker command failed with exit code 1 (use -v to see invocation)

[hans]

clang/test/Driver/cl-options.c

686

Maybe -vctoolsdir should take precedence over %INCLUDE%? It would at least be good to understand the semantics. Like if run inside a VC command prompt, should it be possible to use -vctoolsdir to point to a different VC installation?

Also, in the clang invocation above (using -### is probably better than -v, so it doesn't try to actually run the commands), shouldn't it be executing link.exe under /fake somewhere?

[zahen]

It felt too invasive to swap the precedence of using vctoolsdir vs %INCLUDE% for all clang-cl users so I compromised by skipping the %INCLUDE% check when vctoolsdir was passed on the command line. This way only the users of the new flag will see different behavior.

With that change the suggested test for -internal-isystem passes in both vanilla and vc command prompts.

[zahen]

clang/test/Driver/cl-options.c

686

clang\lib\Driver\ToolChains\MSVC.cpp:1263

AddClangSystemIncludeArgs

1. if nostdinc => return
2. if !nobuiltininc => add driver resourcedir
3. add imsvc command line options
4. if nostdlibinc => return
5. add %INCLUDE%, if any found => return
6. if VcToolChainPath known => add relative dirs + atlmfc + search for WindowsSDK

Swapping the order of #5 and #6 is the part that I mentioned being hesitant to change. The less invasive option that I used is to skip adding %INCLUDE% paths (#5) when vctoolsdir is set.

[hans]

Seems reasonable to me.

I'm still curious why it seems it's not looking for link.exe in the /fake dir though.

[zahen]

In D85998#2236401, @hans wrote:

I'm still curious why it seems it's not looking for link.exe in the /fake dir though.

clang\lib\Driver\ToolChains\MSVC.cpp:311

FindVisualStudioExecutable
FilePath = /fake\bin\Hostx64\x64\link.exe
The test llvm::sys::fs::can_execute(FilePath) fails and the function returns "link.exe". This isn't seen in our environment because absent path probes are always allowed, it's only actual file reads that need to be tracked.

[zahen]

The real reason we don't see it internally is because we use -c for all compilation.

[zahen]

Thanks @hans! I'll need you (or someone else with permission) to land the change on my behalf.

[hans]

I'll land tomorrow unless Alexandre beats me to it.

clang/include/clang/Driver/Options.td

def _SLASH_o : CLJoinedOrSeparate<"o">,
   MetaVarName<"<file or dir/>">;
 def _SLASH_P : CLFlag<"P">, HelpText<"Preprocess to file">;
 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_volatile_iso : Option<["/", "-"], "volatile:iso", KIND_FLAG>,
   Group<_SLASH_volatile_Group>, Flags<[CLOption, DriverOption]>,
   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);
 
+// 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)) {
+    Path = A->getValue();
+    VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;
+    return true;
+  }
+  return false;
+}
+
 // Check various environment variables to try and find a toolchain.
 static bool findVCToolChainViaEnvironment(std::string &Path,
                                           MSVCToolChain::ToolsetLayout &VSLayout) {
   // These variables are typically set by vcvarsall.bat
   // when launching a developer command prompt.
   if (llvm::Optional<std::string> VCToolsInstallDir =
           llvm::sys::Process::GetEnv("VCToolsInstallDir")) {
     // This is only set by newer Visual Studios, and it leads straight to

 
 MSVCToolChain::MSVCToolChain(const Driver &D, const llvm::Triple &Triple,
                              const ArgList &Args)
     : ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args),
       RocmInstallation(D, Triple, Args) {
   getProgramPaths().push_back(getDriver().getInstalledDir());
   if (getDriver().getInstalledDir() != getDriver().Dir)
     getProgramPaths().push_back(getDriver().Dir);
 
-  // Check the environment first, since that's probably the user telling us
-  // what they want to use.
-  // Failing that, just try to find the newest Visual Studio version we can
-  // and use its default VC toolchain.
+  // Check the command line first, that's the user explicitly telling us what to
+  // use. Check the environment next, in case we're being invoked from a VS
+  // command prompt. Failing that, just try to find the newest Visual Studio
+  // version we can and use its default VC toolchain.
+  findVCToolChainViaCommandLine(Args, VCToolChainPath, VSLayout) ||
       findVCToolChainViaEnvironment(VCToolChainPath, VSLayout) ||
       findVCToolChainViaSetupConfig(VCToolChainPath, VSLayout) ||
       findVCToolChainViaRegistry(VCToolChainPath, VSLayout);
 }
 
 Tool *MSVCToolChain::buildLinker() const {
   return new tools::visualstudio::Linker(*this);
 }
 
void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   // Add %INCLUDE%-like directories from the -imsvc flag.
   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 (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())
         return;
     }
+  }
 
   // When built with access to the proper Windows APIs, try to actually find
   // the correct include paths first.
   if (!VCToolChainPath.empty()) {
     addSystemInclude(DriverArgs, CC1Args,
                      getSubDirectoryPath(SubDirectoryType::Include));
     addSystemInclude(DriverArgs, CC1Args,
                      getSubDirectoryPath(SubDirectoryType::Include, "atlmfc"));

clang/test/Driver/cl-options.c

 // NOCLANG-NOT: "--dependent-lib=msvcrt"
 
 // RUN: %clang_cl -O2 -MD /clang:-fno-slp-vectorize /clang:-MD /clang:-MF /clang:my_dependency_file.dep -### -- %s 2>&1 | FileCheck -check-prefix=CLANG %s
 // CLANG: "--dependent-lib=msvcrt"
 // CLANG-SAME: "-dependency-file" "my_dependency_file.dep"
 // CLANG-NOT: "--dependent-lib=libcmt"
 // CLANG-NOT: "-vectorize-slp"
 
+// Validate that the default triple is used when run an empty tools dir is specified
+// RUN: %clang_cl -vctoolsdir "" -### -- %s 2>&1 | FileCheck %s --check-prefix VCTOOLSDIR
+// VCTOOLSDIR: "-cc1" "-triple" "x86_64-pc-windows-msvc19.11.0"
+
+// Validate that built-in include paths are based on the supplied path
+// RUN: %clang_cl -vctoolsdir "/fake" -### -- %s 2>&1 | FileCheck %s --check-prefix FAKEDIR
+// FAKEDIR: "-internal-isystem" "/fake{{.}}include"
+// FAKEDIR: "-internal-isystem" "/fake{{.}}atlmfc{{.}}include"
+
 void f() { }