[llvm] r328123 - Reapply Support layering fixes.

Erik Pilkington via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 4 15:28:48 PDT 2018



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 
> <mailto: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 <mailto: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 <mailto: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
>>             <mailto: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
>>                 <mailto:mail at justinbogner.com>> wrote:
>>
>>                     David Blaikie via llvm-commits
>>                     <llvm-commits at lists.llvm.org
>>                     <mailto: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?

> #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 <http://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
>>                 <mailto: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 <mailto: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/b7724d83/attachment.html>


More information about the llvm-commits mailing list