[llvm] r328123 - Reapply Support layering fixes.

[Justin Bogner via llvm-commits]

Chandler Carruth via llvm-commits <llvm-commits at lists.llvm.org> writes:
> 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:
>>> 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:
>>>>> On Fri, Jun 1, 2018 at 2:31 PM Justin Bogner <mail at justinbogner.com> wrote:
>>>>>> 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.

I meant that we could hoist Signals out of Support so that it depended
on both Demangle and Support, then Demangle depends on Support but
Support does not depend on demangle. Crude ascii to demonstrate:

            ,--> Demangle -.
  Signals -'----------------`--> Support 

In any case, it sounds like we have a simpler solution below.

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

It seems like it'd be overkill based on the below, but I do think moving
Compiler.h into Config/ or some other more-generic-than-support place
would be a reasonable solution for this class of problem. Compiler.h is
literally feature test macros, it's only dependencies are on the host
system and compiler by definition.

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

+1. If we're decoupling this in the libcxxabi case anyway we might as
well just give Demangle its own set of feature test macros for these and
be done with it.