[llvm] r328123 - Reapply Support layering fixes.
Erik Pilkington via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 4 11:45:46 PDT 2018
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.
> 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/da7adb45/attachment.html>
More information about the llvm-commits
mailing list