<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
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 class="moz-txt-link-freetext" href="https://reviews.llvm.org/D95747">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 class="moz-txt-link-freetext" href="https://reviews.llvm.org/D97339">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 class="moz-cite-prefix">On 03/03/2021 04:57, David Blaikie
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAENS6EuLORoQDku+qft3=Vpnu4r9+=02SRrgyMirVSVt_A=fXA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">Seems one of the latest Orc changes ( <a
href="https://github.com/llvm/llvm-project/commit/99a6d003edbe97fcb94854547276ffad3382ec1d"
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 class="gmail-p1"
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
class="gmail-s1"
style="font-variant-ligatures:no-common-ligatures">$ grep -r
"void __jit_debug_register_code" llvm/</span></p>
<p class="gmail-p2"
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
class="gmail-s1"
style="font-variant-ligatures:no-common-ligatures">llvm/lib/ExecutionEngine/GDBRegistrationListener.cpp</span><span
class="gmail-s2"
style="font-variant-ligatures:no-common-ligatures;color:rgb(56,185,199)">:</span><span
class="gmail-s3"
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)"><span
class="gmail-Apple-converted-space"> </span>extern "C" </span><span
class="gmail-s4"
style="font-variant-ligatures:no-common-ligatures;color:rgb(202,51,35)"><b>void
__jit_debug_register_code</b></span><span class="gmail-s3"
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)">();</span></p>
<p class="gmail-p2"
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
class="gmail-s1"
style="font-variant-ligatures:no-common-ligatures">llvm/lib/ExecutionEngine/Orc/TargetProcess/JITLoaderGDB.cpp</span><span
class="gmail-s2"
style="font-variant-ligatures:no-common-ligatures;color:rgb(56,185,199)">:</span><span
class="gmail-s3"
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)">LLVM_ATTRIBUTE_NOINLINE
</span><span class="gmail-s4"
style="font-variant-ligatures:no-common-ligatures;color:rgb(202,51,35)"><b>void
__jit_debug_register_code</b></span><span class="gmail-s3"
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 class="gmail-p2"
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
class="gmail-s3"
style="font-variant-ligatures:no-common-ligatures;color:rgb(0,0,0)"><br>
<br>
</span></p>
</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>