[llvm] r328123 - Reapply Support layering fixes.

[David Blaikie via llvm-commits]

Committed in r333965

On Mon, Jun 4, 2018 at 3:28 PM Erik Pilkington <erik.pilkington at gmail.com>
wrote:

>
>
> On 2018-06-04 6:19 PM, David Blaikie wrote:
>
>
>
> On Mon, Jun 4, 2018 at 11:47 AM Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>> 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.
>>
>
> Fair enough - here's all the macros that'd be needed for
> ItaniumDemangle.cpp I think. Reckon that's OK to duplicate here? Should I
> bring over the comments too, or leave a comment that this is all from
> Support/Compiler.h?
>
> That seems fine to me, that comment saying that this is all from
> Compiler.h would be nice.
>
>
> #ifndef __has_feature
> #define __has_feature(x) 0
> #endif
>
> #ifndef __has_cpp_attribute
> #define __has_cpp_attribute(x) 0
> #endif
>
> #ifndef __has_attribute
> #define __has_attribute(x) 0
> #endif
>
> #ifndef __has_builtin
> #define __has_builtin(x) 0
> #endif
>
> #ifndef LLVM_GNUC_PREREQ
> #if defined(__GNUC__) && defined(__GNUC_MINOR__) &&
> defined(__GNUC_PATCHLEVEL__)
> #define LLVM_GNUC_PREREQ(maj, min, patch)
>     \
>   ((__GNUC__ << 20) + (__GNUC_MINOR__ << 10) + __GNUC_PATCHLEVEL__ >=
>     \
>    ((maj) << 20) + ((min) << 10) + (patch))
> #elif defined(__GNUC__) && defined(__GNUC_MINOR__)
> #define LLVM_GNUC_PREREQ(maj, min, patch)
>     \
>   ((__GNUC__ << 20) + (__GNUC_MINOR__ << 10) >= ((maj) << 20) + ((min) <<
> 10))
> #else
> #define LLVM_GNUC_PREREQ(maj, min, patch) 0
> #endif
> #endif
>
> #if __has_feature(cxx_rvalue_references) || LLVM_GNUC_PREREQ(4, 8, 1)
> #define LLVM_HAS_RVALUE_REFERENCE_THIS 1
> #else
> #define LLVM_HAS_RVALUE_REFERENCE_THIS 0
> #endif
>
> Why do we need this macro?
>

Ah, thanks for the catch - seems we didn't, I must've copy/pasted a bit too
much!


>
>
> #if __has_attribute(used) || LLVM_GNUC_PREREQ(3, 1, 0)
> #define LLVM_ATTRIBUTE_USED __attribute__((__used__))
> #else
> #define LLVM_ATTRIBUTE_USED
> #endif
>
> #if __has_builtin(__builtin_unreachable) || LLVM_GNUC_PREREQ(4, 5, 0)
> #define LLVM_BUILTIN_UNREACHABLE __builtin_unreachable()
> #elif defined(_MSC_VER)
> #define LLVM_BUILTIN_UNREACHABLE __assume(false)
> #endif
>
> #if __has_attribute(noinline) || LLVM_GNUC_PREREQ(3, 4, 0)
> #define LLVM_ATTRIBUTE_NOINLINE __attribute__((noinline))
> #elif defined(_MSC_VER)
> #define LLVM_ATTRIBUTE_NOINLINE __declspec(noinline)
> #else
> #define LLVM_ATTRIBUTE_NOINLINE
> #endif
>
> #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
> #define LLVM_DUMP_METHOD LLVM_ATTRIBUTE_NOINLINE LLVM_ATTRIBUTE_USED
> #else
> #define LLVM_DUMP_METHOD LLVM_ATTRIBUTE_NOINLINE
> #endif
>
> #if __cplusplus > 201402L && __has_cpp_attribute(fallthrough)
> #define LLVM_FALLTHROUGH [[fallthrough]]
> #elif __has_cpp_attribute(gnu::fallthrough)
> #define LLVM_FALLTHROUGH [[gnu::fallthrough]]
> #elif !__cplusplus
> // Workaround for llvm.org/PR23435, since clang 3.6 and below emit a
> spurious
> // error when __has_cpp_attribute is given a scoped attribute in C mode.
> #define LLVM_FALLTHROUGH
> #elif __has_cpp_attribute(clang::fallthrough)
> #define LLVM_FALLTHROUGH [[clang::fallthrough]]
> #else
> #define LLVM_FALLTHROUGH
> #endif
>
>
>>
>>>
>>> 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/a9e5dc1b/attachment.html>