[llvm] r328123 - Reapply Support layering fixes.

Mon Jun 4 11:47:12 PDT 2018

On Mon, Jun 4, 2018 at 8:46 PM Erik Pilkington via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
>
> On 2018-06-04 2:20 PM, David Blaikie wrote:
>
> +Erik who seems to be someone interested in this API
>
> On Mon, Jun 4, 2018 at 11:11 AM Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>> On Mon, Jun 4, 2018 at 8:01 PM David Blaikie via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> (+Chandler since he had some feedback on another layering change anyway)
>>>
>>> On Fri, Jun 1, 2018 at 2:31 PM Justin Bogner <mail at justinbogner.com>
>>> wrote:
>>>
>>>> David Blaikie via llvm-commits <llvm-commits at lists.llvm.org> writes:
>>>> > Author: dblaikie
>>>> > Date: Wed Mar 21 10:31:49 2018
>>>> > New Revision: 328123
>>>> >
>>>> > URL: http://llvm.org/viewvc/llvm-project?rev=328123&view=rev
>>>> > Log:
>>>> > Reapply Support layering fixes.
>>>> >
>>>> > Compiler.h is used by Demangle (which Support depends on) - so sink it
>>>> > into Demangle to avoid a circular dependency
>>>>
>>>> Sorry for seeing this a bit late, but this fix seems very wrong to me.
>>>>
>>>> > DataTypes.h is used by llvm-c (which Support depends on) - so sink it
>>>> > into llvm-c.
>>>> >
>>>> > DataTypes.h could probably be fixed the other way - making llvm-c
>>>> depend
>>>> > on Support instead of Support depending on llvm-c - if anyone feels
>>>> > that's the better option, happy to work with them on that.
>>>> >
>>>> > I /think/ this'll address the layering issues that previous attempts
>>>> to
>>>> > commit this have triggered in the Modules buildbot, but I haven't been
>>>> > able to reproduce that build so can't say for sure. If anyone's having
>>>> > trouble with this - it might be worth taking a look to see if there's
>>>> a
>>>> > quick fix/something small I missed rather than revert, but no worries.
>>>> >
>>>> > Copied: llvm/trunk/include/llvm/Demangle/Compiler.h (from r328122,
>>>> llvm/trunk/include/llvm/Support/Compiler.h)
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Demangle/Compiler.h?p2=llvm/trunk/include/llvm/Demangle/Compiler.h&p1=llvm/trunk/include/llvm/Support/Compiler.h&r1=328122&r2=328123&rev=328123&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- llvm/trunk/include/llvm/Support/Compiler.h (original)
>>>> > +++ llvm/trunk/include/llvm/Demangle/Compiler.h Wed Mar 21 10:31:49
>>>> 2018
>>>> > @@ -1,4 +1,4 @@
>>>> > -//===-- llvm/Support/Compiler.h - Compiler abstraction support --*-
>>>> C++ -*-===//
>>>> > +//===-- llvm/Demangle/Compiler.h - Compiler abstraction support -*-
>>>> C++ -*-===//
>>>> >  //
>>>> >  //                     The LLVM Compiler Infrastructure
>>>> >  //
>>>> >
>>>> > Modified: llvm/trunk/include/llvm/Support/Compiler.h
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=328123&r1=328122&r2=328123&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- llvm/trunk/include/llvm/Support/Compiler.h (original)
>>>> > +++ llvm/trunk/include/llvm/Support/Compiler.h Wed Mar 21 10:31:49
>>>> 2018
>>>> > @@ -7,500 +7,13 @@
>>>> >  //
>>>> >
>>>> //===----------------------------------------------------------------------===//
>>>> >  //
>>>> > -// This file defines several macros, based on the current compiler.
>>>> This allows
>>>> > -// use of compiler-specific features in a way that remains portable.
>>>> > +// Due to layering constraints (Support depends on Demangler) this
>>>> is a thin
>>>> > +// wrapper around the implementation that lives in llvm-c, though
>>>> most clients
>>>> > +// can/should think of this as being provided by Support for
>>>> simplicity (not
>>>> > +// many clients are aware of their dependency on Demangler/it's a
>>>> weird place to
>>>> > +// own this - but didn't seem to justify splitting Support into
>>>> "lower support"
>>>> > +// and "upper support").
>>>>
>>>> Demangler isn't only a "weird place" to own this, it's a complete break
>>>> in abstraction. None of this stuff has to do with demangler at all, so
>>>> sinking it there because demangler happens to depend in it is purely a
>>>> hack. I feel like the fact that we left a shim header in Compiler.h to
>>>> paper over it is pretty strong evidence that you agree here.
>>>>
>>>
>>> Ish. I'm not feeling all that principled or worried about these layering
>>> fixes - it doesn't feel like this breaks anything in a huge way, though it
>>> doesn't feel super great either - though introducing new libraries to
>>> support the variations of layering that are required might be more hassle
>>> than its worth too.
>>>
>>>
>>>> I assume you considered moving Signals.h (the only thing that depends on
>>>> Demangle) out of Support and rejected that idea for one reason or
>>>> another, but that's obviously one option for doing this more cleanly.
>>>>
>>>
>>> Hadn't looked at that option, really, though...
>>>
>>> Looks like that'd have the same problem - since Signals.h is included
>>> from a bunch of places (fewer than Compiler.h, but still tens) & similarly
>>> has nothing to do with the Demangler.
>>>
>>> Or were you suggesting moving it somewhere else? Where? & its
>>> implementation (lib/Support/Signals.cpp) depends on several Support headers
>>> - so it couldn't be easily layered below Support by the looks of it.
>>>
>>>
>>>> If that's problematic for some reason, I think a pretty strong argument
>>>> could be made for moving Compiler.h to Config/ - it's header only and
>>>> really only exists to paper over differences in the build configuration.
>>>>
>>>
>>> We currently don't have any plain headers in Config - so I'm not sure if
>>> that'd work straight off the bat, but don't mind moving it there - is it
>>> worth renaming everything that includes "Support/Compiler.h" to include
>>> "Config/Compiler.h" though? Does that feel better/more right? I'm not
>>> really sure/don't much mind.
>>>
>>
>> I don't think this really helps.
>>
>> I think that if we want Demangle to not depend on Support, we should make
>> it not depend on Support --> remove all uses of constructs from Compiler.h
>> and directly use the language /compiler primitives.
>>
>> Fundamentally, if Demangle needs to be "stand alone", we need to make
>> that 100% true. If we don't then it should just be merged into Support
>> itself.
>>
>
> Yeah, I have no particular context on why/how this library is structured -
> I guess it's isolated from Support/other LLVM libs so it can eventually be
> used in the implementation of libcxxabi?
>
> Yep, exactly. It is in fact already used in libcxxabi, that repo has an
> identical copy of the demangler here in LLVM. This is a somewhat awkward
> state of affairs, but its the best we can do given that libcxxabi needs a
> standalone demangler, and we don't want to have to support 2 distinct
> demanglers in LLVM.
>
> The LLVM demangler's dependency on Compiler.h is a weak one, we only use
> LLVM_UNREACHABLE and LLVM_FALLTHROUGH. If we're having problems with
> layering in Support then I think we should just provide simple definitions
> for these at the top of ItaniumDemangle.cpp.
>

This at least seems like a significantly better short or medium term state.

>
>
> From Rafael's original commit:
>
>     My current plan is:
>
>         Commit something like this
>         Change lld to use it
>         Change lldb to use it as the fallback
>
>         Add a few #ifdefs so that exactly the same file can be used in
>         libcxxabi to export abi::__cxa_demangle.
>
>     Once the fast demangler in lldb can handle any names this
>     implementation can be replaced with it and we will have the one true
>     demangler.
>
> Not sure if anyone else is pursuing that goal now that Rafael's left the
> project and/or how much we want to leave it in a state that makes that more
> possible even if no one's actively pushing in that direction just now.
>
> After skimming the source code, the fast demangler in LLDB really isn't
> anywhere close to being a complete implementation, and has some structural
> problems that would make it impossible to replace the full demangler. The
> demangler in LLVM has seen a lot of work in the past year, and is now much
> faster and much safer. I think that it should be the one true demangler in
> LLVM, hopefully allowing us to remove FastDemangle.
>
> - Dave
>
>
>
>>
>>
>>>
>>>
>>>>
>>>>
>>>> >  //
>>>> >
>>>> //===----------------------------------------------------------------------===//
>>>> >
>>>> > -#ifndef LLVM_SUPPORT_COMPILER_H
>>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
> _______________________________________________
> 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/20180604/2c3a7a3d/attachment.html>