[Driver] Set linkPath and MSVT version when cl.exe is detected.

[hintonda]

Summary

Set linkPath and MSVT version when cl.exe is found in PATH.

[hintonda]

• Cleanup path variables and make sure loop exits.

[STL_MSFT]

If I deal with the include vs. inc problem (in the debugger, by manually controlling which branch is taken), MSVCToolChain::getSubDirectoryPath() is problematic (as I described in my emails). Now, IsVS2017OrNewer is set, which causes it to form C:\Temp\binaries\x86ret\bin\HostX86\x86, which isn't the correct C:\Temp\binaries\x86ret\bin\i386 (where cl.exe was found).

lib/Driver/ToolChains/MSVC.cpp

138

This doesn't work for my dev build's directory structure, because the bin and inc subdirectories are siblings - there's no include subdirectory.

I additionally observe that this loop is very aggressive about looking for include, going all the way out to C:\include. Maybe this isn't a problem in practice (it isn't a problem for me).

[hintonda]

In D36842#844899, @STL_MSFT wrote:

If I deal with the include vs. inc problem (in the debugger, by manually controlling which branch is taken), MSVCToolChain::getSubDirectoryPath() is problematic (as I described in my emails). Now, IsVS2017OrNewer is set, which causes it to form C:\Temp\binaries\x86ret\bin\HostX86\x86, which isn't the correct C:\Temp\binaries\x86ret\bin\i386 (where cl.exe was found).

When is getSubDirectoryPath() getting called? The goal was obviate the need by caching libPath. Which other cases are needed? Btw, I'm assuming you set the variables LIB, etc...

Can you suggest an alternative to the way I'm attempting to find the root directory -- looking for "include" is obviously not the best idea. Would the old way be better?

[STL_MSFT]

When is getSubDirectoryPath() getting called?

It's getting called after MSVCToolChain::findVCToolChainViaEnvironment(). The callstack is when clang::driver::tools::Clang::ConstructJob() calls clang::driver::toolchains::MSVCToolChain::computeMSVCVersion(). That contains the line MSVT = getMSVCVersionFromExe(getSubDirectoryPath(SubDirectoryType::Bin));.

I'm currently seeing whether MSVCToolChain::getSubDirectoryPath() can inspect this->MSVT and if it's non-empty (which is the case when I get past inc vs. include), just return that.

The goal was obviate the need by caching libPath. Which other cases are needed?

One problem seems to be that the linkPath caching is entirely unused.

Btw, I'm assuming you set the variables LIB, etc...

Yes, both INCLUDE and LIB are set in my repro.

Can you suggest an alternative to the way I'm attempting to find the root directory -- looking for "include" is obviously not the best idea. Would the old way be better?

I haven't carefully thought about it, but my first impulse would be to try all three forms: pre-2017, 2017+, and internal-dev.

I am currently building a hacky patch that just checks for either include or inc. That gets me past that point.

lib/Driver/ToolChains/MSVC.cpp

437

I think you intended for the new data member linkPath in MSVCToolChain to be used. However, we're in clang::driver::tools::visualstudio::Linker::ConstructJob() and these mentions of linkPath are referring to this local variable. I can't find any actual reads of the data member.

[hintonda]

This isn't really a good approach.

lib/Driver/ToolChains/MSVC.h

 //===--- MSVC.h - MSVC ToolChain Implementations ----------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_MSVC_H
 #define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_MSVC_H
 
 #include "Cuda.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Tool.h"
 #include "clang/Driver/ToolChain.h"
