[PATCH] D42762: Rewrite the VS Integration Scripts

[Zachary Turner via llvm-commits]

Sure, we could update it every time someone touches the cl driver if we
wanted to.  My premise is that we would almost never *need* to update the
driver, because almost anything that changes between points A and B would
still be available through "Additional Compiler Options" on the Command
Line page.  Since changes to the driver are almost always additive, the set
of changes that would *necessitate* an update to the UI is so small that
the benefits of giving people a UI custom tailored to clang-cl would
outweigh the small maintenance cost associated with fixing backwards
compatibility breakages.

On Fri, Feb 2, 2018 at 9:41 AM Nico Weber <thakis at chromium.org> wrote:

> Independent of everything, I pretty strongly believe that we should assume
> that trunk msvc plugin goes with trunk llvm -- O(1) vs O(n_versions) config
> complexity.
>
> On Fri, Feb 2, 2018 at 12:33 PM, Zachary Turner <zturner at google.com>
> wrote:
>
>> Note also that any change we make to clang-cl to output an empty PDB
>> would only affect 7.0 and higher.  Fixing it in the build system would
>> allow it to work with old versions of LLVM
>>
>> On Fri, Feb 2, 2018 at 9:31 AM Nico Weber <thakis at chromium.org> wrote:
>>
>>> Anyhoo, to move this along, maybe put the rewriting part into a
>>> different diff. There's probably agreement on the vast majority of this
>>> diff :-)
>>>
>>> On Fri, Feb 2, 2018 at 12:30 PM, Nico Weber <thakis at chromium.org> wrote:
>>>
>>>> I'm not saying a corrupt file, but an empty valid pdb file.
>>>>
>>>> Trying to be compatible with cl means being compatible with its
>>>> observable effects as far as reasonable.
>>>>
>>>> On Fri, Feb 2, 2018 at 12:27 PM, Zachary Turner <zturner at google.com>
>>>> wrote:
>>>>
>>>>> I would also argue that clang-cl only tries to be compatible **with
>>>>> cl**.  Not with every possible build system that might support cl.  The
>>>>> compiler should not be be trying to support the build system, it's the
>>>>> other way around.
>>>>>
>>>>> On Fri, Feb 2, 2018 at 9:24 AM Zachary Turner <zturner at google.com>
>>>>> wrote:
>>>>>
>>>>>> I disagree with this.  We should not be putting emitting a corrupt
>>>>>> file under any circumstances.  That's even worse for compatibility than
>>>>>> doing nothing.
>>>>>>
>>>>>> On Fri, Feb 2, 2018 at 9:24 AM Zachary Turner <zturner at google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> I tested and CMake is buggy with respect to how it handles /Zi
>>>>>>> (perhaps it's an intentional tradeoff, I don't know).  Steps to reproduce
>>>>>>> this test:
>>>>>>>
>>>>>>> 1) Build obj2yaml from LLVM using CMake and cl
>>>>>>> 2) delete tools\obj2yaml\CMakeFiles\obj2yaml.dir\vc140.pdb
>>>>>>> 3) delete tools\obj2yaml\CMakeFiles\obj2yaml.dir\coff2yaml.pdb
>>>>>>> 4) delete bin\obj2yaml.pdb
>>>>>>> 5) ninja obj2yaml
>>>>>>>
>>>>>>> You'll get these warnings:
>>>>>>> LINK : program database
>>>>>>> D:\src\llvmbuild\cl\Debug\x64\bin\obj2yaml.pdb missing; performing full link
>>>>>>> obj2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found
>>>>>>> with 'obj2yaml.cpp.obj' or at
>>>>>>> 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb'; linking object as if no
>>>>>>> debug info
>>>>>>> dwarf2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found
>>>>>>> with 'dwarf2yaml.cpp.obj' or at
>>>>>>> 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb'; linking object as if no
>>>>>>> debug info
>>>>>>> elf2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found
>>>>>>> with 'elf2yaml.cpp.obj' or at
>>>>>>> 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb'; linking object as if no
>>>>>>> debug info
>>>>>>> macho2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found
>>>>>>> with 'macho2yaml.cpp.obj' or at
>>>>>>> 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb'; linking object as if no
>>>>>>> debug info
>>>>>>> wasm2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found
>>>>>>> with 'wasm2yaml.cpp.obj' or at
>>>>>>> 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb'; linking object as if no
>>>>>>> debug info
>>>>>>> Error.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found with
>>>>>>> 'Error.cpp.obj' or at 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb';
>>>>>>> linking object as if no debug info
>>>>>>>
>>>>>>> What it should be doing is saying that every cpp file, in addition
>>>>>>> to $(FileName).obj, also produces vc140.pdb.  MSBuild gets this correct
>>>>>>>
>>>>>>> On Fri, Feb 2, 2018 at 9:05 AM 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/5d65c102/attachment-0001.html>