[PATCH] D157859: [ORC] Introduce RedirectionManager interface and implementation using JITLink.

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h:59
+                                   ObjectLinkingLayer &ObjLinkingLayer,
+                                   JITDylib &JD, Error &Err)
+      : ES(ES), ObjLinkingLayer(ObjLinkingLayer), JD(JD),
----------------
sunho wrote:
> lhames wrote:
> > Can we remove the `JD` argument here? The `ResourceTracker` argument to `createRedirectableSymbols` tells us the target `JITDylib` already, and this would allow us to have one global `JITLinkRediretableSymbolManager`, rather than one per JITDylib.
> We need this JD since we are going to generate a stub pool inside this JD. Alternative is manage stub pool per JD within single manager, how does this sound to you?
You can get the target JITDylib from the tracker, so the aim should be to have one manager for all JITDylibs. In this case the `createRedirectableSymbols` method should probably be changed to a set of functions:

```
virtual Error createRedirectableSymbols(ResourceTrackerSP RT, const SymbolAddrMap &InitialDests) = 0;

Error createRedirectableSymbol(JITDylib &JD, const SymbolAddrMap &InitialDests) {
  return createRedirectableSymbol(JD.getDefaultResourceTracker(), InitialDests);
}

Error createRedirectableSymbol(
    ResourceTracker RT, SymbolStringPtr Symbol, ExecutorSymbolDef InitialDest) {
  return createRedirectableSymbols(std::move(RT), {{Symbol, InitialDest}});
}

Error createRedirectableSymbol(
    JITDylib &JD, SymbolStringPtr Symbol, ExecutorSymbolDef InitialDest) {
  return createRedirectableSymbol(JD.getDefaultResourceTracker(), std::move(Symbol), InitialDest);
}
```


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h:100-101
+
+  BumpPtrAllocator BAlloc;
+  StringSaver StringPool{BAlloc};
+  std::mutex Mutex;
----------------
sunho wrote:
> sunho wrote:
> > lhames wrote:
> > > Why a separate `StringSaver` here, rather than using the `SymbolStringPool`?
> > We need to get actual string from SymbolStringPtr. Is there a way to do this?
> Oh actually this was needed since JITLink isn't based on SymbolStringPtr yet. We still need to save the string cause JITLink assumes all the StringRefs are properly owned by external object like symbol string table.
You can get a `StringRef` for a `SymbolStringPtr` using the dereference operator, and that should be safe to use in a `LinkGraph`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157859