Page MenuHomePhabricator

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

Authored by zahen on Aug 14 2020, 2:15 PM.

Details

Summary

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

Diff Detail

Event Timeline

zahen created this revision.Aug 14 2020, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2020, 2:15 PM
zahen requested review of this revision.Aug 14 2020, 2:15 PM
zahen updated this revision to Diff 285803.Aug 14 2020, 5:54 PM

Fix formatting

zahen added a comment.Aug 15 2020, 2:34 PM

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 added a subscriber: aganea.Aug 21 2020, 12:14 PM

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?

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.

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 added a comment.Aug 21 2020, 2:56 PM

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

aganea added inline comments.Aug 21 2020, 3:22 PM
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 updated this revision to Diff 287425.Aug 24 2020, 9:48 AM
zahen edited reviewers, added: aganea; removed: thakis.

Updates as requested

zahen marked 3 inline comments as done.Aug 24 2020, 9:48 AM

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 added a comment.Aug 24 2020, 10:30 AM

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 updated this revision to Diff 287443.Aug 24 2020, 10:34 AM

Good call @aganea on these comments. Updated

zahen marked an inline comment as done.Aug 24 2020, 10:45 AM

@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 added inline comments.Aug 24 2020, 10:51 AM
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 added inline comments.Aug 24 2020, 10:52 AM
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 added inline comments.Aug 24 2020, 11:03 AM
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 added inline comments.Aug 24 2020, 11:03 AM
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 added inline comments.Aug 24 2020, 11:05 AM
clang/lib/Driver/ToolChains/MSVC.cpp
73

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

"link.exe" ...
zahen added inline comments.Aug 24 2020, 1:00 PM
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 added inline comments.Aug 25 2020, 5:13 AM
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 updated this revision to Diff 287671.Aug 25 2020, 8:12 AM

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 marked 3 inline comments as done.Aug 25 2020, 8:14 AM
zahen added inline comments.
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 added a comment.Aug 25 2020, 8:26 AM

Seems reasonable to me.

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

zahen marked an inline comment as done.Aug 25 2020, 8:48 AM

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 added a comment.Aug 25 2020, 8:59 AM

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

hans accepted this revision.Aug 25 2020, 10:50 AM
This revision is now accepted and ready to land.Aug 25 2020, 10:50 AM

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

hans added a comment.Aug 25 2020, 10:54 AM

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