[Driver] Recognize DevDiv internal builds of MSVC, with a different directory structure

[STL_MSFT]

Summary

This is a reasonably non-intrusive change, which I've verified works for both x86 and x64 DevDiv-internal builds.

The idea is to change bool IsVS2017OrNewer into a 3-state ToolsetLayout VSLayout. Either a build is DevDiv-internal, released VS 2017 or newer, or released VS 2015 or older. When looking at the directory structure, if instead of "VC" we see "x86ret", "x86chk", "amd64ret", or "amd64chk", we recognize this as a DevDiv-internal build.

After we get past the directory structure validation, we use this knowledge to regenerate paths appropriately. llvmArchToDevDivInternalArch() knows how we use "i386" subdirectories, and MSVCToolChain::getSubDirectoryPath() uses that. It also knows that DevDiv-internal builds have an "inc" subdirectory instead of "include".

This may still not be the "right" fix in any sense, but I believe that it's non-intrusive in the sense that if the special directory names aren't found, no codepaths are affected. (ToolsetLayout::OlderVS and ToolsetLayout::VS2017OrNewer correspond to IsVS2017OrNewer being false or true, respectively.) I searched for all references to IsVS2017OrNewer, which are places where Clang cares about VS's directory structure, and the only one that isn't being patched is some logic to deal with cross-compilation. I'm fine with that not working for DevDiv-internal builds for the moment (we typically test the native compilers), so I added a comment.

Is there still time to get a patch like this into 5.0.0, or is it too late?

[hintonda]

Looks good, but I'll defer to the owners.

Minor nit...

lib/Driver/ToolChains/MSVC.cpp

142

Perhaps remove the else and grab the subdir name only once before doing comparisons.

[thakis]

This approach looks good to me.

Is there any chance we could have a test for this? See test/Driver for examples; use python bin\llvm-lit.py ../llvm/tools/clang/test/Driver/my_test.cpp to run a single test, ninja check-clang to run all of them. You need some unix binaries to run tests -- if you don't have those flying around, you can download gnuwin-7.zip from http://commondatastorage.googleapis.com/chromium-browser-clang/index.html?path=tools/ and add them to your %PATH%.

[STL_MSFT]

This addresses Don Hinton's feedback. It also unifies IsVS2017OrNewer and IsDevDivInternal into a 3-state enum class ToolsetLayout.

[STL_MSFT]

In D36860#845469, @thakis wrote:

This approach looks good to me.

Thanks!

Is there any chance we could have a test for this?

Unfortunately, testing this involves having a VS toolset with the DevDiv-internal directory structure. Preparing that is a laborious manual process, and requires installation of the Windows SDK (I sent repro instructions to cfe-dev). Even given a real installation of VS, I don't think I could write automated test code to extract a copy with the necessary directory structure (doing so would almost involve replicating the codepaths being touched here). But now that I know that this code exists and that Clang occasionally changes it, especially in response to new VS directory structures, I'll watch for new betas/RCs and verify them against our internal builds.

[thakis]

Many driver tests check in a basic representative directory structure (e.g. test/Driver/Inputs/basic_freebsd_tree/ and its many siblings).

But if you're happy with others breaking this code, I suppose having no tests is ok; I imagine most people won't use this code :-)

[STL_MSFT]

In D36860#846232, @thakis wrote:

Many driver tests check in a basic representative directory structure (e.g. test/Driver/Inputs/basic_freebsd_tree/ and its many siblings).

But if you're happy with others breaking this code, I suppose having no tests is ok; I imagine most people won't use this code :-)

I can't check in copies of cl.exe and link.exe (the former's version is inspected, so it can't just be a dummy file). I'm happy without upstream tests for this logic. In the long run, I hope that we can eliminate this internal directory structure and make it identical to the released directory structure.

lib/Driver/ToolChains/MSVC.h

void AddSystemIncludeWithSubfolder(const llvm::opt::ArgList &DriverArgs,
                                      const Twine &subfolder2 = "",
                                      const Twine &subfolder3 = "") const;
 
   Tool *buildLinker() const override;
   Tool *buildAssembler() const override;
 private:
   std::string VCToolChainPath;
   bool IsVS2017OrNewer = false;
