Page MenuHomePhabricator

[JIT][Breakpoint] Add "BreakpointInjectedSite" and FCB Trampoline
Needs ReviewPublic

Authored by mib on Aug 14 2019, 1:16 PM.

Details

Reviewers
jdoerfert
Group Reviewers
Restricted Project
Summary

BreakpointInjectedSite is a sub-class of the BreakpointSite class.
Its main purpose is to handle Fast Conditional Breakpoint setup.

In the case of Fast Conditional Breakpoints, the process will copy
enough instructions to a new memory space and branch to it.

Then it creates a BreakpointInjectedSite that will build the condition
expression, by fetching the condition from each BreakpointLocation
and depending on the architecture and the source language, inject
a trap instruction that will be caught by LLDB at runtime. To do so, the
trap address is resolved and added to LLDB’s BreakpointSiteList.
The BreakpointInjectedSite also initialize the $__lldb_arg structure
for the conditional expression using Debug Info.
Right now, it only works with un-optimized (“-O0”) code, on x86_64.

After creating the Fast Conditional Breakpoint, the process will set up the
FCB Trampoline for the current ABI. Since the JIT-ed code might modify the
registers state, we need to save the current context, run the JIT-ed code
and the copied instruction, then restore the context and branch back.

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Event Timeline

mib created this revision.Aug 14 2019, 1:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
mib changed the visibility from "Public (No Login Required)" to "mib (Med Ismail Bennani)".Aug 14 2019, 1:17 PM
mib edited reviewers, added: Restricted Project; removed: jdoerfert.Aug 14 2019, 1:29 PM
mib changed the visibility from "mib (Med Ismail Bennani)" to "Custom Policy".
mib removed a reviewer: Restricted Project.Aug 14 2019, 1:50 PM
mib changed the visibility from "Custom Policy" to "Public (No Login Required)".
mib added a reviewer: Restricted Project.Aug 14 2019, 1:56 PM
jdoerfert resigned from this revision.Aug 14 2019, 1:57 PM
labath added a subscriber: labath.Aug 15 2019, 4:21 AM

I didn't go into all the details of this patch, as I am guessing this will still go through a number of revisions, but I did make a couple notes of things that caught my eye while glancing over it.

lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h
51

BreakpointSite is already shared_ptr-enabled. Why this? I don't think that's how you're supposed to use this class.

lldb/include/lldb/Breakpoint/BreakpointSite.h
54–56

This is weird. So, in OO terminology, BreakpointInjectedSite "is-a" BreakpointSite, but if you ask llvm::isa<BreakpointSite>(injected_site), it will return false?

lldb/include/lldb/Expression/ExpressionVariable.h
210

What is the purpose of this AdaptedIterable, and why is it better than just returning say ArrayRef<lldb::ExpressionVariableSP ?

lldb/source/Breakpoint/BreakpointInjectedSite.cpp
227

This isn't a proper format string. You can find their general description here http://llvm.org/docs/ProgrammersManual.html#formatting-strings-the-formatv-function, and the specifics are generally next to the definition of the actual format provider (e.g. https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/FormatProviders.h#L102). I find it very helpful to look at their unit tests too: https://github.com/llvm-mirror/llvm/blob/master/unittests/Support/FormatVariadicTest.cpp.

lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.h
126–128

replace with ArrayRef<uint8_t> GetJumpOpcode(); or something similar.

aprantl added inline comments.Aug 16 2019, 8:39 AM
lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h
36

This comment assumes that the reader knows what injected breakpoints are; can you either link to the file where this is explained or add a one-paragraph description of what this means here?

38

Is this still recognized by doxygen if there is an empty line before the declaration?

39

can this be an inner class of BreakpointInjectedSite?

59

Doxygen comment explaining the \param eters?

65

Please add comments to all non-obvious functions.

91

... and members :-)

lldb/include/lldb/Target/ABI.h
143

Comment explaining what this is supposed to do?

Is there always *exactly* one opcode that fits this requirement, and is an Opcode always < 64 bits on all ABIs?

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py
181

how does this test ensure that the breakpoint was actually injected and that it didn't fall back to a regular breakpoint?

lldb/source/Breakpoint/BreakpointInjectedSite.cpp
31

