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

[Lang Hames via Phabricator via llvm-commits]

lhames marked 13 inline comments as done.
lhames added a comment.

Thanks Dave! I've got a patch that addresses your feedback that I will post from the bus in a minute.



================
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() {
----------------
dblaikie wrote:
> 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)
Yeah — I like that idea.


================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:252-253
+  ~AtomGraph() {
+    for (auto *DA : DefinedAtoms)
+      DA->~DefinedAtom();
+  }
----------------
dblaikie wrote:
> Any chance this container could be one of unique_ptr? I guess DenseSet doesn't support that? Could it?
I had not thought of that. I can’t see any reason that would not work. I will try it out.


================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:454
+  DefinedAtomSet DefinedAtoms;
+  Optional<AddressToAtomMap> AddrToAtomCache;
+};
----------------
dblaikie wrote:
> 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)
Good call.


================
Comment at: lib/ExecutionEngine/JITLink/JITLink.cpp:57-59
+JITLinkMemoryManager::~JITLinkMemoryManager() {}
+
+JITLinkMemoryManager::Allocation::~Allocation() {}
----------------
dblaikie wrote:
> Could use
>   = default?
> here. No big deal, but might give the frontend extra info about the triviality, etc.
It had never occurred to me to use =default in an out-of-line definition before. :P

Good call.


================
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);
----------------
dblaikie wrote:
> 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)
Yep. The problem is that once you convert to shared_ptr you cannot convert back. I think I opted to keep this as a raw pointer for ease of conversion, on the assumption that we're switching to c++14 *reasonably* soon. 


================
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();
----------------
dblaikie wrote:
> might be easier written as:
> 
>   return std::tie(LHSP.getSectionOrdinal(), LHS->getOrdinal()) < std::tie(RHSP.getSectionOrdinal(), RHS->getOrdinal());
That's a neat trick, but once formatted for 80 columns I am not sure it's any more readable.


================
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";
----------------
dblaikie wrote:
> Indentation seems off here
Hrm. Clang format is doing awful things to this. Will hand-format.


================
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");
----------------
dblaikie wrote:
> 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 
There is! Thanks. :)


================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp:408
+        continue;
+      auto &Target = reinterpret_cast<DefinedAtom &>(R.getTarget());
+      if (Target.isDefined() && !LiveAtoms.count(&Target)) {
----------------
dblaikie wrote:
> 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?
Huh. This was a think-o: it should have been a static_cast. Fixed in the latest iteration of the dead-stripping code.  


================
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 "
----------------
dblaikie wrote:
> elses-after-returns can be removed, I think
Indeed.


================
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 {
----------------
dblaikie wrote:
> else-after-continues can be removed
Updated.


================
Comment at: unittests/ExecutionEngine/JITLink/JITLinkTestCommon.cpp:44-45
+JITLinkTestCommon::TestResources::getExternalsResolver() {
+  return [&](const DenseSet<StringRef> &Symbols,
+             jitlink::JITLinkAsyncLookupContinuation LookupContinuation) {
+    jitlink::AsyncLookupResult LookupResult;
----------------
dblaikie wrote:
> 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.
Yep -- 'this' would be better here. Fixed.


================
Comment at: unittests/ExecutionEngine/JITLink/JITLinkTestCommon.h:97-99
+      return support::endian::read<T, 1>(DA->getContent().data() + Offset,
+                                         G.getEndianness());
+    else
----------------
dblaikie wrote:
> 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
Interesting. I tend to prefer the if/else framing because it feels like the "success/failure" state on each path is more obvious. I only switch to early outs for Expected values if I think the indentation will make things really unreadable.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58704