Page MenuHomePhabricator

[ORC] Prototype ORC library reorg + RPC based TargetProcessControl.
ClosedPublic

Authored by lhames on Oct 25 2020, 11:07 PM.

Details

Summary

Introduce OrcRPC based TargetProcessControlImplementation.

Diff Detail

Event Timeline

lhames created this revision.Oct 25 2020, 11:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2020, 11:07 PM
lhames requested review of this revision.Oct 25 2020, 11:07 PM

Great that there is so fast progress! Haven't reviewed in detail (yet), but it looks like there are no conceptual changes compared to the proposal we know? A few notes inline.

llvm/include/llvm/ExecutionEngine/Orc/Shared/TargetProcessControlTypes.h
111

I am getting compile warnings here too (Apple clang-1100.0.33.12). On my machine it says "designated initializers are a C99 feature" :-) One more in OrcRPCTPCServer.h

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
625

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
627

Maybe this could be:

errs() << "unable to launch out-of-process executor: "
       << ExecutorPath.get() << "\n";
dblaikie added inline comments.Oct 26 2020, 2:31 PM
llvm/include/llvm/ExecutionEngine/Orc/RPC/RPCSerialization.h
593–594

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")

599

I'd usually write this as "if (O)" fwiw (especially with the deref on the next line - seems clear enough what's being tested), but all good either way.

llvm/include/llvm/ExecutionEngine/Orc/Shared/TargetProcessControlTypes.h
30–32

Could potentially skip these and use braced init whenever you want to create an object of this type and initialize the members in one go? (similarly below/for other similar types) Or perhaps there are difficult issues with deduced types and such?

llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h
131

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?

261

Would it be simpler to write this as a local stateless lambda (they can be converted to raw function pointers anyway) - I see it's used in two places, but the code is so simple (maybe the type parameters are complicated?) might not be pulling its weight as a named function?

530

StringRef, perhaps?

560–563

What's this about?

llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp
29–51

What's this scope for specifically? (I can't quickly spot which dtor you're trying to run before the return statement/make_unique)

75

Maybe add a makeMutableArrayRef that takes an array reference and deduces the size from that, rather than needing to specify it explicitly?

llvm/lib/ExecutionEngine/Orc/TargetProcessControl.cpp
102

Not sure if you want to, but just in case: You can make this actually a function type (Rather than a function pointer type): "using MainTy = int(int, char *[]);" and then have the usage be "jitTargetAddressToFunction<MainTy*>" - not sure if that's better, but an option in case in interests.

