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

[Alex Denisov via Phabricator via llvm-commits]

AlexDenisov added a comment.

As a side note: it would be very helpful if you could add links to the specs you used to implement relocations.



================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:242
+               JITTargetAddress Addend) {
+    Edges.push_back(Edge(K, Offset, Target, Addend));
+  }
----------------
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`.


================
Comment at: include/llvm/ExecutionEngine/JITLink/JITLink.h:414
+  }
+
+  /// Remove the given defined atom from the graph.
----------------
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.


================
Comment at: lib/ExecutionEngine/JITLink/JITLinkGeneric.h:35
+public:
+  JITLinkerBase(std::unique_ptr<MemoryBuffer> ObjBuffer,
+                JITLinkMemoryManager &MemMgr, JITLinkAsyncLookupFunction Lookup,
----------------
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.


================
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
----------------
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.


================
Comment at: lib/ExecutionEngine/JITLink/JITLink_MachO_x86_64.cpp:562
+    case PCRel32Minus4Anon: {
+      // FIXME: Implement.
+      break;
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58704