[PATCH] D42762: Rewrite the VS Integration Scripts

Fri Feb 2 09:23:14 PST 2018

clang-cl attempts to be compatible to cl. If /Zi is supposd ot create a
vc140.pdb, then clang-cl should probably put an empty pdb there if the file
doesn't already exist.

On Fri, Feb 2, 2018 at 12:05 PM, Zachary Turner <zturner at google.com> wrote:

> It’s an additional pdb, which the linker then uses as an input. The linker
> still writes the final pdb. The msbuild task that actually invokes cl.exe
> is implemented in a dll, so we can only observe its effects.
>
> I’m not sure how cmake is implemented, but this is really a build system
> issue of inputs, outputs, and dependencies. If cmake does have issues,
> patching cmake seems like the appropriate solution.
>
> On Fri, Feb 2, 2018 at 8:54 AM Hans Wennborg <hans at chromium.org> wrote:
>
>> I'd still like to understand how msbuild.exe is running the build to
>> expect vc140.pdb to be written, and why we're outputing the .pdb file
>> in the wrong place. Or is vc140.pdb some additional pdb that's
>> supposed to get output on the side?
>>
>> I don't like the idea of the vs integration rewriting flags. What
>> about users not using msbuild but cmake? Should cmake start rewriting
>> flags for clang-cl too?
>>
>> On Fri, Feb 2, 2018 at 5:49 PM, Zachary Turner <zturner at google.com>
>> wrote:
>> > That’s why we unset the option in msbuild, so it no longer expects
>> that. The
>> > current solution makes it behave exactly as if /Z7 had been chosen in
>> the
>> > UI. Having the compiler write a type server pdb seems like an enormous
>> > amount of work, and writing a 0 byte file seems odd.
>> >
>> > On Fri, Feb 2, 2018 at 8:46 AM Hans Wennborg <hans at chromium.org> wrote:
>> >>
>> >> But if the build system invokes the compiler + linker, expects to end
>> >> up with vc140.pdb but instead ends up with foo.pdb, maybe the compiler
>> >> + linker is not interpreting the flags passed from the build system
>> >> correctly.
>> >>
>> >> On Fri, Feb 2, 2018 at 5:44 PM, Zachary Turner <zturner at google.com>
>> wrote:
>> >> > Also, since this is a build system issue and not a compiler issue, it
>> >> > seems
>> >> > intuitive to me to deal with it at the build system level
>> >> > On Fri, Feb 2, 2018 at 8:42 AM Zachary Turner <zturner at google.com>
>> >> > wrote:
>> >> >>
>> >> >> It’s a compiler generated pdb, /Zi means “all compiler processes
>> should
>> >> >> write to the same pdb”, whereas /Z7 means “put the debug info in the
>> >> >> object
>> >> >> files instead”. If the user does a clean build the file will get
>> >> >> deleted and
>> >> >> there won’t even be anything to touch. The file name comes from
>> another
>> >> >> flag
>> >> >> (/Fo or /Fd, can’t remember) which msbuild defaults to
>> vc$(ToolsetName)
>> >> >> On Fri, Feb 2, 2018 at 8:36 AM Hans Wennborg <hans at chromium.org>
>> wrote:
>> >> >>>
>> >> >>> Yes, but I'd like to understand exactly why. Where does the name
>> >> >>> vc140.pdb come from? What is supposed to go into this file? Maybe
>> >> >>> clang-cl should touch it when invoked with /Zi?
>> >> >>>
>> >> >>> On Fri, Feb 2, 2018 at 5:32 PM, Zachary Turner <zturner at google.com
>> >
>> >> >>> wrote:
>> >> >>> > 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/409bfc73/attachment-0001.html>