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

Mon Aug 26 13:39:04 PDT 2019

mib marked 4 inline comments as done.
mib added a comment.

In D66249#1644791 <https://reviews.llvm.org/D66249#1644791>, @labath wrote:

> In D66249#1643594 <https://reviews.llvm.org/D66249#1643594>, @mib wrote:
>
> > In D66249#1642316 <https://reviews.llvm.org/D66249#1642316>, @labath wrote:
> >
> > > A bunch more comments from me. So far, I've only tried to highlight the most obvious problems or style issues.
> > >
> > >   It's not clear to me whether this is still prototype code or not... If it isn't, I'll have a bunch more.. :)
> >
> >
> > This is prototype code that I'm trying to upstream to build upon.
> >
> > From the comments I already gathered, I started thinking of a better implementation of this feature (maybe v2).
> >  There are still some parts that need do be done, such as resolving the variables on optimized code or patching dynamically the copied instructions.
> >
> > But, I'd like to have a baseline that is upstream to start working on these parts.
> >
> > **All feedback is welcome !** :)
>
>
> Ok, I see where you're going. I think we have a different interpretation of the word "prototype" . When I say "prototype code", I mean: the changes that I've made while experimenting, to try out whether a particular approach is feasible, and which I'm sharing with other people to get early feedback about the direction I am going in. These changes may include various shortcuts or hacks, which I've made to speed up the development of this prototype, but I understand that these will have to be removed before the final code is submitted (in fact, the final patch may often be a near-complete reimplementation of the prototype) . In this light, the phrase "prototype which I am trying to upstream" is a bit of an oxymoron since by the time it is upstreamed, the code should no longer be a prototype. It does not mean that the code should support every possible use case right from the start, but it should follow best coding, design and testing practices, regardless of any "experimental", "prototype" or other label.
>
> With the eventual upstreaming goal in mind, I've tried to make a more thorough pass over this patch. The comments range from minute style issues like using `(void)` in function declarations, which can be trivially fixed, to layering concerns that may require more serious engineering work. Overall, my biggest concern is that there is no proper encapsulation in this patch. Despite being advertised as "fully extensible" this patch bakes in a lot of knowledge in very generic pieces of code. For instance, the `BreakpointInjectedSite` class is riddled with OS (`mach_vm_allocate`), architecture (`rbp`) and language (`Builtin.int_trap()`) specific knowledge -- it shouldn't need to know about none of those. I think this is the biggest issue that needs to be resolved before this patchset is ready for upstreaming. I've also made a number of other suggestions (here and in other threads) about possible alternative courses implementations. These are somewhat subjective and so I am not expecting you to implement all of them. However, I would:
>  a) expect some sort of a reply saying why you chose to do one thing or the other
>  b) strongly encourage you to try them out because I believe they will make fixing some of the other issues easier (for instance, switching to allocating the memory on the stack would mean that we can completely avoid the "how to allocate memory on different OSs" discussion, and just deal with architecture specifics, since subtracting from the stack pointer works the same way on every OS).
>
> Also, this talk of a `v2` makes me worried that this code will become deprecated/unmaintained the moment it hits the repository. Lldb has some bad experience with fire-and-forget features, and the prospect of this makes me want to be even stricter about any bad patterns I notice in the code. IIUC, you are now working on a different take on this feature, which kind of means that you yourself are not sure about whether this is the best approach here. I understand the need for comparison, but why does one of these things need to be in the main repo for you to be able to compare them?

Thanks for taking the time to review this patch **thoroughly**! I really appreciate it :) !

I totally get you're meaning of //prototype//, but I used this word to reflect what I was able to come up with during my internship.

I'm already working on a new implementation that would make all the architecture/OS/language specific bits more generic, by doing a stack allocation for the `$__lldb_arg` and having the trap in the trampoline directly (avoiding using the languages builtins).
The updated patch should arrive by the end of the week.

I'm also planning to generate the trampoline using inline assembly (`asm`)  with `.cfi` instructions as you suggested, but this will come later, on a different patch.

Concerning your comments, I'm 100% for these changes :) I'll fix everything on the next update.

================
Comment at: lldb/include/lldb/Target/ABI.h:165
+
+  virtual size_t GetJumpSize() { return 0; }
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66249