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

[Ben Langmuir via Phabricator via llvm-commits]

benlangmuir added a comment.

Does the `SubTree` v. `WholeTree` terminology have precedent anywhere? I personally didn't find it suggestive enough to guess what it would mean, I think because I mostly think about aliases of regular symbols not aliases of aliases (ie. the "tree" part).  Just a suggestion: how about `SubAliases` and `AllAliases`.  If you disagree, feel free to stick with the current names, I don't feel that strongly about it.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:434
+  /// any aliases in the graph.
+  AssertNoAliases
+};
----------------
Is there a reason that `AliasMovePolicy` does not also have this option?


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:918
+
+  using AliasInfoMap = DenseMap<const Symbol *, AliasInfo>;
+
----------------
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.


================
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());
----------------
Why is it "LLVM_LIKELY" that there are aliases?  I would have expected the opposite here.  Same for `aliases_begin` and `aliases_end` above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132345