<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jun 4, 2018 at 8:46 PM Erik Pilkington via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">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 text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<br>
<div class="m_1453370472255763381moz-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">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">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">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">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">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">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">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. 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></blockquote><div><br></div><div>This at least seems like a significantly better short or medium term state. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
</div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>