[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