This is an archive of the discontinued LLVM Phabricator instance.

Resubmit "Improve Windows toolchain support for non-standard environments."
ClosedPublic

Authored by zturner on Oct 21 2014, 12:39 PM.

Details

Summary
This resubmits change r220226.  That change broke the chromium
build bots because chromium it ships an hermetic MSVC toolchain
that it expects clang to fallback to by finding it on the path.

This patch fixes the issue by bumping up the prioritization of PATH
when looking for MSVC binaries.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 15203.Oct 21 2014, 12:39 PM
zturner retitled this revision from to Resubmit "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: hans, rnk, aaron.ballman.
zturner added a subscriber: Unknown Object (MLST).
rnk added inline comments.Oct 21 2014, 1:06 PM
lib/Driver/WindowsToolChain.cpp
220 ↗(On Diff #15203)

I don't think we need to sink this logic into the toolchain, we can probably keep it local to the fallback code, just changing the order of the check for the VS install dir to come after we've done the PATH search.

226 ↗(On Diff #15203)

This doesn't seem right, we need the path to VS/VC/bin or VS/VC/bin/amd64. I don't think we need to have this first case.

hans edited edge metadata.Oct 21 2014, 1:31 PM

It seems the patch is based on top off the patch which got reverted. It would be better if it was relative to the current trunk.

I tried this on the Chromium build and it worked.

hans added inline comments.Oct 21 2014, 1:33 PM
lib/Driver/WindowsToolChain.cpp
218 ↗(On Diff #15203)

I wonder if we should check PATH first.

If the user runs "cl.exe", she expects the one on PATH to be run. If she uses clang-cl.exe instead, and we fall back, that's the cl.exe we should use.

zturner updated this revision to Diff 15206.Oct 21 2014, 2:10 PM
zturner edited edge metadata.

Fix review issues. Also check the target triple to determine which toolchain to use (x86, x64, or arm).

In D5892#5, @hans wrote:

It seems the patch is based on top off the patch which got reverted. It would be better if it was relative to the current trunk.

I tried this on the Chromium build and it worked.

That's strange, it should be based off of current trunk.

lib/Driver/WindowsToolChain.cpp
226 ↗(On Diff #15203)

I kind of like checking VCINSTALLDIR first, because the only way that's set is if someone runs vcvars, which *also* sets PATH. The only way checking VCINSTALLDIR first causes a problem is if someone runs vcvars, then further modifies the PATH to stick something else first. If someone does that I feel like we could say "sorry, your'e on your own now". PATH is kind of the garbage disposal of the environment, and everyone just dumps what they need into PATH. So it's more likely to get polluted than VCINSTALLDIR, which is much more specific.

zturner updated this revision to Diff 15207.Oct 21 2014, 2:14 PM

Patch should be based off of current ToT now, I originally ran git revert <the original revert>, and then submitted this on top of that. I squashed the two now, so this should be based on top of ToT.

zturner updated this revision to Diff 15258.Oct 22 2014, 10:43 AM

Rebase against ToT again, someone renamed this class @.@

hans added inline comments.Oct 22 2014, 11:15 AM
lib/Driver/MSVCToolChain.cpp
218 ↗(On Diff #15258)

The Windows toolchain was renamed to MSVCToolChain in r220362.

225 ↗(On Diff #15258)

Should this use llvm::sys::Process::GetEnv like below?

228 ↗(On Diff #15258)

Should this append VC\bin ?

When I applied this patch, clang no longer finds link.exe in my VS2012 environment.

VCINSTALLDIR is set to C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\ and link.exe is at C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\bin\link.exe

If I unset VSINSTALLDIR it seems to work due to the PATH walking below.

zturner updated this revision to Diff 15269.Oct 22 2014, 12:17 PM

Hopefully this fixes all the issues. Now, if VCINSTALLDIR is set, it only appends "bin" (since the VC part is already on the path).

hans accepted this revision.Oct 22 2014, 12:40 PM
hans edited edge metadata.

Seems to work for me :)

This revision is now accepted and ready to land.Oct 22 2014, 12:40 PM
zturner closed this revision.Oct 22 2014, 1:51 PM
zturner updated this revision to Diff 15275.

Closed by commit rL220424 (authored by @zturner).