<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 2018-06-04 2:50 PM, David Blaikie
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAENS6Eux_ihcjTWyj4Re2=-EchUO5X99braL24Fa419XZyD9sg@mail.gmail.com">
<div dir="ltr"><br>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Jun 4, 2018 at 11:45 AM Erik Pilkington
<<a href="mailto:erik.pilkington@gmail.com"
moz-do-not-send="true">erik.pilkington@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<br>
<div class="m_-7251740090108470160moz-cite-prefix">On
2018-06-04 2:20 PM, David Blaikie wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">+Erik who seems to be someone interested
in this API<br>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Jun 4, 2018 at 11:11 AM
Chandler Carruth <<a
href="mailto:chandlerc@gmail.com"
target="_blank" moz-do-not-send="true">chandlerc@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr">On Mon, Jun 4, 2018 at 8:01 PM
David Blaikie via llvm-commits <<a
href="mailto:llvm-commits@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-commits@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
<div dir="ltr">(+Chandler since he had some
feedback on another layering change
anyway)<br>
<br>
</div>
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr">On Fri, Jun 1, 2018 at
2:31 PM Justin Bogner <<a
href="mailto:mail@justinbogner.com"
target="_blank"
moz-do-not-send="true">mail@justinbogner.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0 0 0
.8ex;border-left:1px #ccc
solid;padding-left:1ex">David Blaikie
via llvm-commits <<a
href="mailto:llvm-commits@lists.llvm.org"
target="_blank"
moz-do-not-send="true">llvm-commits@lists.llvm.org</a>>
writes:<br>
> Author: dblaikie<br>
> Date: Wed Mar 21 10:31:49 2018<br>
> New Revision: 328123<br>
><br>
> URL: <a
href="http://llvm.org/viewvc/llvm-project?rev=328123&view=rev"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://llvm.org/viewvc/llvm-project?rev=328123&view=rev</a><br>
> Log:<br>
> Reapply Support layering fixes.<br>
><br>
> Compiler.h is used by Demangle
(which Support depends on) - so sink
it<br>
> into Demangle to avoid a circular
dependency<br>
<br>
Sorry for seeing this a bit late, but
this fix seems very wrong to me.<br>
<br>
> DataTypes.h is used by llvm-c
(which Support depends on) - so sink
it<br>
> into llvm-c.<br>
><br>
> DataTypes.h could probably be
fixed the other way - making llvm-c
depend<br>
> on Support instead of Support
depending on llvm-c - if anyone feels<br>
> that's the better option, happy
to work with them on that.<br>
><br>
> I /think/ this'll address the
layering issues that previous attempts
to<br>
> commit this have triggered in the
Modules buildbot, but I haven't been<br>
> able to reproduce that build so
can't say for sure. If anyone's having<br>
> trouble with this - it might be
worth taking a look to see if there's
a<br>
> quick fix/something small I
missed rather than revert, but no
worries.<br>
><br>
> Copied:
llvm/trunk/include/llvm/Demangle/Compiler.h
(from r328122,
llvm/trunk/include/llvm/Support/Compiler.h)<br>
> URL: <a
href="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"
rel="noreferrer" target="_blank"
moz-do-not-send="true">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</a><br>
>
==============================================================================<br>
> ---
llvm/trunk/include/llvm/Support/Compiler.h
(original)<br>
> +++
llvm/trunk/include/llvm/Demangle/Compiler.h
Wed Mar 21 10:31:49 2018<br>
> @@ -1,4 +1,4 @@<br>
> -//===-- llvm/Support/Compiler.h
- Compiler abstraction support --*-
C++ -*-===//<br>
> +//===-- llvm/Demangle/Compiler.h
- Compiler abstraction support -*- C++
-*-===//<br>
> //<br>
> // The LLVM
Compiler Infrastructure<br>
> //<br>
><br>
> Modified:
llvm/trunk/include/llvm/Support/Compiler.h<br>
> URL: <a
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=328123&r1=328122&r2=328123&view=diff"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=328123&r1=328122&r2=328123&view=diff</a><br>
>
==============================================================================<br>
> ---
llvm/trunk/include/llvm/Support/Compiler.h
(original)<br>
> +++
llvm/trunk/include/llvm/Support/Compiler.h
Wed Mar 21 10:31:49 2018<br>
> @@ -7,500 +7,13 @@<br>
> //<br>
>
//===----------------------------------------------------------------------===//<br>
> //<br>
> -// This file defines several
macros, based on the current
compiler. This allows<br>
> -// use of compiler-specific
features in a way that remains
portable.<br>
> +// Due to layering constraints
(Support depends on Demangler) this is
a thin<br>
> +// wrapper around the
implementation that lives in llvm-c,
though most clients<br>
> +// can/should think of this as
being provided by Support for
simplicity (not<br>
> +// many clients are aware of
their dependency on Demangler/it's a
weird place to<br>
> +// own this - but didn't seem to
justify splitting Support into "lower
support"<br>
> +// and "upper support").<br>
<br>
Demangler isn't only a "weird place"
to own this, it's a complete break<br>
in abstraction. None of this stuff has
to do with demangler at all, so<br>
sinking it there because demangler
happens to depend in it is purely a<br>
hack. I feel like the fact that we
left a shim header in Compiler.h to<br>
paper over it is pretty strong
evidence that you agree here.<br>
</blockquote>
</div>
</div>
<div dir="ltr">
<div class="gmail_quote">
<div><br>
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.<br>
</div>
</div>
</div>
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0 0 0
.8ex;border-left:1px #ccc
solid;padding-left:1ex">I assume you
considered moving Signals.h (the only
thing that depends on<br>
Demangle) out of Support and rejected
that idea for one reason or<br>
another, but that's obviously one
option for doing this more cleanly.<br>
</blockquote>
</div>
</div>
<div dir="ltr">
<div class="gmail_quote">
<div><br>
Hadn't looked at that option, really,
though... <br>
<br>
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.<br>
<br>
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.<br>
</div>
</div>
</div>
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0 0 0
.8ex;border-left:1px #ccc
solid;padding-left:1ex">If that's
problematic for some reason, I think a
pretty strong argument<br>
could be made for moving Compiler.h to
Config/ - it's header only and<br>
really only exists to paper over
differences in the build
configuration.<br>
</blockquote>
</div>
</div>
<div dir="ltr">
<div class="gmail_quote">
<div><br>
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.<br>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
</div>
</div>
<div dir="ltr">
<div class="gmail_quote">
<div>I don't think this really helps.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</blockquote>
<div><br>
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?<br>
</div>
</div>
</div>
</blockquote>
</div>
<div text="#000000" bgcolor="#FFFFFF"> Yep, exactly. It is
in fact already used in libcxxabi, that repo has an
identical copy of the demangler here in LLVM.</div>
</blockquote>
<div><br>
If it's an identical copy, how's it dealing with the
Compiler.h dependency?<br>
</div>
</div>
</div>
</blockquote>
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.<br>
<blockquote type="cite"
cite="mid:CAENS6Eux_ihcjTWyj4Re2=-EchUO5X99braL24Fa419XZyD9sg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"> 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.<br>
<br>
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.</div>
<div text="#000000" bgcolor="#FFFFFF"><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>From Rafael's original commit:<br>
<br>
<div> My current plan is:</div>
<div> </div>
<div> Commit something like this</div>
<div> Change lld to use it</div>
<div> Change lldb to use it as the fallback</div>
<div> </div>
<div> Add a few #ifdefs so that exactly the
same file can be used in</div>
<div> libcxxabi to export
abi::__cxa_demangle.</div>
<div> </div>
<div> Once the fast demangler in lldb can
handle any names this</div>
<div> implementation can be replaced with it
and we will have the one true</div>
<div> demangler.<br>
<br>
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.<br>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<div text="#000000" bgcolor="#FFFFFF"> 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.</div>
<div text="#000000" bgcolor="#FFFFFF"><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>
<div>- Dave</div>
<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote"
style="margin:0 0 0
.8ex;border-left:1px #ccc
solid;padding-left:1ex"> <br>
<br>
> //<br>
>
//===----------------------------------------------------------------------===//<br>
> <br>
> -#ifndef LLVM_SUPPORT_COMPILER_H<br>
</blockquote>
</div>
</div>
</blockquote>
</div>
</div>
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br>
<a
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank"
moz-do-not-send="true">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
</body>
</html>