-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[llvm-c] Expose debug support for LLJIT in Orc C-API bindings #73257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[llvm-c] Expose debug support for LLJIT in Orc C-API bindings #73257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few open questions
83cebca
to
786088e
Compare
db37381
to
74340b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short discussion on Discord suggested that it's better to add this for LLJIT
specifically. So, here is the update.
@@ -39,7 +39,7 @@ Error enableDebuggerSupport(LLJIT &J) { | |||
if (!Registrar) | |||
return Registrar.takeError(); | |||
ObjLinkingLayer->addPlugin(std::make_unique<DebugObjectManagerPlugin>( | |||
ES, std::move(*Registrar), true, true)); | |||
ES, std::move(*Registrar), false, true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the RequireDebugSections
flag. Without this change the test won't pass, because the bitcode for the sum()
example has no debug info.
This change may cause some overhead, but this shouldn't be a surprise in the context of debugging. And it may avoid headaches for users: Not remembering to generate debug info is a common problem for JITed code. With this change users get at least function names, like in release builds. Without it, the debugger has zero info.
Alternatively, we could expose it in the C-API and wire it up in the internal function here. See my second comment for details.
} | ||
} | ||
|
||
#if defined(_AIX) or not defined(__ELF__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the test doesn't pass for MachO, because the sum()
example has no debug info and it's a hard-coded requirement in the MachO plugin:
https://github.com/llvm/llvm-project/blob/0424546ed4a75708/llvm/lib/ExecutionEngine/Orc/Debugging/DebuggerSupportPlugin.cpp#L401
@lhames For the reasons described above, it might be interesting to expose the RequireDebugSections
flag in the C-API. What do you think? Would you wire it up in the MachO plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MachO plugin just bails out early if there's no debug info, right? I think that's reasonable behavior. Could we add some debug info to the test case? Hopefully the IR-level debug info is the same for both platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MachO plugin just bails out early if there's no debug info, right? I think that's reasonable behavior.
Not sure. I think it's confusing for users. People are used to have symbol names in non-debug builds, so they can at least set breakpoints on their functions. With the bail out behavior here they have nothing at all and it's not immediately obvious why. It's one of too many things that can go wrong in JITed code debugging.
Could we add some debug info to the test case? Hopefully the IR-level debug info is the same for both platforms?
Anyway, yes that's probably the best solution. I guess minimal debug info will be pretty portable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MachO plugin just bails out early if there's no debug info, right? I think that's reasonable behavior.
Not sure. I think it's confusing for users. People are used to have symbol names in non-debug builds, so they can at least set breakpoints on their functions. With the bail out behavior here they have nothing at all and it's not immediately obvious why. It's one of too many things that can go wrong in JITed code debugging.
Regarding that: I'm working on some lighter-weight symbolication solutions that we could use to enable breakpoints (and symbolicated backtraces) for JIT'd code, even in the case of a crashed executor. So hopefully this situation will be temporary. :)
bf47a4b
to
589fc25
Compare
589fc25
to
f7ae608
Compare
|
f7ae608
to
09de016
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks very much @weliveindetail!
…o RuntimeDyld Fix test failure reported from buildbot clang-s390x-linux-lnt after #73257 OrcCAPITest.cpp:562: Debugger support requires JITLink
Allow C-API users to debug their JITed code. The new API function
LLVMOrcObjectLayerRegisterPluginJITLoaderGDB()
adds the required plugin to provide GDB JIT Interface support. This initial patch covers the ELF use-case by installing the DebugObjectManagerPlugin. It should be easy to add support for other object formats later on.