Page MenuHomePhabricator

Update clang-cl driver for MSVC 2017
ClosedPublic

Authored by zturner on Mar 8 2017, 2:32 PM.

Details

Summary

This patch is originally based on D28365, but the clang ToolChain code has been significantly refactored since then, so rebasing that patch on top of tip was difficult / impossible. In any case, I got all of the changes integrated over here, with one notable difference: The header file containing all of the GUIDs and interface definitions is checked directly into the repository. This way it "just works", and we don't have to worry about if the required header file is present, and more importantly, copies of clang built with VS 2015 can run on a machine with VS 2017 installed and still properly detect the VS 2017 install. This would not be possible otherwise. I've added dberlin as a reviewer for the portion of this CL that involves checking in the header file.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Mar 8 2017, 2:32 PM
rnk accepted this revision.Mar 13 2017, 10:00 AM

lgtm

This revision is now accepted and ready to land.Mar 13 2017, 10:00 AM
dberlin added inline comments.Mar 13 2017, 11:17 AM
clang/lib/Driver/ToolChains/MSVCSetupApi.h
3 ↗(On Diff #91074)

There is no project root, you should paste the LICENSE.txt into the header directly.
Also, generally, i would segregate this into a third_party directory, as keeping code like this directly in the rest of the project makes it a lot messier

zturner added inline comments.Mar 13 2017, 11:42 AM
clang/lib/Driver/ToolChains/MSVCSetupApi.h
3 ↗(On Diff #91074)

Is a third_party directory directly in this folder ok, or should it be some kind of top level third_party directory?

dberlin added inline comments.Mar 14 2017, 1:03 AM
clang/lib/Driver/ToolChains/MSVCSetupApi.h
3 ↗(On Diff #91074)

Normal practice for our open source projects (ie not llvm) would be top-level, because otherwise they tend to be hard to notice.
Again, unsure what llvm wants the policy to be here.

dberlin added inline comments.Mar 14 2017, 1:19 AM
clang/lib/Driver/ToolChains/MSVCSetupApi.h
3 ↗(On Diff #91074)

FWIW, this is likely a remarkably bad idea :)
But i'll email the foundation folks and see what they think.

zturner added inline comments.Mar 14 2017, 8:47 AM
clang/lib/Driver/ToolChains/MSVCSetupApi.h
3 ↗(On Diff #91074)

Given that there's already precedent for doing de-segregating (is that the right word here?) small pieces of 3rd party code, is it ok for this to go in as-is (after pasting the LICENSE.txt into the header) and then after the foundation reaches a decision, we come back and address it all at once?

dberlin added inline comments.Mar 14 2017, 9:05 AM
clang/lib/Driver/ToolChains/MSVCSetupApi.h
3 ↗(On Diff #91074)

I'm not going to approve it, since i think making the problem worse is also a really bad idea, but if someone else does, and you commit it, i'm not going to take offense.

zturner added inline comments.Mar 14 2017, 9:16 AM
clang/lib/Driver/ToolChains/MSVCSetupApi.h
3 ↗(On Diff #91074)

How about this then? For now, I will assume that if _MSC_VER >= 1910 (i.e. you are building with VS 2017), then you also have the Microsoft header, and I will include the Microsoft header and this codepath will work. Otherwise, you get the old codepath (meaning that if you build clang with MSVC <2017, then you also cannot use the built clang to achieve compatibility with MSVC 2017 even if you copy it to a machine that has MSVC 2017 installed.)

Once you hear back and reach a decision, we can get this header checked in and enable clang-cl bootstrapped with MSVC 2015 to be compatible with MSVC >= 2017.

hamzasood edited edge metadata.Mar 14 2017, 9:34 AM

I don't think any Visual Studio versions ship with the header, it needs to be installed via Nuget

I don't think any Visual Studio versions ship with the header, it needs to be installed via Nuget

Yea I realized that when I removed the header locally and tried to find the one to include. That's a bummer.

This revision was automatically updated to reflect the committed changes.