[PATCH] D42762: Rewrite the VS Integration Scripts

Mon Feb 5 12:38:56 PST 2018

olgaark added inline comments.

================
Comment at: llvm/tools/msbuild/Clang.Cpp.Common.props:49
+    <CLToolExe>clang-cl.exe</CLToolExe>
+    <CLToolPath>$(LLVMInstallDir)bin</CLToolPath>
+
----------------
hans wrote:
> It's embarrassing that we didn't figure this out before :-)
> 
> With this, we can remove
> 
> ```
>  if (MSVC)
>     list(APPEND CLANG_LINKS_TO_CREATE ../msbuild-bin/cl)
>   endif()
> ```
> 
> from Clang's tools/driver/CMakeLists.txt
Yes, you can redefine CLToolExe/Path if you want to use 'cl' task, but have you looked at using 'ClangCompile' task instead (see Microsoft.Cpp.Clang.props & targets, as well as clang c2 toolset)?  Would it work better for you? If not, we'll interested to know why.

================
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:
> zturner wrote:
> > 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.
> The best would be to only have one, but where the user could select between the two linkers, I think.
I believe the debugging is different for LLD produced binaries, so if this is the case, I'd recommend creating separate msbuild toolsets for Clang+Link and Clang+LLD.

VS has extensibility to support different debugging and it is expected that when toolset changes the set of available debuggers can change, but not when some arbitrary property is changed.

================
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:
> zturner wrote:
> > 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.
> I feel pretty strongly that we should handle this clang-cl side. If a flag is not supported, either we should ignore it, or if it's something the user would want to know about us not supporting, we should warn. That's what clang-cl tries to do currently, and if there are flags we don't get right, we should fix it.
> 
> And we do move flags from the unsupported to supported category now and then, so the "only case we would ever need to maintain this again is if we started supporting these options" scenario is real.
There is a way currently to extend a rule (not override it) where you can add/remove properties, but I am not sure it will make it better from maintenance perspective for clang-cl rule. You might want to use it for ConfigGeneral page though.

https://github.com/Microsoft/VSProjectSystem/commit/3e2e9c7ea2f3155de9932ab53d9ec37b6bdafa17

Also, if you decide to use ClangCompile task, clang.xml it the one which is matching it.

================
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>
----------------
zturner wrote:
> 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.
If you don't need to remove rules added by default and want just replace some of them (like cl one with clang-cl) you can allow default rules to be added and then include yours, If several rules in the list have the same name, the last one will be used. 

So if you name your rule as "CL" (instead of "clang-cl") and make sure you include it to PropertyPageSchema items after importing MS targets which add 'real' cl rule, your rule will be used instead.

https://reviews.llvm.org/D42762