[llvm] r328123 - Reapply Support layering fixes.

[David Blaikie via llvm-commits]

+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?

>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.

- 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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180604/13eb0cb5/attachment.html>