Page MenuHomePhabricator

Add a way to make scripted breakpoints
ClosedPublic

Authored by jingham on Sep 7 2018, 6:05 PM.

Details

Summary

This change allows you to make a breakpoint resolver kernel in Python.

The breakpoint search mechanism in lldb works on top of a generic mechanism that uses a pair of Searcher - with its associated SearchFilter - and Resolver. The Searcher iterates through the contours of a target, stopping at a depth (module, comp_unit, function...) specified by the Resolver. If the object at the requested depth matches the search filter, then the Searcher calls the Resolver's SearchCallback function, handing it a SymbolContext representing that stage of the search.

In the case of a BreakpointResolver, if the Resolver finds any addresses in that SymbolContext which it wants to break on, it calls AddLocation on its owning Breakpoint to add that location to the breakpoint.

This change allows you to write a simple Python class to add whatever collection of locations makes sense using whatever logic you want. The class must provide a callback method that takes a SBSymbolContext. This will get called at each appropriate stage of the search. You can optionally provide a method that returns the search depth (which defaults to Module if unspecified), and a help method that will be used in the breakpoint description.

When objects of the given class are constructed to represent a specific breakpoint, they are passed a StructuredData object which can be used to parametrize that particular breakpoint. From the SB API's you can pass in an arbitrary SBStructuredData. From the command line I added -k and -v options to "break set" that you provide in pairs to build up a StructuredData::Dictionary which is passed to the resolver. Also, from the command-line the -f and -s options are used to construct the SearchFilter for the breakpoint's Searcher.

For instance, a simple full symbol name breakpoint can be implemented with:

> cat resolver.py
import lldb

class Resolver:
  def __init__(self, bkpt, extra_args, dict):
      self.bkpt = bkpt
      self.extra_args = extra_args

  def __callback__(self, sym_ctx):
      sym_item = self.extra_args.GetValueForKey("symbol")
      if not sym_item.IsValid():
          return
      sym_name = sym_item.GetStringValue(1000)

      sym = sym_ctx.module.FindSymbol(sym_name, lldb.eSymbolTypeCode)
      if sym.IsValid():
          self.bkpt.AddLocation(sym.GetStartAddress())

  def get_short_help(self):
      return "I am a python breakpoint resolver"
> lldb a.out
(lldb) target create "a.out"
Current executable set to 'a.out' (x86_64).
(lldb) command script import resolver.py
(lldb) break set -P resolver.Resolver -k symbol -v break_on_me
(lldb)  break set -P resolver.Resolver -k symbol -v break_on_me
Breakpoint 1: where = a.out`break_on_me at main.c:5, address = 0x0000000100000f40
(lldb) break list
Current breakpoints:
1: I am a python breakpoint resolver, locations = 1
  1.1: where = a.out`break_on_me at main.c:5, address = a.out[0x0000000100000f40], unresolved, hit count = 0

The functionality is advanced enough that this is useful, and I don't expect that will change much. There are tests for all of this.

There are some future work items:

Docs are forthcoming.

Serialization is not all the way working, but I think it's worth getting the current state of things in before I tackle that.

I also need to invent a way for the init to vet its incoming arguments and return an error - which will abort the breakpoint creation - if they don't have some necessary entries.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

jingham created this revision.Sep 7 2018, 6:05 PM
jingham edited the summary of this revision. (Show Details)Sep 7 2018, 6:08 PM
kristina added subscribers: clayborg, kristina.

Please add code reviewers to your differentials when you make them especially when they're this big, at the very least the code owner for that particular project should be added. I added as a reviewer @clayborg as he is the code owner for LLDB.

Thanks for adding Greg. I am the code owner for the breakpoints part of lldb, and most recently I've been the only one adding to the scripting interface. There wasn't another really germane reviewer, and since the dev list was CC'ed I figured if anybody was interested they would weigh in from there.

See inlined comments. Cool stuff.

include/lldb/API/SBBreakpoint.h
26–27

Why does this need to be public?

include/lldb/API/SBStructuredData.h
26–27

Why does this need to be public?

include/lldb/API/SBSymbolContext.h
29–30

Why does this need to be public?

include/lldb/lldb-enumerations.h
261–268

