This is an archive of the discontinued LLVM Phabricator instance.

Make a good guess about where MSVC and Windows SDK libraries are for linking.
ClosedPublic

Authored by zturner on Oct 20 2014, 2:24 PM.

Details

Summary
When a user doesn't run vcvarsall, clang doesn't know where system
libraries are located.  This patch causes clang to still use the
configured environment variables when present, but when they are
not present, to make a (very good) guess.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 15153.Oct 20 2014, 2:24 PM
zturner retitled this revision from to Make a good guess about where MSVC and Windows SDK libraries are for linking..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: aaron.ballman, hans, majnemer, rnk.
zturner added a subscriber: Unknown Object (MLST).

Note that after applying both this patch and my previous patch, I can now compile and link an executable without first running vcvarsall, and the environment that it sets up is identical to that which would have been created by first running vcvarsall.

hans accepted this revision.Oct 20 2014, 2:57 PM
hans edited edge metadata.

lgtm

lib/Driver/ToolChains.h
749 ↗(On Diff #15153)

I'd find it more natural to put the path parameter last, since it's an output.

lib/Driver/WindowsToolChain.cpp
215 ↗(On Diff #15153)

I think \brief only works for comments starting with /, and one-line / comments are treated as brief automatically. And if we want to use doxygen comments for these functions, they should probably go in the header file.

(Phabricator is formatting my slashes. Where it says /, I mean three slashes)

226 ↗(On Diff #15153)

I have no idea if this is correct, but I guess it looks reasonable :)

This revision is now accepted and ready to land.Oct 20 2014, 2:57 PM
zturner added inline comments.Oct 20 2014, 3:01 PM
lib/Driver/WindowsToolChain.cpp
226 ↗(On Diff #15153)

I found someone with an installation of the 7.0 SDK, and this is what he said his looked like. So it seems correct.

zturner updated this revision to Diff 15217.Oct 21 2014, 3:20 PM
zturner edited edge metadata.
  • Rebased on top of the fixed include file / executable search patch patch.
  • updated patch to correctly use the target triple instead of looking directly at the m32 / m64 command line options
  • Correctly handle ARM lib paths.
  • Fix incorrect lib path issue pointed out by Aaron Ballman related to Windows SDK 7.x amd64 folder.
rnk added inline comments.Oct 21 2014, 4:09 PM
lib/Driver/Tools.cpp
7853 ↗(On Diff #15217)

Close the parenthetical before the comma

7856 ↗(On Diff #15217)

The style guide says to use upper case local variables. Some code is inconsistent, but this code uses mostly upper case locals, so let's lean that way here.

7857 ↗(On Diff #15217)

Hm, that seems to be a common pattern elsewhere, so OK.

Our style guide currently says to add const before auto if you know it's const.

7861 ↗(On Diff #15217)

Can we shorten this up a bunch with a helper that takes the arch and returns a string for where to look?

I think this would be cleaner if you do a single call to append, like so:

StringRef SubDir = getVCArchSubDir(WTC.getArch());
llvm::sys::path::append(VSDir, "VC", "lib", SubDir);

Appending the empty string is a no-op, so the basic x86 case can return "", but the ppc case will have to return false or something else weird.

I expect this helper will also apply to VC/bin in the other patch.

It's also nice to avoid hardcoded backslashes so that we can eventually support cross-compilation.

7878 ↗(On Diff #15217)

WindowsSdkLibPath

lib/Driver/WindowsToolChain.cpp
230 ↗(On Diff #15217)

A similar helper like getWindowsSDKArchSubDir(WTC.getArch(), sdkMajor) might help here.

251 ↗(On Diff #15217)

You can use a range for loop over the local array.

zturner updated this revision to Diff 15253.Oct 22 2014, 10:21 AM

Updated with previous comments. After offline discussion, replicating the switch statements instead of making a helper function was agreed upon as the best approach.

zturner updated this revision to Diff 15259.Oct 22 2014, 10:44 AM

Rebase against ToT, someone renamed toolchains::Windows to toolchains::MSVCToolChain

zturner updated this revision to Diff 15270.Oct 22 2014, 12:18 PM

Hopefully this fixes the remainder of the issues.

hans added a comment.Oct 22 2014, 1:34 PM

lgtm2

lib/Driver/MSVCToolChain.cpp
237 ↗(On Diff #15270)

Clearing this seems redundant?

243 ↗(On Diff #15270)

nit: s/an/a/, s/names/name/ ?

247 ↗(On Diff #15270)

is winv6.3 newer than win8? (i have no idea, just noticed the number is lower)

winv6.3 ins windows 8.1

zturner closed this revision.Oct 22 2014, 1:51 PM
zturner updated this revision to Diff 15276.

Closed by commit rL220425 (authored by @zturner).