[PATCH] D149541: [JITLink] Process null symbols
Job Noorman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 15 03:45:50 PDT 2023
jobnoorman marked 2 inline comments as done.
jobnoorman 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:
> > 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).
> > > 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.
>
> Just noticed there's a practical issue here: `Symbol` stores its name as a `StringRef` so I don't think there's a safe way to construct a name on the fly.
>
> So I see two options:
> - Call the symbol `__jitlink_ELF_SYM_NULL` (or something like that) and `assert` there is only one;
> - Update `Symbol` to own its name in a `std::string`.
>
> > > 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).
>
> Ah, of course! Will remove these changes in the next update.
>
>
> Just noticed there's a practical issue here: `Symbol` stores its name as a `StringRef` so I don't think there's a safe way to construct a name on the fly.
I figured out we can use `LinkGraph::allocateContent` for this so all issues should be resolved now. I'll wait for a final LGTM for these last changes before landing this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149541/new/
https://reviews.llvm.org/D149541
More information about the llvm-commits
mailing list