[Driver] Updated for Visual Studio 2017

[hamzasood]

Summary

The patch updates the MSVC ToolChain for the changes made in Visual Studio 2017 (https://blogs.msdn.microsoft.com/vcblog/2016/10/07/compiler-tools-layout-in-visual-studio-15/).
Other notable changes:

• Path handling code has been centralised to make potential future changes less painful.
• A compiler error is emitted if the driver is unable to locate a usable MSVC toolchain. (Previously it'd fail with a cryptic error such as "link.exe is not executable")
• Support for the new Setup Config Server API has been added, albeit block commented out with a preprocessor conditional. This can probably be re-evaluated when the API is officially released (it's currently at the RC stage), but it's left in to make it easy for anyone familiar with the API to give it a go with Clang.

Patch by Hamza Sood.

[hamzasood]

• Fixed an error in retrieving a toolchain path from the registry.
• Updated the base revision.

[amccarth]

(I could be missing something because the diff doesn't have much context.)

If I'm reading this right, it looks like it will no longer search the PATH in order to find cl.exe. If that's the case, then I'm mildly worried about this change in behavior. I know I have multiple versions of VS installed, and commonly select one just by moving its bin directory to the head of PATH. The old behavior would find that one (which is used for things like defaulting -fms-compatibility-version) while the new behavior will find the newest one installed on the machine.

That's not necessarily a deal breaker, but it's an important change for Windows developers to be aware of.

[hamzasood]

• Re-implemented the PATH searching behaviour (thanks @amccarth for pointing that out)
• Updated the base revision.

[hamzasood]

Improved the code slightly.
Sorry for the spam everyone, this is definitely the one.

[rnk]

The new MSVC layout suggests to me that we should look try looking at the path of cl.exe before we open the exe and try to check its version. We'd change this code to inspect the path before looking in the exe:

VersionTuple MSVCToolChain::computeMSVCVersion(const Driver *D,
                                               const ArgList &Args) const {
  bool IsWindowsMSVC = getTriple().isWindowsMSVCEnvironment();
  VersionTuple MSVT = ToolChain::computeMSVCVersion(D, Args);
  if (MSVT.empty()) MSVT = getMSVCVersionFromTriple();
  if (MSVT.empty() && IsWindowsMSVC) MSVT = getMSVCVersionFromExe();
  if (MSVT.empty() &&
      Args.hasFlag(options::OPT_fms_extensions, options::OPT_fno_ms_extensions,
                   IsWindowsMSVC)) {
    // -fms-compatibility-version=18.00 is default.
    // FIXME: Consider bumping this to 19 (MSVC2015) soon.
    MSVT = VersionTuple(18);
  }
  return MSVT;
}

That can definitely be a separate change, though.

lib/Driver/MSVCToolChain.cpp

77

Thanks for rewriting this, the logic here seems much clearer.

187

Is this really supported? Are these libraries prepared to return failure HRESULT values if an exception isn't thrown?

Also, won't this suppress exceptions that would normally be internal to COM, or does that never happen?

188

This is a very surprising use of unique_ptr. I think it would be clearer written this way:

struct RestoreCOMErrorHandler {
  ~RestoreCOMErrorHandler() { _set_com_error_handler(_com_raise_error); }
} COMErrorRestorer;

287

Think there's a better name for this than "SubDirectoryName"? It sounds like these are common to the Win8+ SDk and the VC2017+ tools. We could still call it the "WindowsSDKArch", since that's the package that initially adopted this naming convention.

305

For consistency, please make this a static helper like the helper above.

[hamzasood]

• Modify the environment in some cases in buildLinker to keep link.exe happy.

Thanks @rnk for taking the time to look at this. As suggested:

• Replaced the confusing unique_ptr usage with a struct.
• Renamed llvmArchToSubDirectoryName to llvmArchToWindowsSDKArch.
• Replaced the nested function in getSubDirectoryPath with a static helper.

With regard to _set_com_error_handler, it's a documented function so it is supported. It's not part of the actual COM API, just a wrapper library that sits on top of it. That library is a header only library with inline functions, so you can see how it's used. It checks the HRESULTs before returning them and if the result isn't successful then it calls the error handler; the original HRESULT still gets returned afterwards. You can also see from the headers that it isn't being relied on to process any kind of internal error, so it should be perfectly safe to block out for a bit.

[hamzasood]

• Added an option to set the environment in a clang::driver::Command, which makes the environment modifying introduced in the last update a bit more reliable.

@rnk I looked into using the new MSVC toolchain layout to get a version number without opening an exe, but it doesn't look like it'll be possible. The version number in the toolchain path is the MSVC version number (e.g. Visual Studio 2015 ships with MSVC 14). The version numbers that Clang use are the compiler version numbers (e.g. cl.exe v19 for Visual Studio 2015). As far as I'm aware, there's no mapping between the two.

[awson]

It's weird that cl.exe command-line compiler reports version 19.10.24629 and lives in the 14.10.24629 directory (only first 2 digits are different).

Moreover, in their explanation blogpost they claim that There’s a subdirectory in the MSVC directory with a compiler version number. For VS 2017 RC, that version number is 14.10.24629., i.e., they name this 14.10.24629 a "compiler version".

[hamzasood]

In D28365#639260, @awson wrote:

It's weird th[[ URL | name ]]at cl.exe command-line compiler reports version 19.10.24629 and lives in the 14.10.24629 directory (only first 2 digits are different).

Moreover, in their explanation blogpost they claim that There’s a subdirectory in the MSVC directory with a compiler version number. For VS 2017 RC, that version number is 14.10.24629., i.e., they name this 14.10.24629 a "compiler version".

I'll admit I'm also slightly confused by the difference and maybe someone more familiar with the matter can correct me, but from what I understand: v14 is the toolchain version and v19 is the cl.exe version (i.e. specifically the compiler version). Wikipedia has a useful table showing cl.exe version numbers over the years. I've got no idea why they're kept separate, probably historical reasons.
As for the blog post, I can only assume that it's a typo and they meant to say "compiler tools version".

[rnk]

In D28365#639253, @hamzasood wrote:

• Added an option to set the environment in a clang::driver::Command, which makes the environment modifying introduced in the last update a bit more reliable.

@rnk I looked into using the new MSVC toolchain layout to get a version number without opening an exe, but it doesn't look like it'll be possible. The version number in the toolchain path is the MSVC version number (e.g. Visual Studio 2015 ships with MSVC 14). The version numbers that Clang use are the compiler version numbers (e.g. cl.exe v19 for Visual Studio 2015). As far as I'm aware, there's no mapping between the two.

True, you're right. It's definitely out of scope for this change anyway.

---

Can we revert the linker environment change from this patch? It'll be easier to review separately.

include/clang/Basic/DiagnosticDriverKinds.td

282

It would be nice if we had guidelines on writing clang diagnostics somewhere. I think they are supposed to be sentence fragments, and we typically add another sentence fragment with a semi-colon. I'd reword it like this for consistency:

unable to find a Visual Studio installation; try running Clang from a developer command prompt

283

typo on developer

lib/Driver/Job.cpp

306–308 ↗

(On Diff #83555)

Let's not do this, let's store a std::vector<const char *>. We can get the right lifetime with Args.MakeArgString.

[hamzasood]

In D28365#640775, @rnk wrote:

Can we revert the linker environment change from this patch? It'll be easier to review separately.

Sure. But that'll break compiling for x86 on Visual Studio 2017, is that okay?

[rnk]

In D28365#640825, @hamzasood wrote:

In D28365#640775, @rnk wrote:

Can we revert the linker environment change from this patch? It'll be easier to review separately.

Sure. But that'll break compiling for x86 on Visual Studio 2017, is that okay?

Yeah, that's fine, you can't break what doesn't work yet. :)

[hamzasood]

In D28365#640859, @rnk wrote:

In D28365#640825, @hamzasood wrote:

In D28365#640775, @rnk wrote:

Can we revert the linker environment change from this patch? It'll be easier to review separately.

Sure. But that'll break compiling for x86 on Visual Studio 2017, is that okay?

Yeah, that's fine, you can't break what doesn't work yet. :)

Ha, good point. Does that include the environment stuff in Command too or just the linker?

[rnk]

In D28365#640868, @hamzasood wrote:

Ha, good point. Does that include the environment stuff in Command too or just the linker?

Yes, please make the Command environment changes as part of a separate patch with the linker environment changes.

[hamzasood]

• Rephrased the diagnostic message (and fixed an embarrassing typo).
• Reverted the linker environment change for now; building for x86 with VS2017 won't work in the meantime. I'll submit it for review separately after this one has (hopefully) been accepted, as it relies on a few functions introduced in this patch.

[hamzasood]

Uploaded the correct diff this time (yes, really)...
Not sure how to delete the previous one, but it's very incorrect.

[rnk]

lib/Driver/MSVCToolChain.cpp

34

What are the outstanding issues preventing you from enabling this by default?

160

I think it would be clearer if you separated out the PATH search into a separate helper.

263

Similarly, I think this should be a separate helper. It wouldn't need goto.

[hamzasood]

Broke up findVCToolChainPath into a few smaller functions.

[hamzasood]

lib/Driver/MSVCToolChain.cpp

34

Building on Win32 doesn't imply that you'll have the required header; it currently needs to be installed separately via nuget. While it would be possible to have cmake create a packages.config file when configuring a visual studio project, the API is only at the RC stage and so the distribution method could potentially change between now and the Visual Studio 2017 release (even if that's not a concern, it's probably out of the scope of this patch anyway).
Although the code looks useless sitting there ifdefed out, it could be useful for someone eager enough to get the package themselves and run a custom Clang build.
In the meantime, Visual Studio 2017 installations can only be detected when running in the correct developer command prompt, or by putting one of its toolchain's bin directories at the top of PATH.

[hamzasood]

Ping

[rnk]

lgtm

lib/Driver/MSVCToolChain.cpp

34

This patch looks good, so I don't want to block it, but can you add something like:
// FIXME: Use cmake to auto-detect if the necessary headers exist.
I'm assuming we should test for <Setup.Configuration.h> eventually.

[hamzasood]

Added a FIXME comment about configuring use of the Setup Configuration API.
In its current form (RC3) the header is distributed in a nuget package, so it's installed per-project instead of being in a system location that can be automatically detected. This could change for the final release, so it probably isn't worth thinking about it too much yet.

[rnk]

This is ready to land. Do you need someone to commit this?

[hamzasood]

In D28365#664892, @rnk wrote:

This is ready to land. Do you need someone to commit this?

I think so, yeah.

[rnk]

I had to revert this because it doesn't pass tests on Linux. Can you look into that and resubmit after fixing those test failures?

[hamzasood]

In D28365#665183, @rnk wrote:

I had to revert this because it doesn't pass tests on Linux. Can you look into that and resubmit after fixing those test failures?

Really sorry about that, I stupidly only ran the tests on Windows.
The problem was caused by a change in behaviour when Clang is unable to find a Visual Studio installation. This patch made it emit an error and abort compilation in an attempt to be user friendly, which interferes with some tests if triggered (e.g. on Linux where there's no Visual Studio to find).
I've updated it to only emit a warning and continue compilation as usual, which seemed like a better option than removing it completely.

[rnk]

Doesn't your fix mean that the tests will fail on a Windows machine that doesn't have VS because LLVM was built with mingw? Usually in these situations we provide some way to provide a fake toolchain.

[hamzasood]

In D28365#665408, @rnk wrote:

Doesn't your fix mean that the tests will fail on a Windows machine that doesn't have VS because LLVM was built with mingw? Usually in these situations we provide some way to provide a fake toolchain.

I'm not sure. It should be the same behaviour as before, which is that it'll keep going regardless of whether it can find an MSVC instance. Was there some fake toolchain providing that I missed in the old code?

include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_unsupported_linker : Error<"unsupported value '%0' for -linker option">;
 def err_drv_defsym_invalid_format : Error<"defsym must be of the form: sym=value: %0">;
 def err_drv_defsym_invalid_symval : Error<"Value is not an integer: %0">;
+
+def err_drv_msvc_not_found : Error<
+  "unable to find a Visual Studio installation. "
+  "Try re-running Clang from a devleoper command prompt">;
 }

lib/Driver/MSVCToolChain.cpp

 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include <cstdio>
 
-// Include the necessary headers to interface with the Windows registry and
-// environment.
 #if defined(LLVM_ON_WIN32)
   #define USE_WIN32
+  #if 0
+    #define USE_VS_SETUP_CONFIG
+  #endif
 #endif
 
+// Include the necessary headers to interface with the Windows registry and
+// environment.
 #ifdef USE_WIN32
   #define WIN32_LEAN_AND_MEAN
   #define NOGDI

   #include <windows.h>
 #endif
 
+// Include the headers needed for the setup config COM stuff and define
+// smart pointers for the interfaces we need.
+#ifdef USE_VS_SETUP_CONFIG
+  #include "clang/Basic/VirtualFileSystem.h"
+  #include "llvm/Support/COM.h"
+  #include <comdef.h>
+  #include <memory>
+  #include <Setup.Configuration.h>
+  _COM_SMARTPTR_TYPEDEF(ISetupConfiguration,  __uuidof(ISetupConfiguration));
+  _COM_SMARTPTR_TYPEDEF(ISetupConfiguration2, __uuidof(ISetupConfiguration2));
+  _COM_SMARTPTR_TYPEDEF(ISetupHelper,         __uuidof(ISetupHelper));
+  _COM_SMARTPTR_TYPEDEF(IEnumSetupInstances,  __uuidof(IEnumSetupInstances));
+  _COM_SMARTPTR_TYPEDEF(ISetupInstance,       __uuidof(ISetupInstance));
+  _COM_SMARTPTR_TYPEDEF(ISetupInstance2,      __uuidof(ISetupInstance2));
+
+#endif
+
 using namespace clang::driver;
 using namespace clang::driver::toolchains;
 using namespace clang;
 using namespace llvm::opt;
 
-MSVCToolChain::MSVCToolChain(const Driver &D, const llvm::Triple &Triple,
+// 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);
+
+// Attempts to find the "best" usable VC toolchain.
+static bool findVCToolChainPath(std::string &Path, bool &IsVS2017OrNewer) {
+  // Check the environment first, since that's probably the user telling us
+  // what they want to use. 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;
+    return true;
+  }
+  if (llvm::Optional<std::string> VCInstallDir =
+          llvm::sys::Process::GetEnv("VCINSTALLDIR")) {
+    // If the previous variable isn't set but this one is, then we've found
+    // an older Visual Studio. This variable is set by newer Visual Studios too,
+    // so this check has to appear second.
+    // In older Visual Studios, the VC directory is the toolchain.
+    Path = std::move(*VCInstallDir);
+    IsVS2017OrNewer = false;
+    return true;
+  }
+
+  // We reach this point if the environment doesn't lead to an install,
+  // and so we probably aren't being run from a developer command prompt.
+  // The plan from now is to find the newest Visual Studio version we can, and
+  // use its default VC toolchain..
+  // TODO: Perhaps have an option to let the user select the toolchain that
+  //       they want to use?
+  
+#ifdef USE_VS_SETUP_CONFIG
+  // Query the Setup Config server for installs.
+  // This is the preferred way to discover new Visual Studios, as they're no
+  // longer listed in the registry.
+  // An explicit scope is established to ensure the COM pointers get released
+  // as soon as we're done with them.
+  {
+    llvm::sys::InitializeCOMRAII COM(llvm::sys::COMThreadingMode::SingleThreaded);
+    HRESULT HR;
+
+    // _com_ptr_t will throw a _com_error if a COM calls fail.
+    // The LLVM coding standards forbid exception handling, so we'll have to
+    // stop them from being thrown in the first place.
+    // The unique_ptr will put the regular error handler back when we leave
+    // this scope.
+    _set_com_error_handler([](HRESULT, IErrorInfo *) { });
+    std::unique_ptr<decltype(*_com_raise_error),
+                    decltype(&_set_com_error_handler)>
+        ComErrorHandlerPtr(&_com_raise_error, &_set_com_error_handler);
+
+    ISetupConfigurationPtr Query;
+    HR = Query.CreateInstance(__uuidof(SetupConfiguration));
+    if (FAILED(HR)) goto ConfigQueryUnsuccessful;
+
+    IEnumSetupInstancesPtr EnumInstances;
+    HR = ISetupConfiguration2Ptr(Query)->EnumAllInstances(&EnumInstances);
+    if (FAILED(HR)) goto ConfigQueryUnsuccessful;
+
+    ISetupInstancePtr Instance;
+    HR = EnumInstances->Next(1, &Instance, nullptr);
+    if (HR != S_OK) goto ConfigQueryUnsuccessful;
+
+    ISetupInstancePtr NewestInstance(Instance);
+    uint64_t NewestVersionNum;
+    {
+      bstr_t VersionString;
+      HR = NewestInstance->GetInstallationVersion(VersionString.GetAddress());
+      if (FAILED(HR)) goto ConfigQueryUnsuccessful;
+      HR = ISetupHelperPtr(Query)->ParseVersion(VersionString,
+                                                &NewestVersionNum);
+      if (FAILED(HR)) goto ConfigQueryUnsuccessful;
+    }
+
+    while ((HR = EnumInstances->Next(1, &Instance, nullptr)) == S_OK) {
+      bstr_t VersionString;
+      uint64_t VersionNum;
+      HR = Instance->GetInstallationVersion(VersionString.GetAddress());
+      if (FAILED(HR)) continue;
+      HR = ISetupHelperPtr(Query)->ParseVersion(VersionString,
+                                                &VersionNum);
+      if (FAILED(HR)) continue;
+      if (VersionNum > NewestVersionNum) {
+        NewestInstance = Instance;
+        NewestVersionNum = VersionNum;
+      }
+    }
+
+    bstr_t VCPathWide;
+    HR = NewestInstance->ResolvePath(L"VC",
+                                     VCPathWide.GetAddress());
+    if (FAILED(HR)) goto ConfigQueryUnsuccessful;
+
+    std::string VCRootPath;
+    llvm::convertWideToUTF8(std::wstring(VCPathWide), VCRootPath);
+
+    llvm::SmallString<256> ToolsVersionFilePath(VCRootPath);
+    llvm::sys::path::append(ToolsVersionFilePath,
+                            "Auxiliary",
+                            "Build",
+                            "Microsoft.VCToolsVersion.default.txt");
+
+    auto ToolsVersionFile =
+        clang::vfs::getRealFileSystem()->getBufferForFile(ToolsVersionFilePath);
+    if (!ToolsVersionFile)
+      goto ConfigQueryUnsuccessful;
+
+    llvm::SmallString<256> ToolchainPath(VCRootPath);
+    llvm::sys::path::append(ToolchainPath,
+                            "Tools",
+                            "MSVC",
+                            ToolsVersionFile->get()->getBuffer().rtrim());
+    if (!llvm::sys::fs::is_directory(ToolchainPath))
+      goto ConfigQueryUnsuccessful;
+
+    Path = ToolchainPath.str();
+    IsVS2017OrNewer = true;
+    return true;
+  }
+
+ConfigQueryUnsuccessful:
+#endif /*USE_VS_SETUP_CONFIG*/
+
+  // If the environment checks and Setup Config queries are unsuccessful,
+  // our last hope is that we can find an install in the registry.
+  // VS2017 and newer don't get added to the registry. So if we find something
+  // here, we know that it's an older version.
+  std::string VSInstallPath;
+  if (getSystemRegistryString(R"(SOFTWARE\Microsoft\VisualStudio\$VERSION)",
+                              "InstallDir", VSInstallPath, nullptr) ||
+      getSystemRegistryString(R"(SOFTWARE\Microsoft\VCExpress\$VERSION)",
+                              "InstallDir", VSInstallPath, nullptr)) {
+    if (!VSInstallPath.empty()) {
+      llvm::SmallString<256>
+          VCPath(llvm::StringRef(VSInstallPath.c_str(),
+                                 VSInstallPath.find(R"(\Common7\IDE)")));
+      llvm::sys::path::append(VCPath, "VC");
+
+      Path = VCPath.str();
+      IsVS2017OrNewer = false;
+      return true;
+    }
+  }
+
+  return false;
+}
+
+MSVCToolChain::MSVCToolChain(const Driver &D, const llvm::Triple& Triple,
                              const ArgList &Args)
     : ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args) {
   getProgramPaths().push_back(getDriver().getInstalledDir());
   if (getDriver().getInstalledDir() != getDriver().Dir)
     getProgramPaths().push_back(getDriver().Dir);
+  findVCToolChainPath(VCToolChainPath, IsVS2017OrNewer);
 }
 
 Tool *MSVCToolChain::buildLinker() const {
+  if (VCToolChainPath.empty()) {
+    getDriver().Diag(clang::diag::err_drv_msvc_not_found);
+    return nullptr;
+  }
   return new tools::visualstudio::Linker(*this);
 }
 

   CudaInstallation.print(OS);
 }
 
+// Windows SDKs and VC Toolchains group their contents into subdirectories based
+// on the target architecture. This function converts an llvm::Triple::ArchType
+// to the corresponding subdirectory name.
+static const char *llvmArchToSubDirectoryName(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
+  switch (Arch) {
+  case ArchType::x86:
+    return "x86";
+  case ArchType::x86_64:
+    return "x64";
+  case ArchType::arm:
+    return "arm";
+  default:
+    return "";
+  }
+}
+
+// Get the path to a specific subdirectory in the current toolchain.
+// VS2017 changed the VC toolchain layout, so this should be used instead
+// of hardcoding paths.
+std::string MSVCToolChain::getSubDirectoryPath(SubDirectoryType Type) const {
+  auto llvmArchToLegacyVCSubDirectoryName =
+      [](llvm::Triple::ArchType Arch) -> const char * {
+    using ArchType = llvm::Triple::ArchType;
+    switch (Arch) {
+    case ArchType::x86:
+      // x86 is default in legacy VC toolchains.
+      // e.g. x86 libs are directly in /lib as opposed to /lib/x86.
+      return "";
+    case ArchType::x86_64:
+      return "amd64";
+    case ArchType::arm:
+      return "arm";
+    default:
+      return "";
+    }
+  };
+
+  llvm::SmallString<256> Path(VCToolChainPath);
+  switch (Type) {
+  case SubDirectoryType::Bin:
+    if (IsVS2017OrNewer) {
+      bool HostIsX64 = llvm::Triple(llvm::sys::getProcessTriple()).isArch64Bit();
+      llvm::sys::path::append(Path,
+                              "bin",
+                              (HostIsX64 ? "HostX64" : "HostX86"),
+                              llvmArchToSubDirectoryName(getArch()));
+    } else {
+      llvm::sys::path::append(Path,
+                              "bin",
+                              llvmArchToLegacyVCSubDirectoryName(getArch()));
+    }
+    break;
+  case SubDirectoryType::Include:
+    llvm::sys::path::append(Path, "include");
+    break;
+  case SubDirectoryType::Lib:
+    llvm::sys::path::append(Path,
+        "lib",
+        IsVS2017OrNewer
+            ? llvmArchToSubDirectoryName(getArch())
+            : llvmArchToLegacyVCSubDirectoryName(getArch()));
+    break;
+  }
+  return Path.str();
+}
+
 #ifdef USE_WIN32
 static bool readFullStringValue(HKEY hkey, const char *valueName,
                                 std::string &value) {

 #endif // USE_WIN32
 }
 
-// Convert LLVM's ArchType
-// to the corresponding name of Windows SDK libraries subfolder
-static StringRef getWindowsSDKArch(llvm::Triple::ArchType Arch) {
-  switch (Arch) {
-  case llvm::Triple::x86:
-    return "x86";
-  case llvm::Triple::x86_64:
-    return "x64";
-  case llvm::Triple::arm:
-    return "arm";
-  default:
-    return "";
-  }
-}
-
 // 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 getWindows10SDKVersion(const std::string &SDKPath,
+static bool getWindows10SDKVersionFromPath(const std::string &SDKPath,
                                            std::string &SDKVersion) {
   SDKVersion.clear();
 
   std::error_code EC;

 }
 
 /// \brief Get Windows SDK installation directory.
-bool MSVCToolChain::getWindowsSDKDir(std::string &Path, int &Major,
-                                     std::string &WindowsSDKIncludeVersion,
-                                     std::string &WindowsSDKLibVersion) const {
+static bool getWindowsSDKDir(std::string &Path, int &Major,
+                             std::string &WindowsSDKLibVersion) {
   std::string RegistrySDKVersion;
   // Try the Windows registry.
   if (!getSystemRegistryString(

     return !WindowsSDKLibVersion.empty();
   }
   if (Major == 10) {
-    if (!getWindows10SDKVersion(Path, WindowsSDKIncludeVersion))
+    if (!getWindows10SDKVersionFromPath(Path, WindowsSDKIncludeVersion))
       return false;
     WindowsSDKLibVersion = WindowsSDKIncludeVersion;
     return true;

 
   llvm::SmallString<128> libPath(sdkPath);
   llvm::sys::path::append(libPath, "Lib");
-  if (sdkMajor <= 7) {
+  if (sdkMajor >= 8) {
+    llvm::sys::path::append(libPath,
+      windowsSDKLibVersion,
+      "um",
+      llvmArchToSubDirectoryName(getArch()));
+  } else {
     switch (getArch()) {
       // In Windows SDK 7.x, x86 libraries are directly in the Lib folder.
     case llvm::Triple::x86:
       break;
     case llvm::Triple::x86_64:

     default:
       return false;
     }
-  } else {
-    const StringRef archName = getWindowsSDKArch(getArch());
-    if (archName.empty())
-      return false;
-    llvm::sys::path::append(libPath, windowsSDKLibVersion, "um", archName);
   }
 
   path = libPath.str();

 
 // Check if the Include path of a specified version of Visual Studio contains
 // specific header files. If not, they are probably shipped with Universal CRT.
-bool clang::driver::toolchains::MSVCToolChain::useUniversalCRT(
-    std::string &VisualStudioDir) const {
-  llvm::SmallString<128> TestPath(VisualStudioDir);
-  llvm::sys::path::append(TestPath, "VC\\include\\stdlib.h");
-
+bool MSVCToolChain::useUniversalCRT() const {
+  llvm::SmallString<128> TestPath(getSubDirectoryPath(SubDirectoryType::Include));
+  llvm::sys::path::append(TestPath, "stdlib.h");
   return !llvm::sys::fs::exists(TestPath);
 }
 
-bool MSVCToolChain::getUniversalCRTSdkDir(std::string &Path,
-                                          std::string &UCRTVersion) const {
+static bool getUniversalCRTSdkDir(std::string &Path,
+                                  std::string &UCRTVersion) {
   // vcvarsqueryregistry.bat for Visual Studio 2015 queries the registry
   // for the specific key "KitsRoot10". So do we.
   if (!getSystemRegistryString(
-          "SOFTWARE\\Microsoft\\Windows Kits\\Installed Roots", "KitsRoot10",
-          Path, nullptr))
+          "SOFTWARE\\Microsoft\\Windows Kits\\Installed Roots",
+          "KitsRoot10", Path, nullptr))
     return false;
 
-  return getWindows10SDKVersion(Path, UCRTVersion);
+  return getWindows10SDKVersionFromPath(Path, UCRTVersion);
 }
 
 bool MSVCToolChain::getUniversalCRTLibraryPath(std::string &Path) const {

   if (!getUniversalCRTSdkDir(UniversalCRTSdkPath, UCRTVersion))
     return false;
 
-  StringRef ArchName = getWindowsSDKArch(getArch());
+  StringRef ArchName = llvmArchToSubDirectoryName(getArch());
   if (ArchName.empty())
     return false;
 

   return true;
 }
 
-// Get the location to use for Visual Studio binaries.  The location priority
-// is: %VCINSTALLDIR% > %PATH% > newest copy of Visual Studio installed on
-// system (as reported by the registry).
-bool MSVCToolChain::getVisualStudioBinariesFolder(const char *clangProgramPath,
-                                                  std::string &path) const {
-  path.clear();
-
-  SmallString<128> BinDir;
-
-  // First check the environment variables that vsvars32.bat sets.
-  llvm::Optional<std::string> VcInstallDir =
-      llvm::sys::Process::GetEnv("VCINSTALLDIR");
-  if (VcInstallDir.hasValue()) {
-    BinDir = VcInstallDir.getValue();
-    llvm::sys::path::append(BinDir, "bin");
-  } else {
-    // Next walk the PATH, trying to find a cl.exe in the path.  If we find one,
-    // use that.  However, make sure it's not clang's cl.exe.
-    llvm::Optional<std::string> OptPath = llvm::sys::Process::GetEnv("PATH");
-    if (OptPath.hasValue()) {
-      const char EnvPathSeparatorStr[] = {llvm::sys::EnvPathSeparator, '\0'};
-      SmallVector<StringRef, 8> PathSegments;
-      llvm::SplitString(OptPath.getValue(), PathSegments, EnvPathSeparatorStr);
-
-      for (StringRef PathSegment : PathSegments) {
-        if (PathSegment.empty())
-          continue;
-
-        SmallString<128> FilePath(PathSegment);
-        llvm::sys::path::append(FilePath, "cl.exe");
-        // Checking if cl.exe exists is a small optimization over calling
-        // can_execute, which really only checks for existence but will also do
-        // extra checks for cl.exe.exe.  These add up when walking a long path.
-        if (llvm::sys::fs::exists(FilePath.c_str()) &&
-            !llvm::sys::fs::equivalent(FilePath.c_str(), clangProgramPath)) {
-          // If we found it on the PATH, use it exactly as is with no
-          // modifications.
-          path = PathSegment;
-          return true;
-        }
-      }
-    }
-
-    std::string installDir;
-    // With no VCINSTALLDIR and nothing on the PATH, if we can't find it in the
-    // registry then we have no choice but to fail.
-    if (!getVisualStudioInstallDir(installDir))
-      return false;
-
-    // Regardless of what binary we're ultimately trying to find, we make sure
-    // that this is a Visual Studio directory by checking for cl.exe.  We use
-    // cl.exe instead of other binaries like link.exe because programs such as
-    // GnuWin32 also have a utility called link.exe, so cl.exe is the least
-    // ambiguous.
-    BinDir = installDir;
-    llvm::sys::path::append(BinDir, "VC", "bin");
-    SmallString<128> ClPath(BinDir);
-    llvm::sys::path::append(ClPath, "cl.exe");
-
-    if (!llvm::sys::fs::can_execute(ClPath.c_str()))
-      return false;
-  }
-
-  if (BinDir.empty())
-    return false;
-
-  switch (getArch()) {
-  case llvm::Triple::x86:
-    break;
-  case llvm::Triple::x86_64:
-    llvm::sys::path::append(BinDir, "amd64");
-    break;
-  case llvm::Triple::arm:
-    llvm::sys::path::append(BinDir, "arm");
-    break;
-  default:
-    // Whatever this is, Visual Studio doesn't have a toolchain for it.
-    return false;
-  }
-  path = BinDir.str();
-  return true;
-}
-
 VersionTuple MSVCToolChain::getMSVCVersionFromTriple() const {
   unsigned Major, Minor, Micro;
   getTriple().getEnvironmentVersion(Major, Minor, Micro);

 VersionTuple MSVCToolChain::getMSVCVersionFromExe() const {
   VersionTuple Version;
 #ifdef USE_WIN32
-  std::string BinPath;
-  if (!getVisualStudioBinariesFolder("", BinPath))
-    return Version;
-  SmallString<128> ClExe(BinPath);
+  SmallString<128> ClExe(getSubDirectoryPath(SubDirectoryType::Bin));
   llvm::sys::path::append(ClExe, "cl.exe");
 
   std::wstring ClExeWide;

   return Version;
 }
 
-// Get Visual Studio installation directory.
-bool MSVCToolChain::getVisualStudioInstallDir(std::string &path) const {
-  // First check the environment variables that vsvars32.bat sets.
-  if (llvm::Optional<std::string> VcInstallDir =
-          llvm::sys::Process::GetEnv("VCINSTALLDIR")) {
-    path = std::move(*VcInstallDir);
-    path = path.substr(0, path.find("\\VC"));
-    return true;
-  }
-
-  std::string vsIDEInstallDir;
-  std::string vsExpressIDEInstallDir;
-  // Then try the windows registry.
-  bool hasVCDir =
-      getSystemRegistryString("SOFTWARE\\Microsoft\\VisualStudio\\$VERSION",
-                              "InstallDir", vsIDEInstallDir, nullptr);
-  if (hasVCDir && !vsIDEInstallDir.empty()) {
-    path = vsIDEInstallDir.substr(0, vsIDEInstallDir.find("\\Common7\\IDE"));
-    return true;
-  }
-
-  bool hasVCExpressDir =
-      getSystemRegistryString("SOFTWARE\\Microsoft\\VCExpress\\$VERSION",
-                              "InstallDir", vsExpressIDEInstallDir, nullptr);
-  if (hasVCExpressDir && !vsExpressIDEInstallDir.empty()) {
-    path = vsExpressIDEInstallDir.substr(
-        0, vsIDEInstallDir.find("\\Common7\\IDE"));
-    return true;
-  }
-
-  // Try the environment.
-  std::string vcomntools;
-  if (llvm::Optional<std::string> vs120comntools =
-          llvm::sys::Process::GetEnv("VS120COMNTOOLS"))
-    vcomntools = std::move(*vs120comntools);
-  else if (llvm::Optional<std::string> vs100comntools =
-               llvm::sys::Process::GetEnv("VS100COMNTOOLS"))
-    vcomntools = std::move(*vs100comntools);
-  else if (llvm::Optional<std::string> vs90comntools =
-               llvm::sys::Process::GetEnv("VS90COMNTOOLS"))
-    vcomntools = std::move(*vs90comntools);
-  else if (llvm::Optional<std::string> vs80comntools =
-               llvm::sys::Process::GetEnv("VS80COMNTOOLS"))
-    vcomntools = std::move(*vs80comntools);
-
-  // Find any version we can.
-  if (!vcomntools.empty()) {
-    size_t p = vcomntools.find("\\Common7\\Tools");
-    if (p != std::string::npos)
-      vcomntools.resize(p);
-    path = std::move(vcomntools);
-    return true;
-  }
-  return false;
-}
-
 void MSVCToolChain::AddSystemIncludeWithSubfolder(
     const ArgList &DriverArgs, ArgStringList &CC1Args,
     const std::string &folder, const Twine &subfolder1, const Twine &subfolder2,

       return;
   }
 
-  std::string VSDir;
-
   // When built with access to the proper Windows APIs, try to actually find
   // the correct include paths first.
-  if (getVisualStudioInstallDir(VSDir)) {
-    AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, VSDir, "VC\\include");
+  if (!VCToolChainPath.empty()) {
+    addSystemInclude(DriverArgs,
+                     CC1Args,
+                     getSubDirectoryPath(SubDirectoryType::Include));
 
-    if (useUniversalCRT(VSDir)) {
+    if (useUniversalCRT()) {
       std::string UniversalCRTSdkPath;
       std::string UCRTVersion;
       if (getUniversalCRTSdkDir(UniversalCRTSdkPath, UCRTVersion)) {

         AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, WindowsSDKDir,
                                       "include");
       }
-    } else {
-      addSystemInclude(DriverArgs, CC1Args, VSDir);
     }
+
     return;
   }
 

lib/Driver/ToolChains.h

   bool isPIEDefault() const override;
   bool isPICDefaultForced() const override;
 
+  enum class SubDirectoryType {
+    Bin,
+    Include,
+    Lib,
+  };
+  std::string getSubDirectoryPath(SubDirectoryType Type) const;
+
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const override;

       llvm::opt::ArgStringList &CC1Args) const override;
 
   void AddCudaIncludeArgs(const llvm::opt::ArgList &DriverArgs,
-                          llvm::opt::ArgStringList &CC1Args) const override;
 
-  bool getWindowsSDKDir(std::string &path, int &major,
-                        std::string &windowsSDKIncludeVersion,
-                        std::string &windowsSDKLibVersion) const;
   bool getWindowsSDKLibraryPath(std::string &path) const;
   /// \brief Check if Universal CRT should be used if available
-  bool useUniversalCRT(std::string &visualStudioDir) const;
-  bool getUniversalCRTSdkDir(std::string &path, std::string &ucrtVersion) const;
+  bool useUniversalCRT() const;
   bool getUniversalCRTLibraryPath(std::string &path) const;
-  bool getVisualStudioInstallDir(std::string &path) const;
-  bool getVisualStudioBinariesFolder(const char *clangProgramPath,
-                                     std::string &path) const;
   VersionTuple
   computeMSVCVersion(const Driver *D,
                      const llvm::opt::ArgList &Args) const override;

 
   Tool *buildLinker() const override;
   Tool *buildAssembler() const override;
+
 private:
+  std::string VCToolChainPath;
+  bool IsVS2017OrNewer;
+
   VersionTuple getMSVCVersionFromTriple() const;
   VersionTuple getMSVCVersionFromExe() const;
 

lib/Driver/Tools.cpp

                                               const char *Exe,
                                               const char *ClangProgramPath) {
   const auto &MSVC = static_cast<const toolchains::MSVCToolChain &>(TC);
-  std::string visualStudioBinDir;
-  if (MSVC.getVisualStudioBinariesFolder(ClangProgramPath,
-                                         visualStudioBinDir)) {
-    SmallString<128> FilePath(visualStudioBinDir);
-    llvm::sys::path::append(FilePath, Exe);
-    if (llvm::sys::fs::can_execute(FilePath.c_str()))
-      return FilePath.str();
-  }
-
-  return Exe;
+  SmallString<128> FilePath(MSVC.getSubDirectoryPath(toolchains::MSVCToolChain
+                                                     ::SubDirectoryType::Bin));
+  llvm::sys::path::append(FilePath, Exe);
+  return (llvm::sys::fs::can_execute(FilePath) ? FilePath.str() : Exe);
 }
 
 void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,

     // 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.
-    std::string VisualStudioDir;
     const auto &MSVC = static_cast<const toolchains::MSVCToolChain &>(TC);
-    if (MSVC.getVisualStudioInstallDir(VisualStudioDir)) {
-      SmallString<128> LibDir(VisualStudioDir);
-      llvm::sys::path::append(LibDir, "VC", "lib");
-      switch (MSVC.getArch()) {
-      case llvm::Triple::x86:
-        // x86 just puts the libraries directly in lib
-        break;
-      case llvm::Triple::x86_64:
-        llvm::sys::path::append(LibDir, "amd64");
-        break;
-      case llvm::Triple::arm:
-        llvm::sys::path::append(LibDir, "arm");
-        break;
-      default:
-        break;
-      }
-      CmdArgs.push_back(
-          Args.MakeArgString(std::string("-libpath:") + LibDir.c_str()));
+    CmdArgs.push_back(Args.MakeArgString(
+                          std::string("-libpath:")
+                          + MSVC.getSubDirectoryPath(toolchains::MSVCToolChain
+                                                     ::SubDirectoryType::Lib)));
 
-      if (MSVC.useUniversalCRT(VisualStudioDir)) {
-        std::string UniversalCRTLibPath;
-        if (MSVC.getUniversalCRTLibraryPath(UniversalCRTLibPath))
-          CmdArgs.push_back(Args.MakeArgString(std::string("-libpath:") +
-                                               UniversalCRTLibPath));
-      }
+    if (MSVC.useUniversalCRT()) {
+        CmdArgs.push_back(Args.MakeArgString(std::string("-libpath:")
+                                             + UniversalCRTLibPath));
     }
 
     std::string WindowsSdkLibPath;