Use the Windows SDK arguments over the environment

[Steelskin]

If any of the Windows SDK (and MSVC)-related argument is passed in the command line, they should take priority over the environment variables like INCLUDE or LIB set by vcvarsall from the Visual Studio Developer Environment on Windows.

These changes ensure that all of the arguments related to VC Tools and the Windows SDK cause the driver to ignore the environment.

[llvmbot]

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-lld-coff
@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Fabrice de Gans (Steelskin)

Changes

If any of the Windows SDK (and MSVC)-related argument is passed in the command line, they should take priority over the environment variables like INCLUDE or LIB set by vcvarsall from the Visual Studio Developer Environment on Windows.

These changes ensure that all of the arguments related to VC Tools and the Windows SDK cause the driver to ignore the environment.

---

Full diff: https://github.com/llvm/llvm-project/pull/144805.diff

1 Files Affected:

• (modified) clang/lib/Driver/ToolChains/MSVC.cpp (+24-15)

diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index 1e244865b2117..632cc3a0a8b9b 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -94,10 +94,14 @@ 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 (const Arg *A = Args.getLastArg(options::OPT__SLASH_diasdkdir,
-                                     options::OPT__SLASH_winsysroot)) {
+  // they're doing. If the user passes /vctoolsdir or /winsdkdir or any of the
+  // other Windows SDK options, trust that over env vars.
+  const Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir,
+                                 options::OPT__SLASH_vctoolsversion,
+                                 options::OPT__SLASH_winsysroot,
+                                 options::OPT__SLASH_winsdkdir,
+                                 options::OPT__SLASH_winsdkversion);
+  if (A) {
     // cl.exe doesn't find the DIA SDK automatically, so this too requires
     // explicit flags and doesn't automatically look in "DIA SDK" relative
     // to the path we found for VCToolChainPath.
@@ -110,9 +114,7 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                             llvm::archToLegacyVCArch(TC.getArch()));
     CmdArgs.push_back(Args.MakeArgString(Twine("-libpath:") + DIAPath));
   }
-  if (!llvm::sys::Process::GetEnv("LIB") ||
-      Args.getLastArg(options::OPT__SLASH_vctoolsdir,
-                      options::OPT__SLASH_winsysroot)) {
+  if (!llvm::sys::Process::GetEnv("LIB") || A) {
     CmdArgs.push_back(Args.MakeArgString(
         Twine("-libpath:") +
         TC.getSubDirectoryPath(llvm::SubDirectoryType::Lib)));
@@ -120,9 +122,7 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
         Twine("-libpath:") +
         TC.getSubDirectoryPath(llvm::SubDirectoryType::Lib, "atlmfc")));
   }
-  if (!llvm::sys::Process::GetEnv("LIB") ||
-      Args.getLastArg(options::OPT__SLASH_winsdkdir,
-                      options::OPT__SLASH_winsysroot)) {
+  if (!llvm::sys::Process::GetEnv("LIB") || A) {
     if (TC.useUniversalCRT()) {
       std::string UniversalCRTLibPath;
       if (TC.getUniversalCRTLibraryPath(Args, UniversalCRTLibPath))
@@ -677,9 +677,18 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
     AddSystemIncludesFromEnv(Var);
   }
 
+  // Check if the user has explicitly set a vctoolsdir or any of the
+  // Windows SDK options. If so, we assume the user knows what they're doing
+  // and don't try to find the include directories automatically.
+  // If not, we try to find the include directories automatically.
+  const Arg *A = DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
+                                       options::OPT__SLASH_vctoolsversion,
+                                       options::OPT__SLASH_winsysroot,
+                                       options::OPT__SLASH_winsdkdir,
+                                       options::OPT__SLASH_winsdkversion);
+
   // Add DIA SDK include if requested.
-  if (const Arg *A = DriverArgs.getLastArg(options::OPT__SLASH_diasdkdir,
-                                           options::OPT__SLASH_winsysroot)) {
+  if (A) {
     // cl.exe doesn't find the DIA SDK automatically, so this too requires
     // explicit flags and doesn't automatically look in "DIA SDK" relative
     // to the path we found for VCToolChainPath.
@@ -694,9 +703,9 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
     return;
 
   // Honor %INCLUDE% and %EXTERNAL_INCLUDE%. It should have essential search
-  // paths set by vcvarsall.bat. Skip if the user expressly set a vctoolsdir.
-  if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
-                             options::OPT__SLASH_winsysroot)) {
+  // paths set by vcvarsall.bat. Skip if the user expressly set any of the
+  // Windows SDK options.
+  if (!A) {
     bool Found = AddSystemIncludesFromEnv("INCLUDE");
     Found |= AddSystemIncludesFromEnv("EXTERNAL_INCLUDE");
     if (Found)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[compnerd]

We should add a test for this behaviour like we have for the other options.

[dpaoliello]

Just to confirm, does this make clang-cl line up with cl's behavior?

[compnerd]

Just to confirm, does this make clang-cl line up with cl's behavior?

I suspect not; these options are clang-cl specific.

[Steelskin]

Just to confirm, does this make clang-cl line up with cl's behavior?

As @compnerd said, this is clang-cl specific. None of these options exist in cl.exe.
I am looking at the test failure on Linux but I believe this is unrelated, I'll rebase and re-run.

[compnerd]

Thanks, this looks pretty good to me now.