[PATCH] D90132: [ORC] Prototype ORC library reorg + RPC based TargetProcessControl.

[Lang Hames via Phabricator via llvm-commits]

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

In D90132#2370956 <https://reviews.llvm.org/D90132#2370956>, @sgraenitz wrote:

> Ok, actually the headers don't follow the breakdown. Why is that?

I just followed the existing layout that OrcError used, but I don't think there's any good reason for it. I've moved OrcShared and OrcTargetProcess to Orc/Shared and Orc/TargetProcess subdirectories in the latest patch.



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/RPC/RPCSerialization.h:593-594
+template <typename ChannelT, typename T>
+class SerializationTraits<ChannelT, Optional<T>> {
+public:
+  /// Serialize an Optional<T>.
----------------
dblaikie wrote:
> lhames wrote:
> > dblaikie wrote:
> > > I'd probably write anything with all-public members as "struct" (don't think we have style policy on this - and certainly some folks/styles reserve "struct" for "type with only member variables/no invariants")
> > This is a specialization and the base case is a class, so I think the specialization should be (and perhaps must be) a class? 
> Oh fair, probably. Though traits classes tend to be all public members, so maybe the base template could be a struct too.
In this case the traits have some data members that are used as a cache. I think it makes sense to keep those private.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:130
+public:
+  static const char *getName() { return "ReserveMemResultElement"; }
+};
----------------
dblaikie wrote:
> lhames wrote:
> > dblaikie wrote:
> > > Probably use StringRef for these return results - so the length is computed off the string literal, rather than a StringRef/other string conversion happening later and requiring a null terminator search at that point?
> > I think we want to stick with char*: We use the strings for diagnostics and RPC-handshakes, but neither of those are performance sensitive. The only performance sensitive use case for these strings is as densemap keys,  where using StringRefs would cause us to use value comparisons instead of pointer comparisons.
> Could have a pointer identity DenseMap trait for StringRefs I guess... *goes to look* slightly surprised we don't have one already. My motivation's mostly the same (but less serious) as raw 'new' - const char*s make me give the code a second look in ways that StringRefs don't.
Interesting. I don't think of StringRefs that way: They're very much a view API to me (especially given the lack of guaranteed null termination), and relying on pointer equality between StringRefs (other than for optimization) is more surprising than relying on pointer equality between char*s.

I'm going to stick with `char*` in this patch, but it's worth discussing further. If we decide to change it then the entire RPCTypeName scheme (and associated RPC classes )need to be updated anyway, which his a job for another patch.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:559-562
+#ifdef __APPLE__
+        if (*Sym != '\0')
+          ++Sym;
+#endif
----------------
dblaikie wrote:
> lhames wrote:
> > dblaikie wrote:
> > > What's this about?
> > It's the MachO leading-underscore demangling, but it would be better written as `if (*Sym != '_') ...;`. Updated. 
> Hmm - it strikes me as odd because it's using a compile-time feature rather than say, an API query to test whether the binary format is MachO. I guess that's because of the context of this being on the machine where the code will execute and thus __APPLE__ is synonymous with MachO at that point? If it were possible to rely on some other property like the type of the object file where the symbol came from, that might make it a bit easier to test portably or less error-prone in some weird future where someone might be testing different object formats, etc?
To be clear: This isn't the regular JIT lookup path, this is an RPC wrapper for dlsym in the target process. There's no object file to look at. We're taking a JIT symbol string (always linker mangled) and trying to look it up using dlsym, which expects a C symbol name. If the target process is MachO, which is determinable at compile time and synonymous with `__APPLE__`, then we need to apply MachO "demangling", i.e. dropping the leading underscore. A runtime check wouldn't add anything here.


================
Comment at: llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp:28-50
+  {
+    std::string RegisterWrapperName, DeregisterWrapperName;
+    if (TPC.getTargetTriple().isOSBinFormatMachO()) {
+      RegisterWrapperName += '_';
+      DeregisterWrapperName += '_';
+    }
+    RegisterWrapperName += "llvm_orc_registerEHFrameSectionWrapper";
----------------
dblaikie wrote:
> lhames wrote:
> > dblaikie wrote:
> > > What's this scope for specifically? (I can't quickly spot which dtor you're trying to run before the return statement/make_unique)
> > I often scope parts of functions that perform a specific, isolated task (in this case finding (De)RegisterEHFrameWrapperFnAddr. It acts as a porto-function that's easy to yank out if it grows / becomes complex enough, and it ensures that I don't accidentally re-use temporaries that support that task outside the scope.
> Ah - I'd be inclined to push back a bit there, given it's a bit uncommon & not sure about other readers, but for me leads me to wonder what novel details are in this code such that a dtor needs to run before the return statement/making this code harder to read. But your call.
Fair, and looking at it again this is a short function so there's less value to this kind of scoping. I think this may actually have been yanked out of a larger function earlier and I just failed to drop the braces. I've removed them in the latest patch.


================
Comment at: llvm/lib/ExecutionEngine/OrcTargetProcess/RegisterEHFrames.cpp:72
+  return make_error<JITLinkError>("could not register eh-frame: "
+                                  "__register_frame function not found");
+}
----------------
sgraenitz wrote:
> We cannot use `JITLinkError` in the target process, because it's defined in JITLink. Could it be a `StringError` for the moment? (One more case below.)
Good catch -- thanks!


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:625
+    char *const Args[] = {ExecutorPath.get(), FDSpecifier.get(), nullptr};
+    int RC = execvp(ExecutorPath.get(), Args);
+    if (RC != 0) {
----------------
sgraenitz wrote:
> lhames wrote:
> > sgraenitz wrote:
> > > What is the advantage of using `execvp` over `execv`? (The latter is what lli did.) I think the way it is now, the default "llvm-jitlink-executor" will only be found if the directory is in PATH.
> > > 
> > > Changing this to `execv` I can run a hello world example with the default executor setting once I built both binaries. With `execvp` the default fails, but this still works:
> > > ```
> > > ./llvm-jitlink -oop-executor=./llvm-jitlink-executor ../hello_world.o
> > > ```
> > I like execvp, since it means that lookup for the executor program is going to follow the same paths as the lookup for llvm-jitlink itself. Maybe the best option would be to use execvp but default to a full path to llvm-jitlink-executor?
> > 
> > I'd be inclined to update lli to match this logic.
> > 
> > I don't have strong feelings about this though. If most people find execv preferable then I am happy to change it. 
> I have no strong opinion on it either, except that the default setting should work. If we keep execvp, then the default value for `OOPExecutor` should be `./llvm-jitlink-executor` right?
I think it should default to the same relative path as llvm-jitlink, since that will be the standard build/install path. I've switched to this in the latest version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90132