[Lldb-commits] [PATCH] D66250: [JIT][Unwinder] Add Trampoline ObjectFile and UnwindPlan support for FCB

Fri Aug 23 00:36:46 PDT 2019

labath added a comment.

In D66250#1638338 <https://reviews.llvm.org/D66250#1638338>, @jasonmolenda wrote:

> In D66250#1633455 <https://reviews.llvm.org/D66250#1633455>, @clayborg wrote:
>
> > Why are we not just using ObjectFileJIT? I am guessing these breakpoint expressions create one of these by compiling the breakpoint expression and JIT'ing it just like any other expression. If this is the case, then why do we need to create a ObjectFileTrampoline? Seems like we could add .cfi directives to the assembly we use for the breakpoint condition function so that we can unwind out of it?
>
>
> I hadn't looked at ObjectFileJIT, we should look at whether we can that existing plugin.  I agree they're conceptually doing the same thing, but one difference is that the trampoline is only a function, it's not a mach-o binary that we've written into memory.  I think it would be possible to represent the trampoline unwind plan in eh_frame using a DW_OP_addr encoding for the caller's pc value, but we would have to manually write out the bytes of the eh_frame header, CIE and FDE entries, then the DW_OP_addr.  I think it's easier to stuff in an UnwindPlan with this unwind rule added manually.  Using the existing eh_frame format is laudable but I think it hand-writing the binary format into memory and then reading it back out again would be less maintainable IMO.

Setting the return address is one thing, but I'm not sure it stops there... For instance, I would expect you'll want to also set the "restore" rules for the GP registers too, so that their values come out "right" when you select the original frame in the debugger. We could do something special there too, but if we go the assembly route (which I believe has a lot of other advantages too), then all we'd need is to add some .cfi directives into the asm, and the eh_frame would fall out automatically..

================
Comment at: lldb/source/Target/ABI.cpp:10
 #include "lldb/Target/ABI.h"
+#include "Plugins/ObjectFile/Trampoline/ObjectFileTrampoline.h"
+#include "lldb/Core/Module.h"
----------------
jasonmolenda wrote:
> mib wrote:
> > labath wrote:
> > > If this is going to be something that is called directly from core lldb code, then it not a "plugin" by any stretch of imagination. I think we should put this file some place else.
> > Any suggestion on where to put it ?
> Maybe in source/Symbol along with the ObjectFile.cpp base class.  I agree with Pavel that this isn't a plugin; it subclasses ObjectFile so it probably has to implement the plugin methods, but it's not something that will ever be created by iterating through the active ObjectFile plugins or anything like that.
Yeah, either that, or next to the place that actually uses it (which would be `Target/ABI` right now). I think you have to implement some plugin methods (like `GetPluginName`, which is fine), but I am pretty sure you don't have the implement the CreateInstance stuff or register yourself with the plugin manager (as we will never be creating the objects this way).

That said, I'm still not convinced we really need this class in the first place. I would like to explore the possiblity of injecting the trampoline code as inline asm into the compiled expression. That way, it should automatically become a part of the resuling ObjectFileJIT, and it will solve a couple of other problems along the way (e.g., the "how to allocate memory in a target-independent manner" discussion in the other thread).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66250