[llvm] r328123 - Reapply Support layering fixes.
Erik Pilkington via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 4 11:56:59 PDT 2018
On 2018-06-04 2:50 PM, David Blaikie wrote:
>
>
> On Mon, Jun 4, 2018 at 11:45 AM Erik Pilkington
> <erik.pilkington at gmail.com <mailto:erik.pilkington at gmail.com>> 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.
>
>
> If it's an identical copy, how's it dealing with the Compiler.h
> dependency?
Not *identical* identical, it uses cxxabi_config.h to get these macros.
If we're providing custom defns for unreachable and fallthrough, then we
might as well define these in cxa_demangle.cpp too.
>
> 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.
>
>
>> 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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180604/5fe1e3b6/attachment.html>
More information about the llvm-commits
mailing list