Page MenuHomePhabricator

[lli] Add --jit-linker command line argument
ClosedPublic

Authored by sgraenitz on Feb 23 2021, 3:02 PM.

Details

Summary

The argument value determines the dynamic linker to use (default, rtdyld or jitlink). The JITLink implementation only supports in-process JITing for now.

Diff Detail

Event Timeline

sgraenitz created this revision.Feb 23 2021, 3:02 PM
sgraenitz requested review of this revision.Feb 23 2021, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2021, 3:02 PM

The major issue I faced with this patch is the clash of the GDB JIT interface symbols __jit_debug_register_code and __jit_debug_descriptor. These are defined in both libraries, ExecutionEngine and OrcTargetProcess, and I didn't find a "nice" way to deduplicate them. My current approach turns the definitions in ExecutionEngine into externals and adds OrcTargetProcess as link component to ExecutionEngine. Maybe there is another way to fix this?

lhames accepted this revision.Feb 24 2021, 12:36 AM

This patch adds a --dyld flag

Could this be changed to -jit-linker? It's verbose, but I think -dyld is too likely to cause confusion, especially when we already have -dlopen (which really does call the native dynamic linker). I'm open to shorter names too, it's -dyld that I really want to avoid.

The major issue I faced with this patch is the clash of the GDB JIT interface symbols __jit_debug_register_code and __jit_debug_descriptor. These are defined in both libraries, ExecutionEngine and OrcTargetProcess, and I didn't find a "nice" way to deduplicate them. My current approach turns the definitions in ExecutionEngine into externals and adds OrcTargetProcess as link component to ExecutionEngine. Maybe there is another way to fix this?

We could create an entirely new library for this, but that feels like overkill. I think OrcTargetProcess seems reasonable. I think that the new dependence diagram would look like:

ORC -> { JITLink, OrcShared }
OrcTargetProcess -> { OrcShared }
ExecutionEngine -> { RuntimeDyld, OrcTargetProcess }
This revision is now accepted and ready to land.Feb 24 2021, 12:36 AM

This patch adds a --dyld flag

Could this be changed to -jit-linker? It's verbose, but I think -dyld is too likely to cause confusion

I don't really like the duplication in -jit-linker=jitlink, but well it's acceptable.

We could create an entirely new library for this, but that feels like overkill. I think OrcTargetProcess seems reasonable. I think that the new dependence diagram would look like:

ORC -> { JITLink, OrcShared }
OrcTargetProcess -> { OrcShared }
ExecutionEngine -> { RuntimeDyld, OrcTargetProcess }

Yes, OrcTargetProcess is so small that the overhead in ExecutionEngine will hardly be noticeable.
So let's keep it like that, including the FIXME. I guess we can move the symbol definitions from OrcTargetProcess the the Orc runtime one day.

I guess we can move the symbol definitions from OrcTargetProcess the the Orc runtime one day.

That was my first thought too, but then I realized that for the debugger to see these symbols in the runtime (which is linked in via the JIT) we'd need to have a different solution for registering debug info for JIT'd code, at which point this would be redundant. :)

sgraenitz updated this revision to Diff 326959.Feb 28 2021, 6:07 AM

Rename command line flag: dyld -> jit-linker

sgraenitz edited the summary of this revision. (Show Details)Feb 28 2021, 6:07 AM
sgraenitz updated this revision to Diff 326960.EditedFeb 28 2021, 6:11 AM

Remove the debug support plugin for the moment

sgraenitz retitled this revision from [lli] Add JITLink in-process debugging support to [lli] Add --jit-linker command line argument.Feb 28 2021, 6:15 AM
sgraenitz edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Mar 2 2021, 6:08 AM
This revision was automatically updated to reflect the committed changes.

My bots are failing a few tests on non-mac: http://45.33.8.238/linux/40624/step_12.txt

I think that's due to a build config problem in my end but it's not obvious to me what's wrong. In case you immediately see it, I'm grateful for hints. If not, I'll figure it out eventually :)

Looks like it's not findig the mac mangled gdb symbol name -- what's supposed to provide that?

My bots are failing a few tests on non-mac: http://45.33.8.238/linux/40624/step_12.txt

I think that's due to a build config problem in my end but it's not obvious to me what's wrong. In case you immediately see it, I'm grateful for hints. If not, I'll figure it out eventually :)

Looks like it's not findig the mac mangled gdb symbol name -- what's supposed to provide that?

(Sorry, this question would've been a better fit on D97335 :) Still happy for pointers, though.)

(Sorry, this question would've been a better fit on D97335 :) Still happy for pointers, though.)

I think I figured out the linux bit. SelfTargetProcessControl::lookupSymbols strips GlobalManglingPrefix and the test actually looks up llvm_orc_registerJITLoaderGDBWrapper without underscore. For dlsym() to find this in dlopen(NULL, ...), llvm-jitlink needs to be linked with -rdynamic. I hope it's similarly straightforward on Windows...

I think I figured out the linux bit. SelfTargetProcessControl::lookupSymbols strips GlobalManglingPrefix and the test actually looks up llvm_orc_registerJITLoaderGDBWrapper without underscore. For dlsym() to find this in dlopen(NULL, ...), llvm-jitlink needs to be linked with -rdynamic. I hope it's similarly straightforward on Windows...

And I found a Windows box too. Looks like the test fails on Windows with a normal build config too. Windows doesn't support dlsym()ing (well, GetProcAddress()ing) symbols from the main executable, only from dynamic libraries. So the whole approach seems Windows-incompatible. I'll try to revert some of the bits here to unbreak stuff.

(It'd be nice to land changes spread out over time, so that if a change breaks something it's easier to revert....)

klausler added a subscriber: klausler.EditedMar 2 2021, 12:11 PM

I'm getting a Release mode shared library build failure on x86-64 Linux w/ gcc 9.3.0:

/opt/binutils/2.32/bin/ld: CMakeFiles/lli.dir/lli.cpp.o: undefined reference to symbol '_ZN4llvm7jitlink20JITLinkMemoryManagerD2Ev'
/opt/binutils/2.32/bin/ld: /home/pklausler/llvm-project/build/x86/gcc/9.3.0/Release/shared/./lib/libLLVMJITLink.so.13git: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
tools/lli/CMakeFiles/lli.dir/build.make:139: recipe for target 'bin/lli' failed

Not sure yet whether it's due to this commit or something else.

UPDATE: Reverting this commit doesn't fix the build; will look elsewhere.

I still have not found the specific commit that has broken the shared library Release build on x86-64 Linux with the error that I quoted above. (That build remains broken for me, though.)

@klausler Thanks for reporting this. lli was missing JITLink as a link component in CMake. I fixed it with https://reviews.llvm.org/rG295ea050ad594b8868b6dd944824ba9a16273f91. Sorry for the inconvenience.

klausler removed a subscriber: klausler.Mar 3 2021, 3:28 PM