[llvm] r328123 - Reapply Support layering fixes.

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 4 15:19:04 PDT 2018 

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?

#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

#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/a2d8a163/attachment.html>

More information about the llvm-commits mailing list