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

[Lang Hames via Phabricator via llvm-commits]

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

New patch coming soon: more complete relocation support, weak symbol support, testing infrastructure, and a new llvm-jitlink tool (the jitlink counterpart to llvm-rtdyld).



================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:242
+               JITTargetAddress Addend) {
+    Edges.push_back(Edge(K, Offset, Target, Addend));
+  }
----------------
AlexDenisov wrote:
> There seems to be a discrepancy: within the `Edge`, the `Offset` and `Addend` are defined as `uint32_t` and `int64_t` respectively, but here they are initialized with `JITTargetAddress` which is `uint64_t`.
Good point. This will be fixed in the upcoming patch.


================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:414
+  }
+
+  /// Remove the given defined atom from the graph.
----------------
AlexDenisov wrote:
> Just curious, is it necessary to have both getAtomByName/findAtomByName publicly available (same for ..AtomByAddress)?
> As an LLVM APIs user, I always confused when I see such duality.
> Also, I think the getAtomByName/Address are dangerous: if you have assertions disabled (as it is in release builds), then a user will get undefined behavior if the element is not in the container.
Their purposes are slightly different: Sometimes you know from context that a certain address or symbol name is *definitely* present (usually because you would have error’d out earlier in the code if it wasn’t). In this case you can use the “get*” versions of the APIs and bypass the redundant error check. By contract you are only allowed to call these atoms if you are certain they will succeed.

Otherwise you should use the find* APIs, which do not assume that the query will succeed.

Of note: JITLink implementations should assume potentially malicious input: just because a well formed binary *should* have a particular symbol in it, does not mean that you are allow to assume that it will.


================
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";
----------------
sgraenitz wrote:
> lhames wrote:
> > dblaikie wrote:
> > > Indentation seems off here
> > Hrm. Clang format is doing awful things to this. Will hand-format.
> ```
> LLVM_DEBUG({
>   ...
> });
> ```
> usually works well
That's a handy tip. Thanks Stefan!


================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.h:35
+public:
+  JITLinkerBase(std::unique_ptr<MemoryBuffer> ObjBuffer,
+                JITLinkMemoryManager &MemMgr, JITLinkAsyncLookupFunction Lookup,
----------------
AlexDenisov wrote:
> Seeing a `unique_ptr` here, does it mean that a user of the API would have to give up the ownership of the object file?
> If that's the case, then, in my humble opinion, this API should be reconsidered: either take ownership from the very beginning or do not take it at all.
I am not quite sure what you mean here? Passing ownership into the constructor seems like taking ownership from the get-go?

In any case this has actually changed in the most recent version of the patch, which I will upload shortly: The new version takes ownership of a Context object instead, and its up to the client whether or not their context class owns the underlying buffer.


================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.h:54
+  //   1.1: Build atom graph
+  //   1.2: Identify root atoms and dead-strip graph
+  //   1.3: Run post-dead-strip passes
----------------
AlexDenisov wrote:
> How does dead-strip work? Is it possible to control what is stripped?
> Here is the use-case I'm concerned about:
> ```
> void *address = 0;
> void driver() {
>   void (*fun)() = address;
>   fun();
> }
> 
> void foo() {}
> void bar() {}
> ```
> There is a function that loads the address of another function from a global variable and calls it. Initially, the global address is zero, but it is changed by the JIT client, like this (pseudo-code):
> 
> ```
> address = jit.getAddress("foo")
> jit.callFunction("driver")
> address = jit.getAddress("bar")
> jit.callFunction("driver")
> ```
> Techically, the `foo` and `bar` are dead, but in fact they are used.
The details of dead-stripping have changed a little bit in the yet-to-be-posted patch, but the basic idea is that the target provides a sensible default “live-marker” pass which sets the live symbol roots.

Clients are definitely free to add passes to mark additional definitions live (or remove the live markers), and there is a convenience pass for tagging all atoms live.

The default MachO live-marker pass marks all external symbols live, so in your example ‘foo’ and ‘bar’ would be kept by default.


================
Comment at: lib/ExecutionEngine/JITLink/JITLink_MachO_x86_64.cpp:562
+    case PCRel32Minus4Anon: {
+      // FIXME: Implement.
+      break;
----------------
AlexDenisov wrote:
> Does it make sense to at least add some logs here, so that an end user will see what's going wrong with their code?
I should have used a different comment for these: they are “precommit fixmes”. I wanted to get the high level API design up for review while I worked on the details. The intention is that all relocations should be supported by the time this review is complete. :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58704