[PATCH] D58704: Initial (incomplete) implementation of JITLink - A replacement for RuntimeDyld.

Fri Mar 1 17:09:44 PST 2019

dblaikie added a comment.

(food for thought, not a "this must be answered now" sort of thing, but "Generic atom-graph data structure and algorithms " does sound a bit like the original LLD design, which didn't turn out to be a great fit (at least the way it ended up) for ELF, at least - any ideas on what might be done differently to avoid the same situation here when adding ELF support down the line? (my naive perspective is that the mismatch was around slicing up ELF sections into MachO-like atoms on symbol boundaries was the mismatch - rather than adding support for an atom with multiple symbols (that may not be at the start of the atom) and then having a one atom to one ELF section mapping))

================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:223-224
+
+  edge_iterator edges_begin() { return Edges.begin(); }
+  edge_iterator edges_end() { return Edges.end(); }
+  iterator_range<edge_iterator> edges() {
----------------
Might be able to get away without having specific begin/end accessors - everyone can use the range accessor instead?

(similar feedback for other range accessors in this patch)

================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:252-253
+  ~AtomGraph() {
+    for (auto *DA : DefinedAtoms)
+      DA->~DefinedAtom();
+  }
----------------
Any chance this container could be one of unique_ptr? I guess DenseSet doesn't support that? Could it?

================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:454
+  DefinedAtomSet DefinedAtoms;
+  Optional<AddressToAtomMap> AddrToAtomCache;
+};
----------------
Should this be 'mutable' (since it's modified from a const member function like "getAddrToAtomMap() const")?

Also, I'd tend to implement the non-const version of a function in terms of the const version, rather than the other way around. That way if the object is actually const, the code doesn't hit UB.

(currently calling getAddrToAtomMap() on an actually const object would be UB)

================
Comment at: lib/ExecutionEngine/JITLink/JITLink.cpp:57-59
+JITLinkMemoryManager::~JITLinkMemoryManager() {}
+
+JITLinkMemoryManager::Allocation::~Allocation() {}
----------------
Could use
  = default?
here. No big deal, but might give the frontend extra info about the triviality, etc.

================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:230-234
+  auto *UnownedSelf = Self.release();
+  auto Phase2Continuation =
+      [UnownedSelf](
+          Expected<DenseMap<StringRef, JITEvaluatedSymbol>> LookupResult) {
+        std::unique_ptr<JITLinkerBase> Self(UnownedSelf);
----------------
This leaks or double frees if this lambda is either not called, or called more than once.

Perhaps it'd be worth using shared_ptr here for simplicity/robustness?

(fixme would still apply and worth migrating to a unique_ptr captured by move at that point)

================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:282-288
+                auto &LHSP = LHS->getSection();
+                auto &RHSP = RHS->getSection();
+                if (LHSP.getSectionOrdinal() < RHSP.getSectionOrdinal())
+                  return true;
+                if (LHSP.getSectionOrdinal() > RHSP.getSectionOrdinal())
+                  return false;
+                return LHS->getOrdinal() < RHS->getOrdinal();
----------------
might be easier written as:

  return std::tie(LHSP.getSectionOrdinal(), LHS->getOrdinal()) < std::tie(RHSP.getSectionOrdinal(), RHS->getOrdinal());

================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:293-297
+    for (auto &KV : Layout) {
+    dbgs() << "  Segment "
+           << static_cast<sys::Memory::ProtectionFlags>(KV.first) << ":\n";
+    for (auto *DA : KV.second)
+      dbgs() << "    " << *DA << "\n";
----------------
Indentation seems off here

================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:381-382
+
+  assert(!std::any_of(G->external_atoms_begin(), G->external_atoms_end(),
+                      [](Atom *A) { return !A->isResolved(); }) &&
+         "All atoms should have been resolved by this point");
----------------
Seem to be a few negatives in here, maybe:

  assert(std::all_of(... isResolved()); ...

would be a more direct expression/match the phrasing of the assertion text below?

Oh, and there's probably llvm::all_of that takes a range directly rather than needing to decompose into begin/end 

================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:388
+  assert(G && "Graph is not set yet");
+  G->dump(dbgs(), [this](Edge::Kind K) { return getEdgeKindName(K); });
+}
----------------
I tend to advocate for [&] whenever a lambda is used within the scope it's defined (ie: doesn't leak out via a std::function or similar) - it's just another scope & seems fine for it to have access to all of the names in the outer scope without having to explicitly opt in, etc.

================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:408
+        continue;
+      auto &Target = reinterpret_cast<DefinedAtom &>(R.getTarget());
+      if (Target.isDefined() && !LiveAtoms.count(&Target)) {
----------------
reinterpret_casts (especially of references, I think - I'd worry about potentially turning a non-reference/pointer/thing into a reference with such a cast) make me a bit twitchy - any chance of wrapping the reinterpret_casting up in something more type safe at both ends?

================
Comment at: lib/ExecutionEngine/JITLink/JITLinkMachO_x86_64.cpp:46-82
+  switch (R) {
+  case Branch32:
+    return "Branch32";
+  case Pointer64:
+    return "Pointer64";
+  case Pointer64Anon:
+    return "Pointer64Anon";
----------------
Worth a small .def file or local macro to reduce the duplication?

  #define MY_CASE(n) case n: return #n
  MY_CASE(NegDelta32);
  MY_CASE(NegDelta64);

etc... 

================
Comment at: lib/ExecutionEngine/JITLink/JITLinkMachO_x86_64.cpp:232-238
+      else if (&AtomToFix == &*ToAtom)
+        return PairRelocInfo {
+                 SubRI.r_length == 3 ? NegDelta64 : NegDelta32,
+                 &*FromAtom,
+                 FixupValue - (FixupAddress - ToAtom->getAddress()) };
+      else
+        return make_error<JITLinkError>("SUBTRACTOR relocation must fix up "
----------------
elses-after-returns can be removed, I think

================
Comment at: lib/ExecutionEngine/JITLink/MachOAtomGraphBuilder.cpp:121-127
+      continue;
+    } else if (Flags & object::SymbolRef::SF_Absolute) {
+      LLVM_DEBUG(dbgs() << "Adding absolute \"" << *Name << "\" addr: "
+                        << format("0x%016" PRIx64, *Addr) << "\n");
+      G->addAbsoluteAtom(*Name, *Addr);
+      continue;
+    } else {
----------------
else-after-continues can be removed

================
Comment at: unittests/ExecutionEngine/JITLink/JITLinkTestCommon.cpp:44-45
+JITLinkTestCommon::TestResources::getExternalsResolver() {
+  return [&](const DenseSet<StringRef> &Symbols,
+             jitlink::JITLinkAsyncLookupContinuation LookupContinuation) {
+    jitlink::AsyncLookupResult LookupResult;
----------------
Using '&' capture for a lambda that escapes the current scope makes me a bit twitchy - would it be worth enumerating the things being captured here? I guess they're all members, so it's capturing 'this' by value, but still - I'd be more comfortable with that spelled out.

================
Comment at: unittests/ExecutionEngine/JITLink/JITLinkTestCommon.h:97-99
+      return support::endian::read<T, 1>(DA->getContent().data() + Offset,
+                                         G.getEndianness());
+    else
----------------
else-after-return

Ah, I see, scoping because of "DA" - I'd probably move DA out of the condition and early return for the error case instead:

  auto DA = G.find...;
  if (!DA)
    return DA.takeError();
  return support::endian::read<>(...);

Reduces indentation of the main path

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58704