Page MenuHomePhabricator

[Driver] Recognize DevDiv internal builds of MSVC, with a different directory structure
ClosedPublic

Authored by STL_MSFT on Aug 17 2017, 8:42 PM.

Details

Summary

This is a reasonably non-intrusive change, which I've verified works for both x86 and x64 DevDiv-internal builds.

The idea is to change bool IsVS2017OrNewer into a 3-state ToolsetLayout VSLayout. Either a build is DevDiv-internal, released VS 2017 or newer, or released VS 2015 or older. When looking at the directory structure, if instead of "VC" we see "x86ret", "x86chk", "amd64ret", or "amd64chk", we recognize this as a DevDiv-internal build.

After we get past the directory structure validation, we use this knowledge to regenerate paths appropriately. llvmArchToDevDivInternalArch() knows how we use "i386" subdirectories, and MSVCToolChain::getSubDirectoryPath() uses that. It also knows that DevDiv-internal builds have an "inc" subdirectory instead of "include".

This may still not be the "right" fix in any sense, but I believe that it's non-intrusive in the sense that if the special directory names aren't found, no codepaths are affected. (ToolsetLayout::OlderVS and ToolsetLayout::VS2017OrNewer correspond to IsVS2017OrNewer being false or true, respectively.) I searched for all references to IsVS2017OrNewer, which are places where Clang cares about VS's directory structure, and the only one that isn't being patched is some logic to deal with cross-compilation. I'm fine with that not working for DevDiv-internal builds for the moment (we typically test the native compilers), so I added a comment.

Is there still time to get a patch like this into 5.0.0, or is it too late?

Diff Detail

Event Timeline

STL_MSFT created this revision.Aug 17 2017, 8:42 PM
hintonda edited edge metadata.Aug 18 2017, 7:36 AM

Looks good, but I'll defer to the owners.

Minor nit...

lib/Driver/ToolChains/MSVC.cpp
148

Perhaps remove the else and grab the subdir name only once before doing comparisons.

thakis edited edge metadata.Aug 18 2017, 7:41 AM

This approach looks good to me.

Is there any chance we could have a test for this? See test/Driver for examples; use python bin\llvm-lit.py ../llvm/tools/clang/test/Driver/my_test.cpp to run a single test, ninja check-clang to run all of them. You need some unix binaries to run tests -- if you don't have those flying around, you can download gnuwin-7.zip from http://commondatastorage.googleapis.com/chromium-browser-clang/index.html?path=tools/ and add them to your %PATH%.

STL_MSFT updated this revision to Diff 111776.Aug 18 2017, 5:43 PM
STL_MSFT edited the summary of this revision. (Show Details)

This addresses Don Hinton's feedback. It also unifies IsVS2017OrNewer and IsDevDivInternal into a 3-state enum class ToolsetLayout.

STL_MSFT marked an inline comment as done.Aug 18 2017, 5:53 PM

This approach looks good to me.

Thanks!

Is there any chance we could have a test for this?

Unfortunately, testing this involves having a VS toolset with the DevDiv-internal directory structure. Preparing that is a laborious manual process, and requires installation of the Windows SDK (I sent repro instructions to cfe-dev). Even given a real installation of VS, I don't think I could write automated test code to extract a copy with the necessary directory structure (doing so would almost involve replicating the codepaths being touched here). But now that I know that this code exists and that Clang occasionally changes it, especially in response to new VS directory structures, I'll watch for new betas/RCs and verify them against our internal builds.

thakis accepted this revision.Aug 18 2017, 6:49 PM

Many driver tests check in a basic representative directory structure (e.g. test/Driver/Inputs/basic_freebsd_tree/ and its many siblings).

But if you're happy with others breaking this code, I suppose having no tests is ok; I imagine most people won't use this code :-)

This revision is now accepted and ready to land.Aug 18 2017, 6:49 PM

Many driver tests check in a basic representative directory structure (e.g. test/Driver/Inputs/basic_freebsd_tree/ and its many siblings).

But if you're happy with others breaking this code, I suppose having no tests is ok; I imagine most people won't use this code :-)

I can't check in copies of cl.exe and link.exe (the former's version is inspected, so it can't just be a dummy file). I'm happy without upstream tests for this logic. In the long run, I hope that we can eliminate this internal directory structure and make it identical to the released directory structure.

STL_MSFT closed this revision.Aug 21 2017, 3:22 PM