Symptom: when /Zi is selected, VS always rebuilds all source files, even if just 1 (possibly even none) have changed.<br><br>Fix: Change /Zi to /Z7 in the UI<br><br>More details here: <a href="https://bugs.llvm.org/show_bug.cgi?id=36140">https://bugs.llvm.org/show_bug.cgi?id=36140</a><br><div class="gmail_quote"><div dir="ltr">On Fri, Feb 2, 2018 at 8:12 AM Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Feb 2, 2018 at 4:40 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
> On Fri, Feb 2, 2018 at 6:23 AM Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br>
>><br>
>> (Your reply didn't go to Phabricator, so re-adding folks subscribed<br>
>> there.)<br>
>><br>
>> On Thu, Feb 1, 2018 at 9:08 PM, Zachary Turner via llvm-commits<br>
>> <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> > I'm kind of imagining this world where we have one VS Integration that<br>
>> > works<br>
>> > no matter what version of LLVM you have. The nice thing about this is<br>
>> > that<br>
>> > it allows it to work with hermetic toolchains, older versions of LLVM<br>
>> > that<br>
>> > may already be installed on a user's machine, local dev builds of LLVM,<br>
>> > etc.<br>
>><br>
>> I'm on board with this. It seems useful especially for the case where<br>
>> the developer may have multiple LLVM toolchains installed and want to<br>
>> point at a specific one. It would be nice if we could still trigger<br>
>> the installation of the toolset when installing the LLVM toolchain<br>
>> though.<br>
>><br>
>> But for the integration to work regardless of LLVM version, I don't<br>
>> think the integration can bake in assumptions about what flags<br>
>> clang-cl supports and re-map them etc. The set of flags supported by<br>
>> clang-cl and how they're handled changes frequently.<br>
>><br>
>> I like the idea of a VS integration that's LLVM-version independent --<br>
>> the current one is almost that except for the baked in version number<br>
>> -- but for it to work, I think it has to be really simple, basically<br>
>> just pointing MSVC at clang-cl.exe and nothing more.<br>
><br>
> I’ve already mentioned at least one case whereas this is impossible (/Zi vs<br>
> /Z7), and given that there are thousands of lines of msbuild logic that are<br>
> running and processing these options before they make it to clang-cl, I’m<br>
> certain there are more that we don’t yet know about.<br>
<br>
I'd like to understand the /Zi vs /Z7 thing better. Can you ELI5 the problem?<br>
<br>
<br>
><br>
> Simple is nice, I don’t disagree with that, but not at the expense of user<br>
> experience. i still don’t think there’s any maintenance issues here though.<br>
> I think the current version here could probably sit for 5+ years and never<br>
> need to be touched, continuing to work both with future VS versions and<br>
> future clang-cl versions unmodified.<br>
><br>
><br>
>><br>
>><br>
>> > The only maintenance burden I can think of is one where we remove or add<br>
>> > flags in clang-cl, which doesn't happen very often, if ever.<br>
>><br>
>> It puzzles me that you think we rarely or ever change the flags<br>
>> clang-cl supports or how they're handled. In my experience, the flags<br>
>> change every release.<br>
>><br>
>> > Any flag that<br>
>> > is added to MSVC doesn't require any action from us.<br>
>><br>
>> Depends on the flag, no?<br>
><br>
> Not really. Any flag that msvc adds, assuming we don’t update this file,<br>
> gets passed through to clang-cl which is what you’re proposing i do with all<br>
> options anyway.<br>
><br>
><br>
>><br>
>><br>
>> > I plan to expose a UI for optimizations and warning, so I could see a<br>
>> > maintenace burden when we add new -W or -f flags that are not exposed to<br>
>> > the<br>
>> > UI. But those can still be specified via additional compiler flags.<br>
>> > And<br>
>> > the maintenance burden is actually less than coupling it to the<br>
>> > installed<br>
>> > toolchain because we can do it at our leisure, rather than being pressed<br>
>> > to<br>
>> > get it done by a release.<br>
>><br>
>> But you're saying that the toolset should be independent of the LLVM<br>
>> version? If we add a -Wfoobar flag in Clang x.y.z and want to expose<br>
>> that in your UI, that UI then needs to be conditional on what version<br>
>> of Clang it's targeting. Same thing if we remove -Wquux in another<br>
>> Clang version. This sounds like a maintenance nightmare to me.<br>
><br>
> If we do nothing, any added -W options are still available via Additional<br>
> Compiler Flags. So as with all the other custom logic in the msbuild<br>
> files, we’re still not obligated to maintain that, and it will still<br>
> continue to work fine<br>
><br>
> For options that we remove, sure, we should update the file. One way to<br>
> handle this would be to add a new version of clang-cl.xml every release, and<br>
> conditionally include the proper xml file. How frequently do we remove<br>
> warnings though? Doing so would cause people’s builds to break because<br>
> they’d be passing unrecognized options, so I suspect it’s almost never.<br>
><br>
> Of all the things though, this is the one that I think it’s most important<br>
> to accept the maintenance burden of. This is the difference between “we put<br>
> the minimum amount of work possible into getting this working so we could do<br>
> other things” and “we care about this, we made it as easy as possible to<br>
> use, we designed it with VS users in mind”. As someone who used VS through<br>
> the UI exclusively for over 15 years, there’s going to be a huge difference<br>
> between providing this feature and not providing it.<br>
><br>
> I *still* don’t see the maintenance burden as being high though. We can<br>
> release a new clang-cl.xml like every 2-3 years and it would take all of 30<br>
> minutes to put it together and get it on the marketplace.<br>
><br>
><br>
><br>
>><br>
>><br>
>><br>
>> > One thing I could maybe do to lower the maintenance burden a little is<br>
>> > to<br>
>> > try to have some better logic for detecting the clang version. We were<br>
>> > already using the registry before anyway to find the installed LLVM,<br>
>> > maybe<br>
>> > there's a way for me to just figure out the version without the<br>
>> > additional<br>
>> > registry value. I'll have to look into that though.<br>
>> ><br>
>> ><br>
>> ><br>
>> > On Thu, Feb 1, 2018 at 11:09 AM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Thu, Feb 1, 2018 at 10:48 AM Hans Wennborg via Phabricator<br>
>> >> <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br>
>> >>><br>
>> >>> hans added inline comments.<br>
>> >>><br>
>> >>><br>
>> >>> ================<br>
>> >>> Comment at: llvm/tools/msbuild/Clang.Cpp.Common.props:41<br>
>> >>> +<br>
>> >>> + <!-- The registry key may not be set if it's an old installer,<br>
>> >>> try<br>
>> >>> the newest version that exists --><br>
>> >>> + <LLVMVersion Condition="'$(LLVMVersion)' == '' and<br>
>> >>> Exists('$(LLVMInstallDir)\lib\clang\7.0.0')">7.0.0</LLVMVersion><br>
>> >>> ----------------<br>
>> >>> zturner wrote:<br>
>> >>> > hans wrote:<br>
>> >>> > > As I mentioned before, separating the toolset config from the<br>
>> >>> > > actual<br>
>> >>> > > toolchain installation makes me a little nervous.<br>
>> >>> > ><br>
>> >>> > > But if we're doing it, the version checks below should probably<br>
>> >>> > > include the .1 versions too, i.e. at least 5.0.1 and 6.0.1.<br>
>> >>> > Unless we're going to release the full thing including the compiler,<br>
>> >>> > linker, etc through the marketplace I don't see an alternative. In<br>
>> >>> > any<br>
>> >>> > case, I actually think this it's preferable this way. There's<br>
>> >>> > nothing<br>
>> >>> > really about the two that benefits from having them coupled<br>
>> >>> > together, as far<br>
>> >>> > as I can see. I'm willing to be convinced though, if we can figure<br>
>> >>> > out how<br>
>> >>> > to to do it so that we can still ship it on the marketplace.<br>
>> >>> "There's nothing really about the two that benefits from having them<br>
>> >>> coupled together,"<br>
>> >>><br>
>> >>> The toolset needs to know at least where to find the toolchain and how<br>
>> >>> to<br>
>> >>> invoke it. If we decouple them, there needs to be an interface between<br>
>> >>> them<br>
>> >>> that can't change: in this case the LLVM path and version number in<br>
>> >>> the<br>
>> >>> registry.<br>
>> >><br>
>> >> Has that ever changed? Doesn’t seem too onerous, using the registry is<br>
>> >> the windows way anyway, if anything this feels like the proper way.<br>
>> >><br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>> But at the same time you're baking in all this logic in the toolset<br>
>> >>> about<br>
>> >>> how to invoke the toolchain, what flags are supported, etc. Those<br>
>> >>> things are<br>
>> >>> strongly dependent on the toolchain, which in this de-coupled world<br>
>> >>> seems<br>
>> >>> problematic. It seems like you're actually making the coupling tighter<br>
>> >>> in<br>
>> >>> that way, except you still want to ship the two parts separately.<br>
>> >>><br>
>> >>> Are there restrictions in the marketplace about how big a vsix can be?<br>
>> >>> Because if not, I think we could just package up clang+headers+runtime<br>
>> >>> into<br>
>> >>> a vsix and ship the whole thing, and maybe that would be the best<br>
>> >>> thing.<br>
>> >><br>
>> >> An installer is very large though, and even if it’s allowed it’s kind<br>
>> >> of<br>
>> >> obnoxious to have to download a large amount of stuff if only one thing<br>
>> >> changes. Being able to push changes to the Integration independently<br>
>> >> of an<br>
>> >> llvm release seems like a feature to me.<br>
>> >><br>
>> >> coupling it would also make it more difficult to use a custom built<br>
>> >> llvm<br>
>> >> toolchain, i can just change a registry setting right now and it’s good<br>
>> >> to<br>
>> >> go. Even the builtin VS toolchains use the registry to locate paths,<br>
>> >> and we<br>
>> >> were already reading the registry before this anyway<br>
>> >><br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>> Or we could just stick to the current installer version and make it a<br>
>> >>> little smarter about finding VS2017. Maybe instead of the batch files<br>
>> >>> we<br>
>> >>> write an actual program that finds the installation and copies the<br>
>> >>> files.<br>
>> >><br>
>> >> I definitely think vsix is the way to go. I’d hate to stick with batch<br>
>> >> files and not use the proper method of having an extension. It’s also<br>
>> >> more<br>
>> >> discoverable as an extension on the marketplace.<br>
>> >><br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>><br>
>> >>> ================<br>
>> >>> Comment at: llvm/tools/msbuild/LLVM.props:8<br>
>> >>> + <!-- Friendly names added to the PlatformToolset in the property<br>
>> >>> pages. --><br>
>> >>> + <_PlatformToolsetFriendlyNameFor_llvm<br>
>> >>> Condition="'$(_PlatformToolsetFriendlyNameFor_llvm)' == ''">Clang for<br>
>> >>> Windows</_PlatformToolsetFriendlyNameFor_llvm><br>
>> >>> + </PropertyGroup><br>
>> >>> ----------------<br>
>> >>> zturner wrote:<br>
>> >>> > hans wrote:<br>
>> >>> > > Hmm, we previously intentionally called the toolset "LLVM" with<br>
>> >>> > > the<br>
>> >>> > > thinking that it would eventually include lld and designated the<br>
>> >>> > > complete<br>
>> >>> > > llvm toolchain, not just Clang. And is the "for Windows" part<br>
>> >>> > > necessary?<br>
>> >>> > Do you think there's any value in having a toolset that does<br>
>> >>> > Clang+Link<br>
>> >>> > and a second one that does Clang+LLD? Or do you think we should<br>
>> >>> > stick with<br>
>> >>> > only a single one? I can change the name to LLVM though.<br>
>> >>> The best would be to only have one, but where the user could select<br>
>> >>> between the two linkers, I think.<br>
>> >><br>
>> >> Yea. Can try that in a followup, may be tricky though<br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>><br>
>> >>> ================<br>
>> >>> Comment at: llvm/tools/msbuild/Toolset.targets:38<br>
>> >>> +<br>
>> >>> + <!-- Warn if Fiber Safe Optimizations are enabled, and then<br>
>> >>> ignore<br>
>> >>> them. --><br>
>> >>> + <Warning Condition="'%(ClCompile.EnableFiberSafeOptimizations)'<br>
>> >>> ==<br>
>> >>> 'true'"<br>
>> >>> ----------------<br>
>> >>> zturner wrote:<br>
>> >>> > hans wrote:<br>
>> >>> > > This seems to duplicate a lot of logic from clang-cl. It's nice to<br>
>> >>> > > provide a good UI for the user, but maintaining this seems a lot<br>
>> >>> > > of work.<br>
>> >>> > > Are you not concerned that this will rot?<br>
>> >>> > I don't think it will. Maybe I'm being overly optimistic here, but<br>
>> >>> > the<br>
>> >>> > only case we would ever need to maintain this again is if we started<br>
>> >>> > supporting these options. Fiber Safe Optimizations, for example,<br>
>> >>> > I'm pretty<br>
>> >>> > sure we will never support. If MSVC ever removes the option, for<br>
>> >>> > example,<br>
>> >>> > we can do nothing and continue to work.<br>
>> >>> ><br>
>> >>> > We could also just silently ignore them and just pass the option<br>
>> >>> > through to clang-cl, but these are pretty unusual options with<br>
>> >>> > pretty<br>
>> >>> > specialized use cases, so I feel like if I had gone out of my way to<br>
>> >>> > enable<br>
>> >>> > such a strange option I would want to know if the compiler was not<br>
>> >>> > going to<br>
>> >>> > respect it.<br>
>> >>> I feel pretty strongly that we should handle this clang-cl side. If a<br>
>> >>> flag is not supported, either we should ignore it, or if it's<br>
>> >>> something the<br>
>> >>> user would want to know about us not supporting, we should warn.<br>
>> >>> That's what<br>
>> >>> clang-cl tries to do currently, and if there are flags we don't get<br>
>> >>> right,<br>
>> >>> we should fix it.<br>
>> >>><br>
>> >>> And we do move flags from the unsupported to supported category now<br>
>> >>> and<br>
>> >>> then, so the "only case we would ever need to maintain this again is<br>
>> >>> if we<br>
>> >>> started supporting these options" scenario is real.<br>
>> >><br>
>> >> It’s not a matter of clang-cl doing it right or wrong, it’s that there<br>
>> >> are<br>
>> >> other moving parts before it even gets to clang-cl. Specifically,<br>
>> >> MSBuild.<br>
>> >> We’ve already seen one example of how just letting clang-cl do its<br>
>> >> thing is<br>
>> >> insufficient, and nothing we can ever do in clang-cl can fix that.<br>
>> >> Given<br>
>> >> that it’s required sometimes, and that doing it for all options doesn’t<br>
>> >> increase our maintenance burden, i think it makes sense to do it<br>
>> >> everywhere<br>
>> >> and never have to deal with msbuild issues again.<br>
>> >><br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>><br>
>> >>> ================<br>
>> >>> Comment at: llvm/tools/msbuild/Toolset.targets:46<br>
>> >>> + File="@(ClCompile)(0,0)"<br>
>> >>> + Text="clang-cl does not support MSVC Link Time<br>
>> >>> Optimization. Disable this option in compatibility settings to<br>
>> >>> silence this<br>
>> >>> warning."/><br>
>> >>> +<br>
>> >>> ----------------<br>
>> >>> zturner wrote:<br>
>> >>> > hans wrote:<br>
>> >>> > > But maybe we want clang-cl to map this to -flto one day. Now we<br>
>> >>> > > need<br>
>> >>> > > to update two places. And with the toolset/toolchain install<br>
>> >>> > > split, the two<br>
>> >>> > > places may be installed separately :-/<br>
>> >>> > That's even better then. All we have to do is change this xml, push<br>
>> >>> > a<br>
>> >>> > new build to the market place, and the VS UI will update their<br>
>> >>> > extension for<br>
>> >>> > them.<br>
>> >>> ><br>
>> >>> > Note that we could do the mapping at the MSBuild level, in this file<br>
>> >>> > down below where we have an `ItemGroup`. Just add a line that says<br>
>> >>> > `<AdditionalOptions<br>
>> >>> > Condition="%(ClCompile.WholeProgramOptimization)' ==<br>
>> >>> > 'true'>-flto=thin %(AdditionalOptions)</AdditionalOptions>`<br>
>> >>> ><br>
>> >>> > and we can do this without touching clang.<br>
>> >>> But the toolset is decoupled from the toolchain in your proposal.<br>
>> >>><br>
>> >>> Not only would we need to update both clang-cl and this file, but this<br>
>> >>> file would need to handle clang-cl versions both before and after.<br>
>> >><br>
>> >> We wouldn’t have to update clang-cl. We could map ltcg to -flto=thin<br>
>> >> in<br>
>> >> the extension and it would automatically work with the installed<br>
>> >> toolchain.<br>
>> >><br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>><br>
>> >>> ================<br>
>> >>> Comment at: llvm/tools/msbuild/Toolset.targets:83<br>
>> >>> +<br>
>> >>> + <!-- Warn if XML Documentation is generated, and then ignore it.<br>
>> >>> --><br>
>> >>> + <Warning Condition="'%(ClCompile.GenerateXMLDocumentationFiles)'<br>
>> >>> ==<br>
>> >>> 'true'"<br>
>> >>> ----------------<br>
>> >>> zturner wrote:<br>
>> >>> > hans wrote:<br>
>> >>> > > Keeping up with all these flags seems like a huge amount of work.<br>
>> >>> > > Why<br>
>> >>> > > not just let clang-cl ignore it?<br>
>> >>> > See the large comment at the top of the file. For some options, we<br>
>> >>> > could probably get by with this. Maybe even this one, I debated on<br>
>> >>> > this<br>
>> >>> > particular one.<br>
>> >>> ><br>
>> >>> > My bar was "If the option fundamentally changes assumptions about<br>
>> >>> > the<br>
>> >>> > way code could be compiled, we should generate an error. If it<br>
>> >>> > changes the<br>
>> >>> > behavior of the language in a way we don't support, changes the way<br>
>> >>> > we<br>
>> >>> > generate code in a meaningful way, or causes specialized output<br>
>> >>> > files to be<br>
>> >>> > written, warn, and if it's an option we ignore then drop it"<br>
>> >>> ><br>
>> >>> > The last category there we could probably just pass through in some<br>
>> >>> > cases, but in that comment I also mentioned a case where setting an<br>
>> >>> > option<br>
>> >>> > that clang-cl ignores impacts MSBuild's ability to figure out<br>
>> >>> > dependencies<br>
>> >>> > and ends up causing a full rebuild every time even when nothing<br>
>> >>> > changed.<br>
>> >>> ><br>
>> >>> > We can scour the entire cl build tasks and try to discover if any<br>
>> >>> > other<br>
>> >>> > ones have unintended consequences, but I think it's easier to just<br>
>> >>> > turn them<br>
>> >>> > off at the MSBuild level. And as a side benefit, the user gets<br>
>> >>> > shorter<br>
>> >>> > command lines, which is always nice.<br>
>> >>> ><br>
>> >>> > As for maintenance, this all looks like zero-maintenance code to me.<br>
>> >>> > Did you have an example in mind of where we'd need to update this?<br>
>> >>> > Whether<br>
>> >>> > it be a new VS version, or VS dropping support for one of these<br>
>> >>> > options or<br>
>> >>> > deprecating them, I don't think we'd have to do anything.<br>
>> >>> The maintenance would come from when clang-cl changes how it handles<br>
>> >>> some<br>
>> >>> option, or when VS adds new options.<br>
>> >><br>
>> >> But these are all really obscure options that we will probably never<br>
>> >> touch. When vc adds new options we’re not obligated to update this<br>
>> >> file<br>
>> >> either.<br>
>> >><br>
>> >><br>
>> >>><br>
>> >>><br>
>> >>><br>
>> >>> ================<br>
>> >>> Comment at: llvm/tools/msbuild/install.bat:10<br>
>> >>> +REM Older versions of VS would look for these files in the Program<br>
>> >>> Files\MSBuild directory<br>
>> >>> +REM but with VS2017 it seems to look for these directly in the Visual<br>
>> >>> Studio instance.<br>
>> >>> +REM This means we'll need to do a little extra work to properly<br>
>> >>> detect<br>
>> >>> all the various<br>
>> >>> ----------------<br>
>> >>> zturner wrote:<br>
>> >>> > hans wrote:<br>
>> >>> > > Don't we want to support at least 2015 too?<br>
>> >>> > Mentioned in the other review, but the install.bat file shouldn't<br>
>> >>> > really be used anymore except for during development. The VSIX<br>
>> >>> > supports<br>
>> >>> > both 2015 and 2017 (I tested it in both and confirmed it works)<br>
>> >>> Hmm, but then we should delete it, or at least take it out of the<br>
>> >>> installer, and we need a replacement. As it is now, if we land this,<br>
>> >>> it<br>
>> >>> breaks the installer for versions before 2017.<br>
>> >><br>
>> >><br>
>> >> I thought i took it out of the installer, but maybe I missed something.<br>
>> >> We still need it for dev purposes because it allows us to overwrite the<br>
>> >> existing installed version with new files<br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > llvm-commits mailing list<br>
>> > <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
>> > <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>> ><br>
</blockquote></div>