+#include "llvm/ADT/SmallString.h"
 
 namespace clang {
 namespace driver {
 namespace tools {
 
 /// Visual studio tools.
 namespace visualstudio {
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList &DriverArgs,
                                      llvm::opt::ArgStringList &CC1Args,
                                      const std::string &folder,
                                      const Twine &subfolder1,
                                      const Twine &subfolder2 = "",
                                      const Twine &subfolder3 = "") const;
 
   Tool *buildLinker() const override;
   Tool *buildAssembler() const override;
+  bool findVCToolChainViaEnvironment(std::string &Path,
+                                     bool &IsVS2017OrNewer);
 private:
   std::string VCToolChainPath;
   bool IsVS2017OrNewer = false;
   CudaInstallationDetector CudaInstallation;
+  llvm::SmallString<128> linkPath;
+  mutable VersionTuple MSVT;
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_MSVC_H

lib/Driver/ToolChains/MSVC.cpp

 #endif
 
 using namespace clang::driver;
 using namespace clang::driver::toolchains;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
+static VersionTuple getMSVCVersionFromExe(const std::string &BinDir);
 // 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 various environment variables to try and find a toolchain.
-static bool findVCToolChainViaEnvironment(std::string &Path,
+bool MSVCToolChain::findVCToolChainViaEnvironment(std::string &Path,
                                                   bool &IsVS2017OrNewer) {
   // 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
     // the toolchain directory.
     Path = std::move(*VCToolsInstallDir);
     IsVS2017OrNewer = true;
for (llvm::StringRef PathEntry : PathEntries) {
       llvm::SmallString<256> ExeTestPath;
 
       // If cl.exe doesn't exist, then this definitely isn't a VC toolchain.
       ExeTestPath = PathEntry;
       llvm::sys::path::append(ExeTestPath, "cl.exe");
       if (!llvm::sys::fs::exists(ExeTestPath))
         continue;
 
+      MSVT = getMSVCVersionFromExe(PathEntry);
+      if (MSVT.empty())
+        continue;
+
       // cl.exe existing isn't a conclusive test for a VC toolchain; clang also
       // has a cl.exe. So let's check for link.exe too.
       ExeTestPath = PathEntry;
       llvm::sys::path::append(ExeTestPath, "link.exe");
       if (!llvm::sys::fs::exists(ExeTestPath))
         continue;
 
-      // whatever/VC/bin --> old toolchain, VC dir is toolchain dir.
-      llvm::StringRef TestPath = PathEntry;
-      bool IsBin = llvm::sys::path::filename(TestPath).equals_lower("bin");
-      if (!IsBin) {
-        // Strip any architecture subdir like "amd64".
+      // The install directory will be the parent of the include directory,
+      // which must be an higher up in the path from cl.exe.
+      llvm::SmallString<256> TestPath = PathEntry;
+      while(!TestPath.empty()) {
         TestPath = llvm::sys::path::parent_path(TestPath);
-        IsBin = llvm::sys::path::filename(TestPath).equals_lower("bin");
+        llvm::sys::path::append(TestPath, "include");
+        if (llvm::sys::fs::exists(TestPath)) {
+          Path = llvm::sys::path::parent_path(TestPath);
+          break;
         }
-      if (IsBin) {
-        llvm::StringRef ParentPath = llvm::sys::path::parent_path(TestPath);
-        if (llvm::sys::path::filename(ParentPath) == "VC") {
-          Path = ParentPath;
-          IsVS2017OrNewer = false;
-          return true;
-        }
 
-      } else {
-        // This could be a new (>=VS2017) toolchain. If it is, we should find
-        // path components with these prefixes when walking backwards through
-        // the path.
-        // Note: empty strings match anything.
-        llvm::StringRef ExpectedPrefixes[] = {"",     "Host",  "bin", "",
-                                              "MSVC", "Tools", "VC"};
-
-        auto It = llvm::sys::path::rbegin(PathEntry);
-        auto End = llvm::sys::path::rend(PathEntry);
-        for (llvm::StringRef Prefix : ExpectedPrefixes) {
-          if (It == End)
-            goto NotAToolChain;
-          if (!It->startswith(Prefix))
-            goto NotAToolChain;
-          ++It;
-        }
-
-        // We've found a new toolchain!
-        // Back up 3 times (/bin/Host/arch) to get the root path.
-        llvm::StringRef ToolChainPath(PathEntry);
-        for (int i = 0; i < 3; ++i)
-          ToolChainPath = llvm::sys::path::parent_path(ToolChainPath);
+      if (Path.empty())
+        return false;
 
-        Path = ToolChainPath;
-        IsVS2017OrNewer = true;
-        return true;
-      }
-
-    NotAToolChain:
-      continue;
-    }
+      linkPath = ExeTestPath;
+      IsVS2017OrNewer = (MSVT.getMajor() >= 19 && MSVT.getMinor());
   }
   return false;
 }
 
 // Query the Setup Config server for installs, then pick the newest version
 // and find its default VC toolchain.
 // This is the preferred way to discover new Visual Studios, as they're no
 // longer listed in the registry.
void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   TC.addProfileRTLibs(Args, CmdArgs);
 
   std::vector<const char *> Environment;
 
   // We need to special case some linker paths.  In the case of lld, we need to
   // translate 'lld' into 'lld-link', and in the case of the regular msvc
   // linker, we need to use a special search algorithm.
   llvm::SmallString<128> linkPath;
   StringRef Linker = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "link");
   if (Linker.equals_lower("lld"))
     Linker = "lld-link";
 
   if (Linker.equals_lower("link")) {
     // If we're using the MSVC linker, it's not sufficient to just use link
     // from the program PATH, because other environments like GnuWin32 install
     // their own link.exe which may come first.
+    if (linkPath.empty())
       linkPath = FindVisualStudioExecutable(TC, "link.exe");
 
 #ifdef USE_WIN32
     // When cross-compiling with VS2017 or newer, link.exe expects to have
     // its containing bin directory at the top of PATH, followed by the
     // native target bin directory.
     // e.g. when compiling for x86 on an x64 host, PATH should start with:
     // /bin/HostX64/x86;/bin/HostX64/x64
     if (TC.getIsVS2017OrNewer() &&
VersionTuple MSVCToolChain::computeMSVCVersion(const Driver *D,
   return MSVT;
 }
 
 std::string
 MSVCToolChain::ComputeEffectiveClangTriple(const ArgList &Args,
                                            types::ID InputType) const {
   // The MSVC version doesn't care about the architecture, even though it
   // may look at the triple internally.
-  VersionTuple MSVT = computeMSVCVersion(/*D=*/nullptr, Args);
+  if (MSVT.empty())
+    MSVT = computeMSVCVersion(/*D=*/nullptr, Args);
   MSVT = VersionTuple(MSVT.getMajor(), MSVT.getMinor().getValueOr(0),
                       MSVT.getSubminor().getValueOr(0));
 
   // For the rest of the triple, however, a computed architecture name may
   // be needed.
   llvm::Triple Triple(ToolChain::ComputeEffectiveClangTriple(Args, InputType));
   if (Triple.getEnvironment() == llvm::Triple::MSVC) {
     StringRef ObjFmt = Triple.getEnvironmentName().split('-').second;