Page MenuHomePhabricator

Improve Windows toolchain support for non-standard environments.
ClosedPublic

Authored by zturner on Oct 17 2014, 11:28 AM.

Details

Summary
Typically clang finds Visual Studio by the user explicitly setting
up a Visual Studio environment via vcvarsall.  But we still try to
behave intelligently and fallback to different methods of finding
Visual Studio when this is not done.  This patch improves various
fallback codepaths to make Visual Studio locating more robust.

Specifically, this patch:

* Adds support for searching environment variables for VS 12.0
* Correctly locates include folders for Windows SDK 8.x (this was
  previously broken, and would cause clang to error).  Windows
  SDK 8.x segregates its includes into \shared, \um, and \winrt.
* Prefers locating link.exe in the same location as cl.exe.  This
  is helpful in case another link.exe is in the path earlier than
  Visual Studio (e.g. GnuWin32)
* Minor cleanup in the registry reading code to make it more
  robust in the presence of long pathnames.

Diff Detail

Event Timeline

zturner updated this revision to Diff 15088.Oct 17 2014, 11:28 AM
zturner retitled this revision from to Improve Windows toolchain support for non-standard environments..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: rnk, aaron.ballman, hans.
zturner added a subscriber: Unknown Object (MLST).
zturner updated this revision to Diff 15093.Oct 17 2014, 12:05 PM

Fix an issue with a stack smash.

There is still an outstanding issue remaining after this patch, which is that link cannot locate libraries. I think we can address this in a followup patch by adding some library paths to the link line similar to how we already do for system include paths that we pass to cl.

hans added inline comments.Oct 17 2014, 12:58 PM
lib/Driver/Tools.cpp
7820

I'm not a big fan of our code for finding a msvc installation when vcvars.bat hasn't been run. I think that if the user wants to run clang-cl as a drop-in replacement for cl.exe, it should be dropped into the same environment.

But since we have it, we might as well make it work better. The TODO sounds reasonable, as long as it still checks PATH first, before going off and looking in other places.

Also, will cl.exe and link.exe etc. actually run if they're not on PATH? I recall having problems executing them without being on PATH because they failed to load some dll.

Style nit: FIXME without name is more common than TODO, and the |text| syntax isn't used.

7939

nit: it's

7942

smartPath?

zturner added inline comments.Oct 17 2014, 1:06 PM
lib/Driver/Tools.cpp
7820

Consider the case where you have a script which invokes clang many times with different sets of options each time. Sometimes m32, sometimes m64, for example. In this case it's cumbersome and difficult for the script to configure the environment beforehand, because there's no way to run a batch file "in process". If you try to run vcvars from a script it will just modify the environment of the shell process that is spawned to run the batch file.

I'm not a huge fan of it either though, and I actually think an ideal solution is to allow clang to accept a command line option that will just tell it the path to the root of the Visual Studio installation to use, and clang can just set up a consistent environment prior to invoking cl or link.

7942

Heh. Probably not the best name in the world. I'll fix that.

zturner updated this revision to Diff 15097.Oct 17 2014, 2:19 PM

Fix review issues.

zturner updated this revision to Diff 15099.Oct 17 2014, 3:15 PM
zturner added a reviewer: majnemer.

+majnemer

Also prefer finding the clang cl.exe fallback location by using the visual studio directory which is being used for includes instead of looking in the PATH first.

VC system headers are tied to a particular cl.exe and should not be mixed. Previously, we were looking for cl.exe in the PATH, and using a different algorithm to find the VC include directories. Now, the two methods use the same algorithm. Or at the very least, they try to. This also makes it possible for clang to fallback to cl even when cl is not in the PATH.

hans edited edge metadata.Oct 17 2014, 4:45 PM

Using getVisualStudioDir() for both headers and binaries seems like a big win!

lib/Driver/Tools.cpp
7819

Shouldn't it be searching for FallbackName rather than hard-coding cl.exe?

7820

I think VSDir can be passed to can_execute directly and the Twine gets constructed implicitly.

7825

It seems the code above is now doing what this fixme is suggesting :)

7845

I realize you didn't add them, but again I think we can drop the Twines. (hopefully)

7942

Maybe we should repurpose FindFallback to a function that finds vs executables and just pass in link.exe instead of doing this dance? We could then cut down on the comment too, because it would be obvious that the link.exe and cl.exe are from the same visual studio.

zturner updated this revision to Diff 15146.Oct 20 2014, 11:38 AM
zturner edited edge metadata.

Fix suggested review issues.

hans accepted this revision.Oct 20 2014, 12:55 PM
hans edited edge metadata.

lgtm

lib/Driver/Tools.cpp
7825

nit: I'd just use VSDir.str(). Same for FilePath below.

This revision is now accepted and ready to land.Oct 20 2014, 12:55 PM
zturner closed this revision.Oct 30 2014, 12:54 PM

This has been submitted

lib/Driver/Tools.cpp
7825

I did use VSDir.str(), did you mean you'd just use VSDir? Won't compile, there's no implicit conversion from SmallString to Twine.