[PATCH] D42762: Rewrite the VS Integration Scripts

[Zachary Turner via llvm-commits]

Symptom: when /Zi is selected, VS always rebuilds all source files, even if
just 1 (possibly even none) have changed.

Fix: Change /Zi to /Z7 in the UI

More details here: https://bugs.llvm.org/show_bug.cgi?id=36140
On Fri, Feb 2, 2018 at 8:12 AM Hans Wennborg <hans at chromium.org> wrote:

> On Fri, Feb 2, 2018 at 4:40 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > On Fri, Feb 2, 2018 at 6:23 AM Hans Wennborg <hans at chromium.org> wrote:
> >>
> >> (Your reply didn't go to Phabricator, so re-adding folks subscribed
> >> there.)
> >>
> >> On Thu, Feb 1, 2018 at 9:08 PM, Zachary Turner via llvm-commits
> >> <llvm-commits at lists.llvm.org> wrote:
> >> > I'm kind of imagining this world where we have one VS Integration that
> >> > works
> >> > no matter what version of LLVM you have.  The nice thing about this is
> >> > that
> >> > it allows it to work with hermetic toolchains, older versions of LLVM
> >> > that
> >> > may already be installed on a user's machine, local dev builds of
> LLVM,
> >> > etc.
> >>
> >> I'm on board with this. It seems useful especially for the case where
> >> the developer may have multiple LLVM toolchains installed and want to
> >> point at a specific one. It would be nice if we could still trigger
> >> the installation of the toolset when installing the LLVM toolchain
> >> though.
> >>
> >> But for the integration to work regardless of LLVM version, I don't
> >> think the integration can bake in assumptions about what flags
> >> clang-cl supports and re-map them etc. The set of flags supported by
> >> clang-cl and how they're handled changes frequently.
> >>
> >> I like the idea of a VS integration that's LLVM-version independent --
> >> the current one is almost that except for the baked in version number
> >> -- but for it to work, I think it has to be really simple, basically
> >> just pointing MSVC at clang-cl.exe and nothing more.
> >
> > I’ve already mentioned at least one case whereas this is impossible (/Zi
> vs
> > /Z7), and given that there are thousands of lines of msbuild logic that
> are
> > running and processing these options before they make it to clang-cl, I’m
> > certain there are more that we don’t yet know about.
>
> I'd like to understand the /Zi vs /Z7 thing better. Can you ELI5 the
> problem?
>
>
> >
> > Simple is nice, I don’t disagree with that, but not at the expense of
> user
> > experience.  i still don’t think there’s any maintenance issues here
> though.
> > I think the current version here could probably sit for 5+ years and
> never
> > need to be touched, continuing to work both with future VS versions and
> > future clang-cl versions unmodified.
> >
> >
> >>
> >>
> >> > The only maintenance burden I can think of is one where we remove or
> add
> >> > flags in clang-cl, which doesn't happen very often, if ever.
> >>
> >> It puzzles me that you think we rarely or ever change the flags
> >> clang-cl supports or how they're handled. In my experience, the flags
> >> change every release.
> >>
> >> >  Any flag that
> >> > is added to MSVC doesn't require any action from us.
> >>
> >> Depends on the flag, no?
> >
> > Not really.  Any flag that msvc adds, assuming we don’t update this file,
> > gets passed through to clang-cl which is what you’re proposing i do with
> all
> > options anyway.
> >
> >
> >>
> >>
> >> > I plan to expose a UI for optimizations and warning, so I could see a
> >> > maintenace burden when we add new -W or -f flags that are not exposed
> to
> >> > the
> >> > UI.  But those can still be specified via additional compiler flags.
> >> > And
> >> > the maintenance burden is actually less than coupling it to the
> >> > installed
> >> > toolchain because we can do it at our leisure, rather than being
> pressed
> >> > to
> >> > get it done by a release.
> >>
> >> But you're saying that the toolset should be independent of the LLVM
> >> version? If we add a -Wfoobar flag in Clang x.y.z and want to expose
> >> that in your UI, that UI then needs to be conditional on what version
> >> of Clang it's targeting. Same thing if we remove -Wquux in another
> >> Clang version. This sounds like a maintenance nightmare to me.
> >
> > If we do nothing, any added -W options are still available via Additional
> > Compiler Flags.   So as with all the other custom logic in the msbuild
> > files, we’re still not obligated to maintain that, and it will still
> > continue to work fine
> >
> > For options that we remove, sure, we should update the file.  One way to
> > handle this would be to add a new version of clang-cl.xml every release,
> and
> > conditionally include the proper xml file.  How frequently do we remove
> > warnings though?  Doing so would cause people’s builds to break because
> > they’d be passing unrecognized options, so I suspect it’s almost never.
> >
> > Of all the things though, this is the one that I think it’s most
> important
> > to accept the maintenance burden of.  This is the difference between “we
> put
> > the minimum amount of work possible into getting this working so we
> could do
> > other things” and “we care about this, we made it as easy as possible to
> > use, we designed it with VS users in mind”.  As someone who used VS
> through
> > the UI exclusively for over 15 years, there’s going to be a huge
> difference
> > between providing this feature and not providing it.
> >
> > I *still* don’t see the maintenance burden as being high though.  We can
> > release a new clang-cl.xml like every 2-3 years and it would take all of
> 30
> > minutes to put it together and get it on the marketplace.
> >
> >
> >
> >>
> >>
> >>
> >> > One thing I could maybe do to lower the maintenance burden a little is
> >> > to
> >> > try to have some better logic for detecting the clang version.  We
> were
> >> > already using the registry before anyway to find the installed LLVM,
> >> > maybe
> >> > there's a way for me to just figure out the version without the
> >> > additional
> >> > registry value.  I'll have to look into that though.
> >> >
> >> >
> >> >
> >> > On Thu, Feb 1, 2018 at 11:09 AM Zachary Turner <zturner at google.com>
> >> > wrote:
> >> >>
> >> >> On Thu, Feb 1, 2018 at 10:48 AM Hans Wennborg via Phabricator
> >> >> <reviews at reviews.llvm.org> wrote:
> >> >>>
> >> >>> hans 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>
> >> >>> ----------------
> >> >>> zturner wrote:
> >> >>> > 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.
> >> >>> "There's nothing really about the two that benefits from having them
> >> >>> coupled together,"
> >> >>>
> >> >>> The toolset needs to know at least where to find the toolchain and
> how
> >> >>> to
> >> >>> invoke it. If we decouple them, there needs to be an interface
> between
> >> >>> them
> >> >>> that can't change: in this case the LLVM path and version number in
> >> >>> the
> >> >>> registry.
> >> >>
> >> >> Has that ever changed?  Doesn’t seem too onerous, using the registry
> is
> >> >> the windows way anyway, if anything this feels like the proper way.
> >> >>
> >> >>
> >> >>>
> >> >>>
> >> >>> But at the same time you're baking in all this logic in the toolset
> >> >>> about
> >> >>> how to invoke the toolchain, what flags are supported, etc. Those
> >> >>> things are
> >> >>> strongly dependent on the toolchain, which in this de-coupled world
> >> >>> seems
> >> >>> problematic. It seems like you're actually making the coupling
> tighter
> >> >>> in
> >> >>> that way, except you still want to ship the two parts separately.
> >> >>>
> >> >>> Are there restrictions in the marketplace about how big a vsix can
> be?
> >> >>> Because if not, I think we could just package up
> clang+headers+runtime
> >> >>> into
> >> >>> a vsix and ship the whole thing, and maybe that would be the best
> >> >>> thing.
> >> >>
> >> >> An installer is very large though, and even if it’s allowed it’s kind
> >> >> of
> >> >> obnoxious to have to download a large amount of stuff if only one
> thing
> >> >> changes.  Being able to push changes to the Integration independently
> >> >> of an
> >> >> llvm release seems like a feature to me.
> >> >>
> >> >>  coupling it would also make it more difficult to use a custom built
> >> >> llvm
> >> >> toolchain, i can just change a registry setting right now and it’s
> good
> >> >> to
> >> >> go.  Even the builtin VS toolchains use the registry to locate paths,
> >> >> and we
> >> >> were already reading the registry before this anyway
> >> >>
> >> >>
> >> >>>
> >> >>>
> >> >>> Or we could just stick to the current installer version and make it
> a
> >> >>> little smarter about finding VS2017. Maybe instead of the batch
> files
> >> >>> we
> >> >>> write an actual program that finds the installation and copies the
> >> >>> files.
> >> >>
> >> >> I definitely think vsix is the way to go.  I’d hate to stick with
> batch
> >> >> files and not use the proper method of having an extension.  It’s
> also
> >> >> more
> >> >> discoverable as an extension 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>
> >> >>> ----------------
> >> >>> 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.
> >> >>
> >> >> Yea.  Can try that in a followup, may be tricky 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'"
> >> >>> ----------------
> >> >>> 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.
> >> >>
> >> >> It’s not a matter of clang-cl doing it right or wrong, it’s that
> there
> >> >> are
> >> >> other moving parts before it even gets to clang-cl.  Specifically,
> >> >> MSBuild.
> >> >> We’ve already seen one example of how  just letting clang-cl do its
> >> >> thing is
> >> >> insufficient, and nothing we can ever do in clang-cl can fix that.
> >> >> Given
> >> >> that it’s required sometimes, and that doing it for all options
> doesn’t
> >> >> increase our maintenance burden, i think it makes sense to do it
> >> >> everywhere
> >> >> and never have to deal with msbuild issues again.
> >> >>
> >> >>
> >> >>>
> >> >>>
> >> >>>
> >> >>> ================
> >> >>> 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."/>
> >> >>> +
> >> >>> ----------------
> >> >>> zturner wrote:
> >> >>> > 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.
> >> >>> But the toolset is decoupled from the toolchain in your proposal.
> >> >>>
> >> >>> Not only would we need to update both clang-cl and this file, but
> this
> >> >>> file would need to handle clang-cl versions both before and after.
> >> >>
> >> >> We wouldn’t have to update clang-cl.  We could map ltcg to -flto=thin
> >> >> in
> >> >> the extension and it would automatically work with the installed
> >> >> toolchain.
> >> >>
> >> >>
> >> >>>
> >> >>>
> >> >>>
> >> >>> ================
> >> >>> Comment at: llvm/tools/msbuild/Toolset.targets:83
> >> >>> +
> >> >>> +    <!-- Warn if XML Documentation is generated, and then ignore
> it.
> >> >>> -->
> >> >>> +    <Warning
> Condition="'%(ClCompile.GenerateXMLDocumentationFiles)'
> >> >>> ==
> >> >>> 'true'"
> >> >>> ----------------
> >> >>> zturner wrote:
> >> >>> > 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.
> >> >>> The maintenance would come from when clang-cl changes how it handles
> >> >>> some
> >> >>> option, or when VS adds new options.
> >> >>
> >> >> But these are all really obscure options that we will probably never
> >> >> touch.  When vc adds new options we’re not obligated to update this
> >> >> file
> >> >> either.
> >> >>
> >> >>
> >> >>>
> >> >>>
> >> >>>
> >> >>> ================
> >> >>> 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
> >> >>> ----------------
> >> >>> zturner wrote:
> >> >>> > 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)
> >> >>> Hmm, but then we should delete it, or at least take it out of the
> >> >>> installer, and we need a replacement. As it is now, if we land this,
> >> >>> it
> >> >>> breaks the installer for versions before 2017.
> >> >>
> >> >>
> >> >> I thought i took it out of the installer, but maybe I missed
> something.
> >> >> We still need it for dev purposes because it allows us to overwrite
> the
> >> >> existing installed version with new files
> >> >
> >> >
> >> > _______________________________________________
> >> > llvm-commits mailing list
> >> > llvm-commits at lists.llvm.org
> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180202/2f8e2b0a/attachment.html>