[PATCH] D42762: Rewrite the VS Integration Scripts

Zachary Turner via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 2 07:40:59 PST 2018


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.

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/25f48341/attachment.html>


More information about the llvm-commits mailing list