Page MenuHomePhabricator

Fix automatic detection of ARM MSVC toolset in clang.exe
Needs ReviewPublic

Authored by DaveBartolomeo on Jul 15 2016, 2:46 PM.

Details

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.

Diff Detail

Event Timeline

DaveBartolomeo retitled this revision from to Fix automatic detection of ARM MSVC toolset in clang.exe.
DaveBartolomeo updated this object.
compnerd accepted this revision.Jul 15 2016, 5:17 PM
compnerd added a reviewer: compnerd.
compnerd added a subscriber: 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?

This revision is now accepted and ready to land.Jul 15 2016, 5:17 PM

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.

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 edited edge metadata.Jul 17 2016, 8:55 PM

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 edited edge metadata.Jul 18 2016, 11:10 AM

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 requested changes to this revision.Jul 18 2016, 5:53 PM
compnerd edited edge metadata.

@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.

This revision now requires changes to proceed.Jul 18 2016, 5:53 PM

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 edited edge metadata.

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 added inline comments.Aug 7 2016, 10:04 PM
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)) {