This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Updated for Visual Studio 2017
ClosedPublic

Authored by hamzasood on Jan 5 2017, 9:57 AM.

Details

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.

Diff Detail

Event Timeline

hamzasood updated this revision to Diff 83263.Jan 5 2017, 9:57 AM
hamzasood retitled this revision from to [Driver] Updated for Visual Studio 2017.
hamzasood updated this object.
hamzasood added reviewers: rnk, ruiu, hans.
hamzasood added a subscriber: cfe-commits.
hamzasood updated this revision to Diff 83303.Jan 5 2017, 1:56 PM
  • Fixed an error in retrieving a toolchain path from the registry.
  • Updated the base revision.

(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 updated this revision to Diff 83318.Jan 5 2017, 3:39 PM
  • Re-implemented the PATH searching behaviour (thanks @amccarth for pointing that out)
  • Updated the base revision.
hamzasood updated this revision to Diff 83325.Jan 5 2017, 4:43 PM

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

rnk edited edge metadata.Jan 6 2017, 1:29 PM

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
76

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

186

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?

187

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;
352

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.

370

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

hamzasood updated this revision to Diff 83473.Jan 6 2017, 5:40 PM
hamzasood edited edge metadata.
  • 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 updated this revision to Diff 83555.Jan 8 2017, 5:33 AM
  • 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 added a subscriber: awson.Jan 8 2017, 6:23 AM

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

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 added a comment.Jan 9 2017, 4:52 PM
  • 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.

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 added a comment.Jan 9 2017, 5:18 PM
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. :)

In D28365#640859, @rnk 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 added a comment.Jan 10 2017, 10:44 AM

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 updated this revision to Diff 83941.Jan 11 2017, 3:04 AM
  • 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 updated this revision to Diff 83943.Jan 11 2017, 3:15 AM

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

rnk added inline comments.Jan 13 2017, 11:56 AM
lib/Driver/MSVCToolChain.cpp
34

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

163

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

266

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

hamzasood updated this revision to Diff 84446.Jan 14 2017, 4:41 AM

Broke up findVCToolChainPath into a few smaller functions.

hamzasood added inline comments.Jan 14 2017, 4:42 AM
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 marked 5 inline comments as done.Jan 23 2017, 8:57 AM

Ping

rnk accepted this revision.Jan 30 2017, 4:16 PM

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.

This revision is now accepted and ready to land.Jan 30 2017, 4:16 PM
hamzasood updated this revision to Diff 86502.Jan 31 2017, 3:23 PM

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 added a comment.Feb 2 2017, 9:59 AM

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

In D28365#664892, @rnk wrote:

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

I think so, yeah.

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Feb 2 2017, 1:21 PM

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 updated this revision to Diff 86900.Feb 2 2017, 3:22 PM
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 added a comment.Feb 2 2017, 4:27 PM

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.

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?