[Lldb-commits] [PATCH] D66249: [JIT][Breakpoint] Add "BreakpointInjectedSite" and FCB Trampoline

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ok, it sounds we're in agreement then. I'l looking forward to the updated patches. :)



================
Comment at: lldb/include/lldb/Target/ABI.h:165
+
+  virtual size_t GetJumpSize() { return 0; }
+
----------------
mib wrote:
> labath wrote:
> > The point of my earlier suggestion to use ArrayRef is that you don't need to have a separate `XXXSize` function. `ArrayRef<uint8_t>` will encode both the size and the contents.
> > 
> > That said, I'm not convinced that this is a good api anyway. I think it would be extremely hard to make anything on top of these functions that would *not* be target specific (e.g. how would you handle the fact that on arm the jump target is expressed as a relative offset to the address of the jump instruction **+ 8**?).
> > 
> > I think it would be better to just have the ABI plugin just return the fully constructed trampoline jump sequence (say by giving it the final address of the sequence, and the address it should jump to) instead of somebody trying to create a target-independent assembler on top of it.
> I got rid of those getters, except `GetJumpSize`, which is used in the `Process::SaveInstructions` method to copy enough instructions to write the jump instruction.
Ok, I see. In that case, I'd recommend tweaking the interfaces so that this isn't needed also. For instance, the ABI plugin could save the instructions itself, or it could just return the bytes it wants to write, and have the Process class (or somebody) do the actual writing. The advantages of that would be:
- the knowledge about the size of the jump instruction (sequence) is encoded in a single place
- the abi class is free to dynamically choose the best jump sequence based on the target address (e.g., on thumb, the range for a single jump instruction is ridiculously small, and even the +/-2GB range in intel may not always be enough)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66249