we usually write this just as BuildConditionExpression()

48

Should this return an error instead of logging and returning false?

342

This seems awfully specific. Shouldn't this be target-dependent, and is there no pre-existing table in LLDB that we could derive this from?

383

This should be in an x86_64-specific subclass, I think?

444

This seems unnecessarily slow. Perhaps use an llvm::Twine?

lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
225

. at the end :-)

273

We don't typically use end-of-line comments in LLVM and prefer full sentences.

mib marked 9 inline comments as done.Aug 20 2019, 4:12 PM
mib updated this revision to Diff 216747.Thu, Aug 22, 5:45 PM
mib marked 16 inline comments as done.

Add doxygen documentation.

Move ArgumentMetadata struct inside BreakpointInjectedSite.

Change string format for logging.

Make $__lldb_create_args_struct generation architecture-independent.

lldb/include/lldb/Breakpoint/BreakpointSite.h
54–56

No, it returns true.

lldb/include/lldb/Expression/ExpressionVariable.h
210

Given a class with a container, AdaptedIterable will generate the proper iterator accessor for this class.

In this case, it's pretty convenient to do:

for (auto var : expr_vars)
{ }

instead of:

for (auto var : expr_vars.GetContainer())
{ }

It doesn't require to have a getter on the container to iterate over it.

Most of LLDB container classes are built upon std containers (vector, map ...).
I could change it to ArrayRef<lldb::ExpressionVariableSP>, but I don't see why using it would be better, + it might break things (e.g. ArrayRef doesn't implement some of the std containers method like erase() or clear()).

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py
181

The breakpoint doesn't get injected if we fallback to a regular breakpoint.
And I check the state of the breakpoint on the command above:

self.assertFalse(location.GetInjectCondition(), VALID_BREAKPOINT_LOCATION)
lldb/source/Breakpoint/BreakpointInjectedSite.cpp
48

Error is handled on the upper function, at the process level.

444

As discussed, using llvm::Twine doesn't fit the implementation. Since this part of the code will probably change in the future, to support more DWARF Expression, I'll leave it as is for now, and try to come up with a more efficient approach in the coming patches.

mib updated this revision to Diff 216749.Thu, Aug 22, 5:59 PM
This comment was removed by mib.

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.. :)

lldb/include/lldb/Breakpoint/BreakpointSite.h
54–56

Then does llvm::isa<BreakpointInjectedSite>(injected_site) return false? They can't both return true, given the current implementations (but they should, if the dynamic type of injected_site really is BreakpointInjectedSite...

lldb/include/lldb/Expression/ExpressionVariable.h
210

You are adding this function in this very patch. It can't "break" anything if nobody is using it. If you want to expose the full feature set of the underlying container, then why wrap it in this thing, why not return a reference to the container itself?

Though I am not sure if you actually want to do that -- having somebody from the outside tinker with the contents of an internal container seems like it would break encapsulation in most cases.

If you look at the llvm classes, you'll find that they usually just do iterator_range<???::iterator> variables() { return make_iterator_range(m_variables.begin(), m_variables.end()); } in situations like these...

lldb/include/lldb/Target/ABI.h
163

I don't think this does what you think it does... it returns an ArrayRef containing a single element -- zero. (And a dangling ArrayRef even...)

165

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.

lldb/source/Breakpoint/BreakpointInjectedSite.cpp
342

