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

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:559-562
+#ifdef __APPLE__
+        if (*Sym != '\0')
+          ++Sym;
+#endif
----------------
sgraenitz wrote:
> lhames wrote:
> > 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.
> We shouldn't ignore use cases like ELF on `__APPLE__` and ELF on `_WIN32`. Here symbols don't come with the appropriate mangling prefix, which means that the lookup logic becomes a little more complicated:
> * `printf` should get prefixed and resolve to `_printf`
> * my own function `foo` shouldn't and instead resolve to plain `foo`
> 
> I assume that foo will be resolved on the jitlink side, because the symbol is defined in user code. Thus, the request for it won't make it to the jitlink-executor side in the first place. OTOH printf will be looked up here on the jitlink-executor side, so adding the mangling prefix is correct.
> 
> There is a similar logic in SelfTargetProcessControl, where we add the GlobalManglingPrefix unconditionally if the process triple's default object format is MachO.
> 
> Maybe this is beyond the scope of this initial version, so I'd propose to investigate it in more detail and come up with a follow-up patch.
I've always thought of ELF on Darwin/Win32 as a hack to paper over the fact that RuntimeDyldMachO and RuntimeDyldCOFF were missing features (especially debugger registration). Under that view the right answer is to implement those features in JITLink and then always use an object format that matches the platform (MachO for Darwin, ELF for Linux, COFF for Win32). That way there's never any question about the ABI or mangling lining up with the surrounding process.

I *do* occasionally use cross-format execution for testing purposes (e.g. llvm-jitlinking an ELF object to test JITLink ELF support), but I'm not sure whether we should make that kind of use case officially supported. If we do decide that it should be supported that's a bigger discussion that should happen outside this review, and should not block it.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:817
 
-  if (!NoExec && !TT.isOSWindows())
+  if (!NoExec && !this->TPC->getTargetTriple().isOSWindows())
     ObjLayer.addPlugin(std::make_unique<EHFrameRegistrationPlugin>(
----------------
sgraenitz wrote:
> sgraenitz wrote:
> > I think this only works for `isOSBinFormatMachO()` at the moment. Current symptom when targeting ELF:
> > ```
> > bin/llvm-jitlink: Missing definition for llvm_orc_registerEHFrameSectionWrapper
> > ```
> > 
> This note here is still open. EH-frame registration didn't work for me on Linux. With the proposed fix it does.
eh-frame registration isn't fully hooked up for JITLink/ELF as far as I know, but this should still work. I suspect the problem was that executable symbols weren't exported from llvm-jitlink-executor. I've fixed that in the latest patch -- could you let me know if it fixes your issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90132