[PATCH] D132345: [JITLink] Add support for symbol aliases.

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

@benlangmuir Any take on the alternative proposal -- make alias metadata available, but leave it to clients to act on?

In that case the alias infrastructure and iterators from this patch would remain, but we'd drop the MovePolicy and automated updates for aliases.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:434
+  /// any aliases in the graph.
+  AssertNoAliases
+};
----------------
benlangmuir wrote:
> Is there a reason that `AliasMovePolicy` does not also have this option?
AssertNoAliases was introduced early. I think that by the end of the patch I'd decided that it was a bad idea, but forgot to take it back out.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:918
+
+  using AliasInfoMap = DenseMap<const Symbol *, AliasInfo>;
+
----------------
benlangmuir wrote:
> I'm worried about this type storing `AliasInfo` by value.  You're holding references and returning pointers to the values in several places, which I worry is going to blow up if you accidentally insert into the map at the wrong time.   I haven't found any actual UAF in the below code, but there are several footguns here -- `AliasInfo &AI` that is lexically in scope after a mutation of the map (ie. only working because nothing touches `AI` again) -- also you're relying on the fact `DenseMap::erase` never shrinks.
You're right -- you can definitely imagine an iteration over aliases being used to insert new aliases.

Aliases are expected to be rare -- we can probably allocate entries in the graph's BumpPtrAllocator and hold pointers to them in the map.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:1459
+  iterator_range<alias_iterator> aliases(Symbol &S) const {
+    if (!LLVM_LIKELY(S.HasAliasInfo))
+      return iterator_range<alias_iterator>(alias_iterator(), alias_iterator());
----------------
benlangmuir wrote:
> Why is it "LLVM_LIKELY" that there are aliases?  I would have expected the opposite here.  Same for `aliases_begin` and `aliases_end` above.
I think the logic was "If you were writing general-case code for symbols that _might_ be aliased then you would have used `alias_tree`, so a call to `aliases` suggests knowledge that a symbol is aliased".

I don't really think this needs to be optimized though -- I'll just remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132345