[PATCH] D74300: [ORC] Add generic initializer/deinitializer support.

[Lang Hames via Phabricator via llvm-commits]

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

Thanks very much for the review Stefan!

I'll apply your suggestions and update the patch shortly.

The part of this patch that I think still needs the most thought is the new state: Initialized.

Initially I thought I would want this to prevent symbols from being looked up (in the default case) until they were initialized. In practice though I'm not sure it's needed: dlsym interposes for JIT'd code can't use it: You're allowed to dlsym a symbol in initializers (i.e. before the initializer for that symbol is complete), and for clients they can just wait until their initialization call returns before looking up any symbols that depend on initialization and then they're safe.

So the question, since the code already been written, is: Can we think of any use for gating on initializers having run, or should we rip this part back out for now to simplify the patch? The benefit to removing it would be to eliminate some new APIs (e.g. Platform::notifySymbolsReady) and reduce some session locking since the platform won't need to re-lock the session to move the symbols to the initialized state.

Let me know if you have any thoughts on this. I probably shouldn't hold the patch up too much longer either way though: we can fix this in-tree after this lands easily enough.



================
Comment at: llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp:155
+
+    // Promote globals and define new materializing symbols.
+    SmallVector<GlobalValue *, 16> PromotedGlobals;
----------------
sgraenitz wrote:
> Is GV promotion here related to the initializers change? Is it always necessary or can we skip it in the whole module partitioning case?
I think it would be possible to skip it in the whole module case -- I will give that a try.


================
Comment at: llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp:168
+      else if (GV.getName().startswith("\01L"))
+        GV.setName("__" + GV.getName().substr(1) + "." + Twine(++NextPromoId));
+      else if (GV.hasLocalLinkage())
----------------
sgraenitz wrote:
> This is an ObjC special case right? Can we have a comment for it?
It's not ObjC specific, but I think it is hardcoding something that it shouldn't be:

The \01 bit comes from https://llvm.org/docs/LangRef.html#identifier : Symbols with that prefix should not be mangled. This is fine.

The 'L' that follows is meant to capture the assembler private prefix, but we should probably do that with DL.getPrivatePrefix rather than hardcoding it.

This issue is orthogonal to this patch so I'll fix it on trunk.


================
Comment at: llvm/lib/ExecutionEngine/Orc/Core.cpp:1130
       // Move symbol to the emitted state.
+
       assert(SymEntry.getState() == SymbolState::Resolved &&
----------------
sgraenitz wrote:
> Unintended newline I guess.
Fixed. Thanks!


================
Comment at: llvm/lib/ExecutionEngine/Orc/Core.cpp:1958
+              CompoundErr =
+                  joinErrors(std::move(CompoundErr), Result.takeError());
+          }
----------------
sgraenitz wrote:
> The typical ErrorList vs. reportError() case as discussed in D72176. Isn't it a similar situation as with the OnComplete handler in ReExportsMaterializationUnit::materialize()? That one reports the incoming error.
> 
> While looking at this, should it be the handler's responsibility at all to think about incoming errors? Is there a deeper reason for this? Otherwise, couldn't the NotifyComplete callback bring a Optional<SymbolMap> instead of an Expected<SymbolMap>? IIUC AsynchronousSymbolQuery has two distinct code paths for success vs. error already: handleComplete() and handleFailed(). The latter could call NotifyComplete(None) and handle the error independently. However, it's not directly connected to this review.
> The typical ErrorList vs. reportError() case as discussed in D72176. Isn't it a similar situation as with the OnComplete handler in ReExportsMaterializationUnit::materialize()? That one reports the incoming error.

Yep. This was just laziness on my part while getting the prototype up and running. This should probably just log the errors and return a new "couldn't find some initializer symbols" error.

> While looking at this, should it be the handler's responsibility at all to think about incoming errors? Is there a deeper reason for this? Otherwise, couldn't the NotifyComplete callback bring a Optional<SymbolMap> instead of an Expected<SymbolMap>?

I considered using Optional rather than Expected when I was designing the lookup API, but in the end decided that it was better to force clients to explicitly acknowledge a failure result: In some of my prototypes that used Optional I ran into issues when I accidentally continued past a failure result.


================
Comment at: llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:37
+
+/// Adds helper function decls and wrapper functions tha call the helper with
+/// some additional prefix arguments.
----------------
sgraenitz wrote:
> Typo: tha -> that
> This is how far I got.
Fixed. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74300