+  bool IsDevDivInternal = false;
   CudaInstallationDetector CudaInstallation;
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_MSVC_H

lib/Driver/ToolChains/MSVC.cpp

 
 // 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 &IsVS2017OrNewer) {
+                                          bool &IsVS2017OrNewer,
+                                          bool &IsDevDivInternal) {
   // 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) {
         IsBin = llvm::sys::path::filename(TestPath).equals_lower("bin");
       }
       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 if (llvm::sys::path::filename(ParentPath) == "x86ret"
+            || llvm::sys::path::filename(ParentPath) == "x86chk"
+            || llvm::sys::path::filename(ParentPath) == "amd64ret"
+            || llvm::sys::path::filename(ParentPath) == "amd64chk") {
+          Path = ParentPath;
+          IsDevDivInternal = true;
+          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", "",
MSVCToolChain::MSVCToolChain(const Driver &D, const llvm::Triple &Triple,
   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.
-  findVCToolChainViaEnvironment(VCToolChainPath, IsVS2017OrNewer) ||
+  findVCToolChainViaEnvironment(VCToolChainPath, IsVS2017OrNewer, IsDevDivInternal) ||
       findVCToolChainViaSetupConfig(VCToolChainPath, IsVS2017OrNewer) ||
       findVCToolChainViaRegistry(VCToolChainPath, IsVS2017OrNewer);
 }
 
 Tool *MSVCToolChain::buildLinker() const {
   if (VCToolChainPath.empty())
     getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
   return new tools::visualstudio::Linker(*this);
case ArchType::x86_64:
     return "amd64";
   case ArchType::arm:
     return "arm";
   default:
     return "";
   }
 }
 
+// Similar to the above function, but for DevDiv internal builds.
+static const char *llvmArchToDevDivInternalArch(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
+  switch (Arch) {
+  case ArchType::x86:
+    return "i386";
+  case ArchType::x86_64:
+    return "amd64";
+  case ArchType::arm:
+    return "arm";
+  default:
+    return "";
+  }
+}
+
 // Get the path to a specific subdirectory in the current toolchain for
 // a given target architecture.
 // VS2017 changed the VC toolchain layout, so this should be used instead
 // of hardcoding paths.
 std::string
 MSVCToolChain::getSubDirectoryPath(SubDirectoryType Type,
                                    llvm::Triple::ArchType TargetArch) const {
   llvm::SmallString<256> Path(VCToolChainPath);
   switch (Type) {
   case SubDirectoryType::Bin:
-    if (IsVS2017OrNewer) {
+    if (IsDevDivInternal) {
+      llvm::sys::path::append(Path, "bin", llvmArchToDevDivInternalArch(TargetArch));
+    } else if (IsVS2017OrNewer) {
       bool HostIsX64 =
           llvm::Triple(llvm::sys::getProcessTriple()).isArch64Bit();
       llvm::sys::path::append(Path, "bin", (HostIsX64 ? "HostX64" : "HostX86"),
                               llvmArchToWindowsSDKArch(TargetArch));
 
     } else {
       llvm::sys::path::append(Path, "bin", llvmArchToLegacyVCArch(TargetArch));
     }
     break;
   case SubDirectoryType::Include:
+    if (IsDevDivInternal) {
+      llvm::sys::path::append(Path, "inc");
+    } else {
       llvm::sys::path::append(Path, "include");
+    }
     break;
   case SubDirectoryType::Lib:
-    llvm::sys::path::append(
-        Path, "lib", IsVS2017OrNewer ? llvmArchToWindowsSDKArch(TargetArch)
-                                     : llvmArchToLegacyVCArch(TargetArch));
+    if (IsDevDivInternal) {
+      llvm::sys::path::append(Path, "lib", llvmArchToDevDivInternalArch(TargetArch));
+    } else if (IsVS2017OrNewer) {
+      llvm::sys::path::append(Path, "lib", llvmArchToWindowsSDKArch(TargetArch));
+    } else {
+      llvm::sys::path::append(Path, "lib", llvmArchToLegacyVCArch(TargetArch));
+    }
     break;
   }
   return Path.str();
 }
 
 #ifdef USE_WIN32
 static bool readFullStringValue(HKEY hkey, const char *valueName,
                                 std::string &value) {