Fix automatic detection of ARM MSVC toolset in clang.exe

[DaveBartolomeo]

Summary

Clang was failing to find the ARM version of the MSVC link.exe, even though it could correctly find link.exe for x86 and x64. Clang looks in the %VCINSTALLDIR%\VC\bin directory for link.exe when targeting x86 (finding the x86-hosted, x86-targeting tools), the VC\bin\amd64 directory when targeting x64 (finding the x64-hosted, x64-targeting tools), and the non-existent VC\bin\arm directory when targeting ARM (which is where the ARM-hosted tools would be if they existed). I just switched it to always look for the x86-hosted tools for all targets, in VC\bin\x86, VC\bin\x86_amd64, or VC\bin\x86_arm.

[compnerd]

I imagine that at this point, most usage is still based around the x86 toolchain rather than x64 (I didnt even notice the x64 tools until recently). That is, any reason that we shouldnt be using x64 for x64, x64_x86 for x86, and x64_arm for ARM?

[DaveBartolomeo]

In D22426#486100, @compnerd wrote:

I imagine that at this point, most usage is still based around the x86 toolchain rather than x64 (I didnt even notice the x64 tools until recently). That is, any reason that we shouldnt be using x64 for x64, x64_x86 for x86, and x64_arm for ARM?

Most usage is indeed x86-hosted. While the x64-hosted x64-targeting compiler has been available from the beginning of MSVC's x64 support, the x64-hosted x86-targeting toolset is relatively recent (VS 2102 or VS 2013, I think). In general, the x86-hosted compilers are marginally faster than the equivalent x64-hosted compiler, and since x86-hosted has always been the default, most developers don't switch to the x64-hosted toolset unless they run into code that won't compile or link without running out of memory space on the x86-hosted toolset. Also, the x64-hosted toolset won't run on machines running a 32-bit OS, which are more rare these days, but still exist. The existing Clang behavior would have failed on a 32-bit OS when targeting x64, though, so if we've gotten away with it this long, we're probably OK.

One reasonable solution would be to choose the toolset that is hosted on the same architecture as the host of clang.exe (e.g. x64-hosted Clang looks for x64-hosted MSVC). If this sounds good, I can make that change in a follow-up commit.

[compnerd]

I was worried about the OOM situation with the 32-bit toolchain. As long as there is a way to get to the 64-bit version, I don't think that it matters too much that we default to x86. It sounds like even then, its not been a concern?

[majnemer]

In D22426#486605, @compnerd wrote:

I was worried about the OOM situation with the 32-bit toolchain. As long as there is a way to get to the 64-bit version, I don't think that it matters too much that we default to x86. It sounds like even then, its not been a concern?

We had big OOM problems with the x86 hosted tool chain when targeting x86, I think we'd much rather use the x64 hosted toolchain whenever possible...

[rnk]

In D22426#486129, @DaveBartolomeo wrote:

One reasonable solution would be to choose the toolset that is hosted on the same architecture as the host of clang.exe (e.g. x64-hosted Clang looks for x64-hosted MSVC). If this sounds good, I can make that change in a follow-up commit.

I like that, it seems more principled. In the meantime, I think we should go with VC/amd64_{x86|arm}/bin instead. Maybe I'm biased, but I've seem more linker failures due to address space exhaustion than 32-bit Windows kernels.

[compnerd]

@rnk, okay, that seems reasonable enough. Although, we should check to ensure that the x64 toolchain is available and make a decision on that.

BTW, seems that I had missed the proposed solution. That is exactly what I had in mind, and in an offline conversation with @majnemer it seemed acceptable to him as well.

[DaveBartolomeo]

Just to make sure I'm clear on the consensus, the new plan is:

If clang.exe is x64-hosted and an x64-hosted MSVC toolchain is available, use the x64-hosted MSVC toolchain. Otherwise, use the x86-hosted MSVC toolchain.

Right?

[DaveBartolomeo]

Updated the selection algorithm based on review feedback. Now, if clang.exe itself is x64-hosted, we'll look for the x64-hosted MSVC toolset if it exists. If clang.exe is not x64-hosted, or if the x64-hosted MSVC toolset is not found, we fall back to x86-hosted MSVC toolset.

[compnerd]

lib/Driver/MSVCToolChain.cpp

478

As per the consensus, this should be:

if (llvm::sys::getProcessTriple().getArch() == llvm::Triple::x86_64 && llvm::sys::fs::exists(X64BinDir)) {

lib/Driver/MSVCToolChain.cpp

if (VcInstallDir.hasValue()) {
 
     if (!llvm::sys::fs::can_execute(ClPath.c_str()))
       return false;
   }
 
   if (BinDir.empty())
     return false;
 
+  SmallString<128> X64BinDir = BinDir;
   switch (getArch()) {
   case llvm::Triple::x86:
+    llvm::sys::path::append(X64BinDir, "amd64_x86");
     break;
   case llvm::Triple::x86_64:
-    llvm::sys::path::append(BinDir, "amd64");
+    llvm::sys::path::append(BinDir, "x86_amd64");
+    llvm::sys::path::append(X64BinDir, "amd64");
     break;
   case llvm::Triple::arm:
-    llvm::sys::path::append(BinDir, "arm");
+    llvm::sys::path::append(BinDir, "x86_arm");
+    llvm::sys::path::append(X64BinDir, "amd64_arm");
     break;
   default:
     // Whatever this is, Visual Studio doesn't have a toolchain for it.
     return false;
   }
+
+#if defined(_M_X64)
+  // If running x64-hosted Clang, choose the x64-hosted version of the MSVC
+  // toolset, if it exists.
+  if (llvm::sys::fs::exists(X64BinDir)) {
+    path = X64BinDir.str();
+    return true;
+  }
+#endif
+
   path = BinDir.str();
   return true;
 }
 
 VersionTuple MSVCToolChain::getMSVCVersionFromExe() const {
   VersionTuple Version;
 #ifdef USE_WIN32
   std::string BinPath;