Since this is public API we shouldn't change the enumeration values as this can cause problems with LLDBRPC.framework. Change eSearchDepthInvalid to be -1 and leave all other enums the same (including not renaming kNumSearchDepthKinds)?

packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py
4 ↗(On Diff #164556)

Do you want this to be a class global? Seems like this belongs as an actual member variable and should be initialized in the constructor

5–7 ↗(On Diff #164556)

Might be better to not do any work of consequence in the constructor and maybe only give the breakpoint as the only arg? Maybe something like:

def __init__(self, bkpt):
    self.bkpt = bkpt
    self.args = None

def initialize(self, args):
    self.args = args
    # Check args and return an SBError if they are bad
    return lldb.SBError()

This allows you to reject the arguments if they are invalid

9 ↗(On Diff #164556)

Do we want this to be named something besides the callback operator?

20–23 ↗(On Diff #164556)

Why do we need to manually add the breakpoint here? Can we just return True or False from this function to ok the symbol context? Or is the idea that we might move the address around a bit or add multiple locations give a single symbol context?

28–31 ↗(On Diff #164556)

Seems like "get_depth" should just be a normal method name. The double underscore indicates this function is private. Any reason this needs to be private?

33 ↗(On Diff #164556)

ditto

37 ↗(On Diff #164556)

ditto

scripts/Python/python-wrapper.swig
358

Isn't the code generated by this internal to LLDB? Would be nice to not have to make the shared pointer constructor for lldb::SBBreakpoint public if possible?

362

Ditto about not making lldb::SBStructuredData(args_impl) public if possible?

scripts/interface/SBBreakpoint.i
229–232

Do we enforce the limitation mentioned in the comment? If not, then remove the comment? Else it would be great if this wasn't public. One solution to avoid making the function public would be to have the Resolver.callback function return a list of lldb.SBAddress() objects. The internal LLDB code that made the callback call could then add the locations to the breakpoint itself from inside LLDB's private layer?

scripts/interface/SBStringList.i
42 ↗(On Diff #164556)

remove whitespace change

I addressed the individual concerns in the comments. There were a couple of overall points:

  1. Could we avoid adding AddLocation by having the callback return a list of addresses to set breakpoints. I actually like directly saying "Set me a location there" and I have another use for the return value. So I'd prefer to keep that as it it. But I'm up for being convinced if you have a strong reason.
  1. Making some lldb_private -> SB object constructors public. The other option is to friend the SWIGPythonCreateScriptedBreakpointResolver to these classes. I think friend classes are uglier, but I don't have a strong feeling about this.
  1. Names for callbacks. I was mostly following the convention that Enrico seems to have used in the past. If we are going to remove underscores we should get rid of them for all the functions you are supposed to provide. In my mind the underscores were to make the functions we are forcing you to provide less likely to collide with normal function names. But if that doesn't seem a valuable enough goal, I'll happily dump them.
include/lldb/API/SBBreakpoint.h
26–27

See the reply in LLDBSwigPythonCreateScriptedBreakpointResolver...

include/lldb/API/SBStructuredData.h
26–27

See the reply in LLDBSwigPythonCreateScriptedBreakpointResolver...

include/lldb/API/SBSymbolContext.h
29–30

See the reply in LLDBSwigPythonCreateScriptedBreakpointResolver...

include/lldb/lldb-enumerations.h
261–268

This was not public API before this checkin. Before that it was an enum in an lldb_private class (Searcher). So there aren't any compatibility issues with other uses.

packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py
4 ↗(On Diff #164556)

This is just for testing purposes, this would not be good general practice. I want a way to discover whether, when the get_depth call returns Module, the search callback gets called at the module depth and ditto for CompUnit. However, I have no way in the test to get my hands on the specific Python instantiation for a given breakpoint resolver, so I can't ask it questions. But I can query class statics. So I have the instance record how it was called in the global, and then I can assert it in the test. I only use this in the initial breakpoint setting - because I know that only one ScriptedBreakpointResolver is going to get to run then. If you tried to use it, for instance, on module load it clearly wouldn't work. But it serves the testing purposes.

I didn't intend this to be a model, I'll make an example for the examples directory that doesn't have this testing specific goo.

5–7 ↗(On Diff #164556)

There isn't currently a way for a breakpoint to fail to be made. Right now we really only have breakpoints that find no locations. So that is a concept we'd have to add before this was really useful. I think that's a good idea but its out of scope for what I have time for with this checkin.

But even when we do that, I'd prefer to have an optional is_valid method on the Python class, and you would check the args in the constructor and if you didn't like them you would return false for is_valid. The reason for that is that I'd like to keep the interface for people writing these dingi as simple as possible. We aren't ever going to call init w/o calling initialize so the separation doesn't really meaningfully postpone work. And I'd rather not make people define two methods if they don't have to.

Doing it the way I suggested, you only HAVE to write the init, and validating is optional so you don't have to do anything if you don't need to.

9 ↗(On Diff #164556)

I was back and forth on that. This is an object whose job is to provide a search callback. So it seemed like calling the callback "search_callback" was redundant. But I don't have a strong feeling about that. It there something you think would be clearer?

20–23 ↗(On Diff #164556)

The latter. If your resolver returns "Module" as its depth, it will get called ONCE per module, with the Symbol Context containing that module and nothing below it. But you might very well want to make more than one breakpoint location in that module. So we can't use a True or False.

I thought about having the callback return a vector of addresses, which I would then turn into breakpoints, but to tell the truth getting that to work through the generic Script & ScriptPython interfaces was more trouble than it was worth.

28–31 ↗(On Diff #164556)

I never really knew what the logic for these names in other similar constructs was. I had assumed that the double underscores were to make the names unlikely to collide with other common names more than to indicate they are private.
\_\_callback\_\_
is no more or less private than \_\_get_depth\_\_, so if we remove the underscores we should do so for both. I don't have a strong feeling here except that it does seem useful to keep the names we require from colliding with other common names folks might use.

scripts/Python/python-wrapper.swig
358

I didn't see much harm in making it public. It won't go into the Python API (since I didn't add it to the .i file) and it is clearly something you can't make from the SB API layer in C++ since you can't get your hands on the internal object.

The other option was to friend LLDBSwigPythonCreateScriptedBreakpointResolver to SBBreakpoint & SBStructuredData. That seemed to me uglier, but I don't have a strong opinion. If you prefer the friend route I'm happy to make that change.

scripts/interface/SBBreakpoint.i
229–232

I do enforce the restriction (around line 507 in the SBBreakpoint.cpp). I even return a helpful error if you try to misuse it.

As we both noted, the other option is to return a list of SBAddress objects.

I was intending to use the return value to allow you to pop up one level in the search. That's useful if you want to have the Searcher do the CompUnit iteration for you, so you return CompUnit for the depth, but then you find the comp unit you were looking for in this module, you can return false to indicate that you are done with this level of iteration. Then the Searcher would "pop up one level" - i.e. move on to the next module. This isn't fully hooked up yet, but that was my plan for the callback return.

I also think this makes what you are doing in the Resolver less magical - you are directly adding locations rather than handing out a list of addresses. That seems clearer to me.

And (though this is less important) it was also just a PITN to make passing the list back work through the Script -> PythonScript API's, so unless you really object to this, I'd rather do it the way it is currently done. Seems to me making these scripting interfaces - which we are going to have to pipe through any language we ultimately support - as simple as possible is not a bad principle...

I could also change the name to "AddLocationInScriptedResolver" to make it clear this isn't a general purpose function, if that would make things clearer.

scripts/interface/SBStringList.i
42 ↗(On Diff #164556)

Oops... Will do when I answer the other concerns.

Only issue now is documentation and why are we requiring addresses to be in compile unit. See inlined comments.

include/lldb/API/SBTarget.h
667

Might be nice to add some documentation here for class_name, extra_args

include/lldb/Breakpoint/BreakpointResolverScripted.h
34–37 ↗(On Diff #164556)

indentation is off

include/lldb/lldb-enumerations.h
268

It wasn't public? Why isn't this entire thing new then? The diffs show just a few changes? If no one was able to use this enum before if it wasn't in any public arguments, then we can change as needed. But from the diff it seems this was here?

packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py
4 ↗(On Diff #164556)

Gotcha, no worries then.

9 ↗(On Diff #164556)

It is is similar to other stuff lets leave this then. Just something I noticed.

23 ↗(On Diff #164556)

Can locations have names at all? Do we want AddLocation to take an optional name as a "const char *"?

31 ↗(On Diff #164556)

No worries. If it is like other python plug-ins then this is fine.

33 ↗(On Diff #164556)

If we set the depth to compile unit, do we get called with a symbol context for each compile unit? Is there a way to get called for each function? I would use this functionality for our PGO plug-in that sets a breakpoint on every concrete function. I currently use a regex bp and then weed out all inlines by disabling them. But it would be nice to avoid having extra inline locations in the first place.

scripts/Python/python-wrapper.swig
358

no worries then.

scripts/interface/SBBreakpoint.i
232

So the question becomes should we outlaw this for all breakpoints? Might be nice to be able to add extra locations to any kind of breakpoint to allow sharing a condition. Any reason to strictly limit this to only script breakpoints? Also, remember my previous comment about locations being able to have names?

scripts/interface/SBTarget.i
735

Can't remember if there is a way to get documentation to show up in python in the .i file but if there is, we should document the exact class and how to write one here so if someone does:

>>> help(lldb.SBTarget.BreakpointCreateFromScript)

The will get all the documentation they need for how to use this.

source/API/SBBreakpoint.cpp
512

Does this get passed back down into python to ok the address? Why would we need to do this here? The script is asking us to set the breakpoint at this address, seeing if it passes the filter seems redundant?

source/API/SBStructuredData.cpp
83

Another option would be to have a callback that can be supplied to SBStructuredData::ForEach(...) function whose signature is:

bool Callback(const char *key, lldb::SBStructuredData &object);

If use returns false to stop, true to continue iteration. When I have looked at using SBStructuredData in the past, I wanted this call.

source/Core/SearchFilter.cpp
751–760

What if our python filter was disassembling code and finding calls to PLT entries and set breakpoints using addresses if found in disassembly? Not sure why we give people the ability to set breakpoints with any address anywhere in a module and then require the breakpoint to be in a compile unit?

I addressed most of the comments. I remember we argued about how the filters should work w.r.t. the resolvers back when I first did this, but I still claim my way is right ;-) I gave some arguments for that in response to your inline comment.

I'm going to save this (not sure what happens if you change the diff with in-flight comments) then I'll update the diff with the few little nits you've found and with documentation for SBTarget.CreateBreakpointFromScript.

include/lldb/lldb-enumerations.h
268

This was a pre-commit to this patch set (I was trying to make it a little smaller, though I didn't succeed much...):


r341690 | jingham | 2018-09-07 11:43:04 -0700 (Fri, 07 Sep 2018) | 6 lines

NFC: Move Searcher::Depth into lldb-enumerations as SearchDepth.

In a subsequent commit, I will need to expose the search depth
to the SB API's, so I'm moving this define into lldb-enumerations
where it will get added to the lldb module.


packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py
23 ↗(On Diff #164556)

Breakpoint locations can't currently have names. The problem with adding names to locations is that I don't currently have any code that tracks "the same" location from if the binary changes. Instead, if the binary changes and you rerun, I discard all the locations from the old binary, and rerun the Resolver on the new binary. At that point the name would go missing. I think that would be disconcerting.

I actually want to put in some heuristics to figure out equivalent locations when the binary changes so that I can do a better job of preserving location specific options. Right now, they all get lost if the binary changes. Actually that's not 100% true, if you go from 1 location in a module to 1 location, I am pretty sure I consider this the same location and copy over the options. But if it gets harder than that I just punt at present.

If I can make the locations more stable, then it would make sense to me to be able to name them.

33 ↗(On Diff #164556)

It should except for the part where I only implemented the searcher search down to the CU level 'cause that was all I needed at the time. I'll fix that and use the breakpoint resolvers to test it, but I'd rather do that as a separate commit.

scripts/interface/SBBreakpoint.i
232

Right now, the whole model of the breakpoints is that the Resolvers know everything about how to set the locations. If we allow users to tack on locations to other Resolvers, there's no way for the Resolver to know how to recreate the breakpoint with the added location. So for instance, I don't know how to copy or serialize and deserialize a breakpoint including those extra locations. I'm also not sure what I would do if the binary changes? Would it be okay to just ditch the added location? This would introduce enough odd corner cases that I don't think the feature is worth it.

OTOH, I think it would be really cool to have a way to provide the standard lldb breakpoint resolvers as Python classes that you could subclass. Maybe the subclasses could even use another signature and do something like:

class MyResolver(lldb.FileAndLineResolver):
    def __init__(self, bkpt, extra_args):
        # Same as before
    def __callback__(self, sym_ctx, breakpoint_candidates):
        # breakpoint_candidates are the addresses the resolver WOULD have set...

where the internal resolver would run first, and present a list of the addresses it would have set for this sym_ctx, and you can nix some, or add others. That would keep the model cleaner, since the Resolvers would remain the source of truth for the breakpoint, but allow you to modify the results of the standard resolvers.

source/API/SBStructuredData.cpp
83

It seems a little weird to me to have an API on StructuredData that only makes sense for Dictionaries. This API at least says up front that it only handles Dictionaries (they are the only thing with Keys...) If you are really doing a ForEach you'd want to traverse the other data types as well, running over arrays, digging into arrays of Dictionaries, etc.

TTTT, however, this function doesn't get used anywhere. I put it in in testing so I could make sure I was getting passed all the keys I was putting in. It isn't terribly germane to this patch, so if we're going to make a fancier version I'd rather do that as a separate piece of work.

source/Core/SearchFilter.cpp
751–760

The problem this is trying to solve is, what if your Resolver wants to get called back at the Module level to do searches because that's convenient for some reason, but the user provided a CompileUnit specific search filter? You wouldn't want to add locations that violate the user's filter.

This happens for instance for symbol name breakpoints. The command:

break set -n foo -f bar.c

means "set breakpoints on all the symbols 'foo' in bar.c". But our symbol name resolver gets called back at the module depth. That makes sense, since the symbols are stored in the module. So once you find a symbol you have to run it past the overall SearchFilter to make sure it passes the file filter set by the user above.

In lldb_private you can ask the breakpoint's search filter questions. So for instance if you were doing expensive work function by function you could ask the filter at the beginning of each function if it passes, and then only do the work in that function range if it does.

I didn't make a way to do this in the SB API's - since I don't think I've ever used this in the lldb builtin breakpoint resolvers. But if this seems like a useful thing, it would be easy to add an:

bool SBBreakpoint::AddressPassesFilter(SBAddress &address)

API to allow this.

jingham updated this revision to Diff 165184.Sep 12 2018, 5:16 PM

Add documentation for SBTarget.CreateBreakpointFromScript, and fixed a few minor nits that Greg pointed out.

clayborg accepted this revision.Sep 13 2018, 7:12 AM
This revision is now accepted and ready to land.Sep 13 2018, 7:12 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald Transcript
kwk added a subscriber: kwk.Nov 13 2019, 8:10 AM
kwk added inline comments.
source/Core/SearchFilter.cpp
757

@jingham Do you know why you check for the CU again here? I mean there's CompUnitPasses, that should do this, right?

jingham marked an inline comment as done.Nov 13 2019, 12:23 PM
jingham added inline comments.
source/Core/SearchFilter.cpp
757

The way the interaction between the Breakpoint Resolvers and the Search Filters work is that the SearchFilter asks the Resolver at what level it wants to be presented Symbol Containers (the module, the CU, Functions, etc.). Then the Search Filter iterates over the Symbol Container space handing the Resolver entities at the level the Resolver requested. Of course, the SearchFilter will only pass the resolver entities at that level that pass its test. But there is no guarantee that the an object passed at the level the Resolver requested is passes the SearchFilter in its entirety. So when the Resolver finds an address it was looking for in the search entity, it still needs to make sure that address actually passes SearchFilter. So the Resolver always needs to call AddressPasses before it accepts that address.

For instance, name breakpoints are searched at the Module level, because name tables are per module, so that is the convenient level to do the search. That means the by-name BreakpointResolver is going to get passed Modules. But if the SearchFilter was actually a by CU one, when the resolver finds a name some Module it was passed, it still needs to be sure that it's in the SearchFilter's CU, which it does by calling back into AddressPasses before setting a breakpoint on that address.

Was that what you were asking?