[PATCH] D42762: Rewrite the VS Integration Scripts

Thu Feb 1 08:48:43 PST 2018

zturner added inline comments.

================
Comment at: llvm/tools/msbuild/Clang.Cpp.Common.props:41
+    
+    <!-- The registry key may not be set if it's an old installer, try the newest version that exists -->
+    <LLVMVersion Condition="'$(LLVMVersion)' == '' and Exists('$(LLVMInstallDir)\lib\clang\7.0.0')">7.0.0</LLVMVersion>
----------------
hans wrote:
> As I mentioned before, separating the toolset config from the actual toolchain installation makes me a little nervous.
> 
> But if we're doing it, the version checks below should probably include the .1 versions too, i.e. at least 5.0.1 and 6.0.1.
Unless we're going to release the full thing including the compiler, linker, etc through the marketplace I don't see an alternative.  In any case, I actually think this it's preferable this way.  There's nothing really about the two that benefits from having them coupled together, as far as I can see.   I'm willing to be convinced though, if we can figure out how to to do it so that we can still ship it on the marketplace.

================
Comment at: llvm/tools/msbuild/LLVM.props:8
+    <!-- Friendly names added to the PlatformToolset in the property pages. -->
+    <_PlatformToolsetFriendlyNameFor_llvm Condition="'$(_PlatformToolsetFriendlyNameFor_llvm)' == ''">Clang for Windows</_PlatformToolsetFriendlyNameFor_llvm>
+  </PropertyGroup>
----------------
hans wrote:
> Hmm, we previously intentionally called the toolset "LLVM" with the thinking that it would eventually include lld and designated the complete llvm toolchain, not just Clang. And is the "for Windows" part necessary?
Do you think there's any value in having a toolset that does Clang+Link and a second one that does Clang+LLD?  Or do you think we should stick with only a single one?  I can change the name to LLVM though.

================
Comment at: llvm/tools/msbuild/Toolset.targets:38
+    
+    <!-- Warn if Fiber Safe Optimizations are enabled, and then ignore them. -->
+    <Warning Condition="'%(ClCompile.EnableFiberSafeOptimizations)' == 'true'"
----------------
hans wrote:
> This seems to duplicate a lot of logic from clang-cl. It's nice to provide a good UI for the user, but maintaining this seems a lot of work. Are you not concerned that this will rot?
I don't think it will.  Maybe I'm being overly optimistic here, but the only case we would ever need to maintain this again is if we started supporting these options.  Fiber Safe Optimizations, for example, I'm pretty sure we will never support.  If MSVC ever removes the option, for example, we can do nothing and continue to work.

We could also just silently ignore them and just pass the option through to clang-cl, but these are pretty unusual options with pretty specialized use cases, so I feel like if I had gone out of my way to enable such a strange option I would want to know if the compiler was not going to respect it.

================
Comment at: llvm/tools/msbuild/Toolset.targets:46
+             File="@(ClCompile)(0,0)"
+             Text="clang-cl does not support MSVC Link Time Optimization.  Disable this option in compatibility settings to silence this warning."/>
+
----------------
hans wrote:
> But maybe we want clang-cl to map this to -flto one day. Now we need to update two places. And with the toolset/toolchain install split, the two places may be installed separately :-/
That's even better then.  All we have to do is change this xml, push a new build to the market place, and the VS UI will update their extension for them.

Note that we could do the mapping at the MSBuild level, in this file down below where we have an `ItemGroup`.  Just add a line that says `<AdditionalOptions Condition="%(ClCompile.WholeProgramOptimization)' == 'true'>-flto=thin %(AdditionalOptions)</AdditionalOptions>`

and we can do this without touching clang.

================
Comment at: llvm/tools/msbuild/Toolset.targets:83
+    
+    <!-- Warn if XML Documentation is generated, and then ignore it. -->
+    <Warning Condition="'%(ClCompile.GenerateXMLDocumentationFiles)' == 'true'"
----------------
hans wrote:
> Keeping up with all these flags seems like a huge amount of work. Why not just let clang-cl ignore it?
See the large comment at the top of the file.  For some options, we could probably get by with this.  Maybe even this one, I debated on this particular one.

My bar was "If the option fundamentally changes assumptions about the way code could be compiled, we should generate an error.  If it changes the behavior of the language in a way we don't support,  changes the way we generate code in a meaningful way, or causes specialized output files to be written, warn, and if it's an option we ignore then drop it"