This entire function is target specific. I would struggle to find a single line in this expression that could be reused on linux for instance (and let's not even talk about windows..).
I still believe that the simplest approach here would be to allocate memory for this structure on the stack, side-stepping the whole "how to allocate memory in a target-independent manner" discussion completely.

421

Note that this will generally not be the correct address for PIC (so, pretty much everything nowadays). You also need consider the actual address that the object file got loaded at, not just the thing that's written in the file itself.

lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.h
12–58

Using defines for constants is very c-like (and they definitely shouldn't come before the #includes).

mib marked 2 inline comments as done.Fri, Aug 23, 1:31 PM

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 ! :)

lldb/include/lldb/Breakpoint/BreakpointSite.h
54–56

llvm::isa<BreakpointSite>(injected_site) & llvm::isa<BreakpointInjectedSite>(injected_site) both return true.

labath added a subscriber: xiaobai.Mon, Aug 26, 2:41 AM
In D66249#1643594, @mib 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?

lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h
175

Return Expected<string> instead of a string+error combo.

lldb/include/lldb/Breakpoint/BreakpointSite.h
54–56

I'm sorry, I was wrong. It is indeed possible for both of these expressions to return true. However, I was also right in saying this implementation is incorrect :), because the reason that llvm::isa<BreakpointSite>(injected_site) does work is because this function does not get invoked at all -- that expression hits the upcast special case/optimization in the isa implementation https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/Casting.h#L62, which does not invoke the classof method when the cast can be statically proven "correct".

So, this function is dead code and should be removed.

lldb/source/Breakpoint/BreakpointInjectedSite.cpp
43

It looks like this entire loop is wrong for the case when you have more than one breakpoint location -- you'll print static int hit_count twice, the second time inside the if statement. And you'll just accumulate weird stuff inside the trap variable.

60–80

This encodes a lot of language specifics, which probably don't belong in a generic class like this. I guess this should somehow go through the ExpressionParser plugin, but I am super familiar with how those work. @xiaobai, do you have any thoughts on that?

72

I was confused when reading this because I thought this refers to the overall count of the conditions, not the fact whether you're processing the first condition or not. I'd rename this to first_condition to make it clearer that its value can change between loop iterations (single_condition sounds too immutable -- either there is just one condition, or there are many of them).

Another pattern you can consider is having a single condition_prefix variable, and printing the conditions via something like:

prefix = "if (";
for (c: conditions) {
  stream << prefix << c.getText();
  prefix = " || ";
}

I generally find this pattern cleaner, but that is somewhat a matter of personal taste.

141

Is this memory freed anywhere? You should really use a safer memory management construct here. It could be as simple as vector<uint8_t>

157–180

The error handling here seems to be very inconsistent. Sometimes you just set the error string (but don't do anything with it), another time you log something.

Overall, I'd recommend preferring returning actual (llvm::)errors instead of a bool+log combo because it's shorter, and enables one to decide what to do with the errors higher up (e.g. it may be possible to display the error to the user in the future).

246

This also looks like an improper core code -> plugin dependency.

274

llvm format strings require position specifiers, so this needs to be {0}/{1}

430–432

Note that DW_OP_fbreg does not mean that that the location is relative to the rbp (frame pointer register). It is relative to whatever the debug info says it's the base of this functions frame via DW_AT_frame_base. So, that could be rsp+constant, or anything else really... I think it's fine to not support any other frame base than a plain rbp, but it would probably be good to detect that and abort if the frame is not rbp-based.

lldb/source/Breakpoint/BreakpointOptions.cpp
589

This is pretty minute, but I don't remember seeing this style of writing the ternary operator anywhere else in lldb or llvm codebase (i.e., wrapping the condition inside () even when it is a single token). It would be better to avoid that for consistency.

lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
234–328

This code would be much simpler if you just used some kind of a stream to build up the assembly. You'd avoid needing to compute the size of the buffer in advance, allocating a variable-length array (non-standard extension), and having a sanity double check at the end.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
307

no (void)

lldb/source/Target/Process.cpp
1701

Why stuff the string inside a std::string, if you're just going to call .c_str() on it anyway?

xiaobai added inline comments.Mon, Aug 26, 12:48 PM
lldb/source/Breakpoint/BreakpointInjectedSite.cpp
12

Please don't introduce dependencies on Plugins in non-plugin libraries. I'm working on removing them.

Is BreakpointInjectedSite inherently tied to clang? If so, why not move it to the ClangExpressionParser plugin? If not, there should be some kind of generalization to ExpressionParser and PersistentExpressions.

246

Indeed it is. Let's find a way to avoid that.

264

Could you use CompilerDecl here?

342

As others have state, this is entirely target-specific. I'd really rather avoid behavior specific to one platform in something a general as lldbBreakpoint.

mib marked 4 inline comments as done.Mon, Aug 26, 1:38 PM
In D66249#1643594, @mib 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.

lldb/include/lldb/Target/ABI.h
165

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, it sounds we're in agreement then. I'l looking forward to the updated patches. :)

lldb/include/lldb/Target/ABI.h
165

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)