<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
Yes, got it and I also wonder if there's a better low hanging
solution.<br>
<br>
The underlying question that I wanted to point out was: Would the
escape to Support be a one-time solution for JITSymbol or will we
see more of the same soon. GDB JIT interface seems to be the next
candidate, but OTOH it's quite a special case again. I will have a
look at OProfile and Perf implementations for RuntimeDyld to make a
better estimation.<br>
<br>
I guess you'd prefer us to act on it soon now? What time frame are
you having in mind? Is next week acceptable? (I am more or less ooo
for the rest of the week.)<br>
<br>
<blockquote type="cite">@Peter, @Nico: I've noticed your post-review
comments. I will have a look tonight (~10h from now).</blockquote>
These should be fixed now. Thanks again for reporting your issues
and sorry for the inconvenience.<br>
<br>
<div class="moz-cite-prefix">On 03/03/2021 19:40, David Blaikie
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAENS6EspQ-V+8SP=Qcmsu0Sv=MiB=kR2ZffEFCx6Zs0QRkidHg@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">To clarify, I'm not sure JITSymbol should be in
llvmSupport, but that it's the only place it can be right now
that would be correct. Maybe there's some other layering changes
(not necessarily introducing a new library, but possibly
changing other dependency edges, etc) - but maybe there's
already some JIT Stuff in llvmSupport and that's where it should
go. It's a simple enough header/wouldn't come at a great cost to
include it in Support.</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Mar 3, 2021 at 1:51 AM
Stefan Gränitz <<a href="mailto:stefan.graenitz@gmail.com"
moz-do-not-send="true">stefan.graenitz@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div> Hi David<br>
<br>
Thanks for the details. Yes, the layering issue is something
we should take care of soon. It also makes trouble for the
modules build (see: <a
href="https://reviews.llvm.org/D95747" target="_blank"
moz-do-not-send="true">https://reviews.llvm.org/D95747</a>).<br>
<br>
I think we should split up the JITSymbol.h and move
JITTargetAddress into OrcShared. What remains would be the
JITSymbol class. Moving this one to Support sounds like a
nice solution to me.<br>
<br>
However, we have a similar situation with the GDB JIT
interface declarations. They should have their own header,
yes, but where would we put it? Support too? Not sure about
it. Having the definition only in OrcTargetProcess would be
acceptable IMHO. The only alternative seems to be an
entirely new library (as discussed in the review <a
href="https://reviews.llvm.org/D97339" target="_blank"
moz-do-not-send="true">https://reviews.llvm.org/D97339</a>).<br>
<br>
What do you think?<br>
<br>
@Peter, @Nico: I've noticed your post-review comments. I
will have a look tonight (~10h from now).<br>
<br>
Best,<br>
Stefan<br>
<br>
<div>On 03/03/2021 04:57, David Blaikie wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Seems one of the latest Orc changes ( <a
href="https://github.com/llvm/llvm-project/commit/99a6d003edbe97fcb94854547276ffad3382ec1d"
target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/99a6d003edbe97fcb94854547276ffad3382ec1d</a>
) while not itself changing/breaking the layering in
LLVM's own build, it has revealed some pre-existing
problems with the layering that we'd worked around at
Google in a way that isn't viable after this recent
change.<br>
<br>
One immediate/easily observed issue:
lib/ExecutionEngine's CMakeLists.txt says it depends on
OrcTargetProcess, but OrcTargetProcess includes
lib/ExecutionEngine/JITSymbol.h<br>
<br>
The only common dependency for all the uses of
JITSymbol.h seems to be llvm/Support (ie: without
introducing new dependencies or new libraries,
JITSymbol.h would need to be moved to llvm/Support to
fix this particular dependency cycle/issue)<br>
<br>
We do have a bunch of other workarounds for Orc layering
in the Google internal build system too - so perhaps I
can enumerate some/all of the issues here, as it might
be best to take a holistic approach to fixing these
issues.<br>
<br>
Let's see what I can document/figure out... <br>
<br>
ExecutionEngine/Orc -> ExecutionEngine<br>
ExecutionEngine/Interpreter -> ExecutionEngine<br>
ExecutionEngine/RuntimeDyld -> ExecutionEngine<br>
ExecutionEngine/IntelJITEvents -> ExecutionEngine<br>
ExecutionEngine/OProfileJIT -> ExecutionEngine<br>
ExecutionEngine/PerfJITEvents -> ExecutionEngine<br>
ExecutionEngine/MCJIT -> ExecutionEngine<br>
<br>
And there's actually no #includes in ExecutionEngine
that reference those libraries, so that's pretty good.<br>
<br>
It is this CMakeLists.txt dependency from
ExecutionEngine to OrcTargetProcess. Which happens
without a #include:<br>
<br>
<p
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span
style="font-variant-ligatures:no-common-ligatures">$
grep -r "void __jit_debug_register_code" llvm/</span></p>
<p
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(219,39,218)"><span
style="font-variant-ligatures:no-common-ligatures">llvm/lib/ExecutionEngine/GDBRegistrationListener.cpp</span><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(56,185,199)">:</span><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)"><span>
</span>extern "C" </span><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(202,51,35)"><b>void
__jit_debug_register_code</b></span><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)">();</span></p>
<p
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(219,39,218)"><span
style="font-variant-ligatures:no-common-ligatures">llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderGDB.cpp</span><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(56,185,199)">:</span><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)">LLVM_ATTRIBUTE_NOINLINE
</span><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(202,51,35)"><b>void
__jit_debug_register_code</b></span><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)">()
{</span></p>
<br>
Would be better if this wasn't declared arbitrarily
(instead, if it was declared in a header and defined as
usual, the circular dependence would be more clear, I
think?) - but either way, the circular dependency needs
to be fixed.<br>
<br>
- Dave<br>
<p
style="margin:0px;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(219,39,218)"><span
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)"><br>
<br>
</span></p>
</div>
</blockquote>
<pre cols="72">--
<a href="https://flowcrypt.com/pub/stefan.graenitz@gmail.com" target="_blank" moz-do-not-send="true">https://flowcrypt.com/pub/stefan.graenitz@gmail.com</a></pre>
</div>
</blockquote>
</div>
</blockquote>
<pre class="moz-signature" cols="72">--
<a class="moz-txt-link-freetext" href="https://flowcrypt.com/pub/stefan.graenitz@gmail.com">https://flowcrypt.com/pub/stefan.graenitz@gmail.com</a></pre>
</body>
</html>