The last category there we could probably just pass through in some cases, but in that comment I also mentioned a case where setting an option that clang-cl ignores impacts MSBuild's ability to figure out dependencies and ends up causing a full rebuild every time even when nothing changed.

We can scour the entire cl build tasks and try to discover if any other ones have unintended consequences, but I think it's easier to just turn them off at the MSBuild level.  And as a side benefit, the user gets shorter command lines, which is always nice.

As for maintenance, this all looks like zero-maintenance code to me.  Did you have an example in mind of where we'd need to update this?  Whether it be a new VS version, or VS dropping support for one of these options or deprecating them, I don't think we'd have to do anything.

================
Comment at: llvm/tools/msbuild/Toolset.targets:111
+           File="@(ClCompile)(0,0)"
+           Text="clang-cl does not support OpenMP (/openmp).  This file cannot be compiled."/>
+    
----------------
hans wrote:
> Maybe it will one day. The OpenMP folks claim their runtime works on Windows and we've shipped it for some releases. It seems better to keep this logic tied to the toolchain?
I can put some conditions that only cause it to error if clang version is less than or equal to 7, but that seems higher maintenance because we'll probably have many new releases before that ever happens.  Even if we do tie it to the toolchain, the user is still going to have an error in their code if they have `/openmp` on, they'll just have to spend more time figuring out that the underlying cause is we don't currently support this option.  I'd rather err on the side of improving the user experience today and worrying about what we might do in the future when it happens.

Even if we do have to push a new version of the extension every so often, that's less trouble for the user than downloading a new installer because they get auto-updated through the VS Extension Manager.

================
Comment at: llvm/tools/msbuild/Toolset.targets:128
+             -->
+        <WholeProgramOptimization/>
+        <EnableFiberSafeOptimizations/>
----------------
hans wrote:
> More stuff to keep in sync with MSVC's options and clang-cl :-/
Mentioned before, but I dont' think we have to do anything to keep this in sync with MSVC.  All of these lines should be able to remain forever, regardless of what MSVC does.

================
Comment at: llvm/tools/msbuild/Toolset.targets:166
+       instead.  The code below is mostly a straight fork from the common Microsoft build file,
+       with only cl.xml replaced with clang-cl.xml. -->
+  <PropertyGroup>
----------------
hans wrote:
> I guess we'll need to update the fork with each VS release (but still stay compatible with old versions?)
I don't think it would be necessary to update every release unless they fundamentally change the way the UI looks.  The logic didn't change between 2015 and 2017 for example.  Unless they redesign the entire Project Settings dialog, I think we should be future proof.

================
Comment at: llvm/tools/msbuild/clang-cl.xml:343
+  <BoolProperty ReverseSwitch="openmp-" Name="OpenMPSupport" DisplayName="Open MP Support" Description="This switch is incompatible with clang-cl and will generate an error if set to true.  (/openmp)" Category="Incompatible" Switch="openmp" F1Keyword="VC.Project.VCCLCompilerTool.OpenMP">
+  </BoolProperty>
+  <BoolProperty Name="EnableModules" DisplayName="Enable C++ Modules (experimental)" Description="This switch is incompatible with clang-cl and will generate an error if set to true.  (/experimental:module)" Category="Incompatible" Switch="experimental:module" F1Keyword="VC.Project.VCCLCompilerTool.ModulesSupport">
----------------
hans wrote:
> This seems like a huge amount of stuff to keep in sync with MSVC and clang-cl's flags.
This stuff almost never changes though.  Most of these options have been here since at least VS 2010, which is about as far back as I can remember.  The only one that wasn't here in 2015 is `/diagonstics`

================
Comment at: llvm/tools/msbuild/install.bat:10
+REM Older versions of VS would look for these files in the Program Files\MSBuild directory
+REM but with VS2017 it seems to look for these directly in the Visual Studio instance.
+REM This means we'll need to do a little extra work to properly detect all the various
----------------
hans wrote:
> Don't we want to support at least 2015 too?
Mentioned in the other review, but the install.bat file shouldn't really be used anymore except for during development.  The VSIX supports both 2015 and 2017 (I tested it in both and confirmed it works)

https://reviews.llvm.org/D42762