[PATCH] D149541: [JITLink] Process null symbols

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h:552-553
+
+      auto &GSym = G->addAbsoluteSymbol(*Name, orc::ExecutorAddr(0), 0,
+                                        Linkage::Strong, Scope::Local, false);
+      setGraphSymbol(SymIndex, GSym);
----------------
jobnoorman wrote:
> lhames wrote:
> > Let's name these `__jitlink_ELF_SYM_UND_<st_value>` -- it'll make them easy to spot when debugging, and allow us to keep the invariant that all absolute symbols are named (which means that the change to `JITLink.cpp` would no longer be needed).
> > 
> > Side note: We should probably change `Symbol::hasName` so that empty symbols still count as valid names, but that's outside the scope of this patch.
> > Let's name these `__jitlink_ELF_SYM_UND_<st_value>` -- it'll make them easy to spot when debugging
> 
> Should we maybe go for `__jitlink_ELF_SYM_UND_<SymIndex>` to make sure the name is always unique? `st_value` will always be 0 in this case.
> 
> > and allow us to keep the invariant that all absolute symbols are named (which means that the change to `JITLink.cpp` would no longer be needed).
> 
> I might be missing something here but wouldn't this change still be needed? It handles printing edges with targets that are `!isDefined()` rather than `!hasName()`.
> Should we maybe go for __jitlink_ELF_SYM_UND_<SymIndex> to make sure the name is always unique? st_value will always be 0 in this case.

Yep -- that sounds good to me.

> I might be missing something here but wouldn't this change still be needed? It handles printing edges with targets that are !isDefined() rather than !hasName().

If all absolute symbols are named then they're handled under the `if (TargetSym.hasName())` condition, and all symbols are either named or have an associated Block (the `else` case).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149541/new/

https://reviews.llvm.org/D149541