llvm/lib/ExecutionEngine/OrcTargetProcess/TargetExecutionUtils.cpp
39 ↗(On Diff #300578)

Don't see much !! in LLVM - maybe (ProgramName != nullptr) instead?

llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp
52

Not possible to support, or "not implemented yet"?

91

Generally favor the implicit conversion (StringRef Arg1 = argv[1]) - means only implicit conversions can occur, making the code easier to read (because you don't have to be worried that some explicit conversion is occurring - I find this particularly important with unique_ptrs (unique_ptr<T> u(v()) is "oh, I need to check if this is taking ownership of a raw pointer" versus unique_ptr<T> u = v() "oh, great, it's returning unique_ptr by value")

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
604–605

These get a bit hard to read - would it be worth having two separate names for the PipeFD arrays, maybe even some constants for the 0/1, which end is which sort of thing?

sgraenitz added inline comments.Oct 27 2020, 7:48 AM
llvm/tools/llvm-jitlink/llvm-jitlink.cpp
822

I think this only works for isOSBinFormatMachO() at the moment. Current symptom when targeting ELF:

bin/llvm-jitlink: Missing definition for llvm_orc_registerEHFrameSectionWrapper
lhames updated this revision to Diff 302059.Oct 30 2020, 8:44 PM
lhames marked an inline comment as done.

Address review comments.

lhames updated this revision to Diff 302060.Oct 30 2020, 8:45 PM

Fix diff to include original changes.

lhames added inline comments.Oct 30 2020, 8:47 PM
llvm/include/llvm/ExecutionEngine/Orc/RPC/RPCSerialization.h
593–594

This is a specialization and the base case is a class, so I think the specialization should be (and perhaps must be) a class?

llvm/include/llvm/ExecutionEngine/Orc/Shared/TargetProcessControlTypes.h
30–32

Yeah. I'm still conservative with my braced inits as I've seen them fail on MSVC builders but don't have a good sense of what those compilers do/don't support. Ideally this can be removed, but we can try fixing that selectively after the patch lands so that any failures due to it are clear.

111

Thanks for catching this. It should be fixed in the latest patch.

llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h
131

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.

261

Oh -- I didn't realize that stateless lambdas were convertible to raw function types. That's neat!

I think we keep it though: As you noticed -- Naming the argument types ends up making the lambda quite long. I guess we could use auto for this once we move to C++17?

530

StringRefs aren't safely convertible to a c_str -- We'd need to copy to a std::string anyway.

560–563

It's the MachO leading-underscore demangling, but it would be better written as if (*Sym != '_') ...;. Updated.

llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp
29–51

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.

75

Good idea, but I think I'd introduce that in a separate patch.

llvm/lib/ExecutionEngine/OrcTargetProcess/TargetExecutionUtils.cpp
39 ↗(On Diff #300578)
% grep -iR '!!' include lib  | wc -l
   155
% grep -iR '!= nullptr' include lib  | wc -l
   658

Less common, but common enough -- I think this one can stay (or we should come up with a style guide point to dictate the preferred style).

llvm/tools/llvm-jitlink/llvm-jitlink-executor/llvm-jitlink-executor.cpp
52

Not implemented yet. We just need a windows implementation. I've added a FIXME.

91

Fixed.

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
625

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.

627

Good call. Fixed.

I'm curious to get your take on the library breakdown and corresponding headers too. We now have:

include/llvm/ExecutionEngine/Orc               -> lib/ExecutionEngine/Orc              (libLLVMOrc)
include/llvm/ExecutionEngine/Orc/Shared        -> lib/ExecutionEngine/OrcShared        (libLLVMOrcShared)
include/llvm/ExecutionEngine/Orc/TargetProcess -> lib/ExecutionEngine/OrcTargetProcess (libLLVMOrcTargetProcess)

This achieves the initial goal that executors should not need to link Orc (instead they only need to link OrcTargetProcess and OrcShared).

The library dependencies are:

Orc -> OrcShared, RuntimeDyld, JITLink, IR, Support
OrcTargetProcess -> OrcShared

Side note: My ideal solution would be to (a) move OrcJIT out into its own top-level directory, and (2) break libLLVMOrc up into quite a few libraries: ORC Core, ObjLinking, RTDyldLinking, IR, LLJIT, Shared, and TargetProcess at least. Unfortunately I don't think this is worth pursuing at the moment: Any real Orc use-case will need one of RTDyldObjectLinkingLayer or ObjectLinkingLayer, both of which depend and libObject, which depends on everything Orc does today anyway. However if/when libObject gets refactored to eliminate these dependencies we should revisit this.

dblaikie added inline comments.Oct 31 2020, 12:34 PM
llvm/include/llvm/ExecutionEngine/Orc/RPC/RPCSerialization.h
593–594

Oh fair, probably. Though traits classes tend to be all public members, so maybe the base template could be a struct too.

llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h
131

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.

261

I think we keep it though: As you noticed -- Naming the argument types ends up making the lambda quite long.

Fair

I guess we could use auto for this once we move to C++17?

I was going to suggest that the conversion isn't available then (because the call operator becomes a template), but apparently it does work - neat! and it does template argument deduction to instantiate a specific call operator when the conversion is performed. So, yeah, one day.

560–563

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?

llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp
29–51

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.

llvm/lib/ExecutionEngine/OrcTargetProcess/TargetExecutionUtils.cpp
39 ↗(On Diff #300578)

Fair enough (FWIW the numbers skew a bit - not all of the !!s are for pointers, some for integer values (so would be comparable to != 0), Optional, etc - and, amusingly two dozen are emphatic comments or assert messages like "Case index out of range!" - always 3 in those cases, it seems, I didn't see any emphases)

sgraenitz added a comment.EditedNov 3 2020, 4:36 AM

I'm curious to get your take on the library breakdown and corresponding headers too.

There's a number of includes still pointing to #include "llvm/ExecutionEngine/Orc/TargetProcess/XY". Otherwise, yes LGTM.

This achieves the initial goal that executors should not need to link Orc

+1 very reasonable!

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
625

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'm curious to get your take on the library breakdown and corresponding headers too.

There's a number of includes still pointing to #include "llvm/ExecutionEngine/Orc/TargetProcess/XY". Otherwise, yes LGTM.

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

Two issues when trying to build llvm-jitlink-executor with the libs separated:

llvm/lib/ExecutionEngine/OrcTargetProcess/LLVMBuild.txt
21 ↗(On Diff #302060)

OrcShared is required here now

llvm/lib/ExecutionEngine/OrcTargetProcess/RegisterEHFrames.cpp
72 ↗(On Diff #302060)

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

lhames marked 2 inline comments as done.Nov 10 2020, 5:39 PM

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.

llvm/include/llvm/ExecutionEngine/Orc/RPC/RPCSerialization.h
593–594

In this case the traits have some data members that are used as a cache. I think it makes sense to keep those private.

llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h
131

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.

560–563

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.

llvm/lib/ExecutionEngine/Orc/TPCEHFrameRegistrar.cpp
29–51

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.

llvm/lib/ExecutionEngine/OrcTargetProcess/RegisterEHFrames.cpp
72 ↗(On Diff #302060)

Good catch -- thanks!

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
625

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.

lhames updated this revision to Diff 304363.Nov 10 2020, 5:40 PM
lhames marked an inline comment as done.

Address review comments.

Please let me know if you have any other thoughts / comments, otherwise I'm hoping to commit this by the end of the week so we can move on to the next step: Full support for initializers in remote processes.

Last issue: EH-frame registration isn't ready for ELF right? That would be great to fix (see the unticked inline comment).
I can offer to check the mangling prefix details once this had landed.

llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h
560–563

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.

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
625

Ok I see, this makes it independent from the current working directory. Great!

822

This note here is still open. EH-frame registration didn't work for me on Linux. With the proposed fix it does.

lhames updated this revision to Diff 304661.Nov 11 2020, 2:45 PM

Make executable symbols in llvm-jitlink-executor visible.

sgraenitz accepted this revision.Nov 12 2020, 6:37 AM

Ok, I grabbed your latest version and checked again for the EH frame registration on Linux. This works very well now! Thanks

This revision is now accepted and ready to land.Nov 12 2020, 6:37 AM
lhames added inline comments.Nov 13 2020, 2:40 PM
llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h
560–563

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.

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
822

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?

lhames closed this revision.Nov 13 2020, 2:41 PM

Committed in 1d0676b54c4.

sgraenitz added inline comments.Nov 23 2020, 8:27 AM
llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h
560–563

Yes, you are right, there won't be official support and so it shouldn't impact ongoing upstream development. Still, these use-cases exist downstream and for me it's worth it to keep them working (somehow). Eventually, I could solve this specific issue with a customized TPCDynamicLibrarySearchGenerator -- the other way around this time: https://gist.github.com/weliveindetail/7add5f366abe5f75da4476aa3f27d6bf

llvm/tools/llvm-jitlink/llvm-jitlink.cpp
822

Yes, I did a clean rebuild of the executor with symbols exported and that fixed it. Thanks!