[Lldb-commits] [PATCH] D51830: Add a way to make scripted breakpoints
Jim Ingham via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Tue Sep 11 13:35:43 PDT 2018
jingham added a comment.
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.
2. 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.
3. 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.
================
Comment at: include/lldb/API/SBBreakpoint.h:26-27
+ SBBreakpoint(const lldb::BreakpointSP &bp_sp);
+
~SBBreakpoint();
----------------
clayborg wrote:
> Why does this need to be public?
See the reply in LLDBSwigPythonCreateScriptedBreakpointResolver...
================
Comment at: include/lldb/API/SBStructuredData.h:26-27
+
+ SBStructuredData(lldb_private::StructuredDataImpl *impl);
~SBStructuredData();
----------------
clayborg wrote:
> Why does this need to be public?
See the reply in LLDBSwigPythonCreateScriptedBreakpointResolver...
================
Comment at: include/lldb/API/SBSymbolContext.h:29-30
+ SBSymbolContext(const lldb_private::SymbolContext *sc_ptr);
+
~SBSymbolContext();
----------------
clayborg wrote:
> Why does this need to be public?
See the reply in LLDBSwigPythonCreateScriptedBreakpointResolver...
================
Comment at: include/lldb/lldb-enumerations.h:260-267
+ eSearchDepthInvalid = 0,
+ eSearchDepthTarget,
eSearchDepthModule,
eSearchDepthCompUnit,
eSearchDepthFunction,
eSearchDepthBlock,
eSearchDepthAddress,
----------------
clayborg wrote:
> 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)?
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.
================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:4
+class Resolver:
+ got_files = 0
+ def __init__(self, bkpt, extra_args, dict):
----------------
clayborg wrote:
> 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
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.
================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:5-7
+ def __init__(self, bkpt, extra_args, dict):
+ self.bkpt = bkpt
+ self.extra_args = extra_args
----------------
clayborg wrote:
> 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
>
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.
================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:9
+
+ def __callback__(self, sym_ctx):
+ sym_name = "not_a_real_function_name"
----------------
clayborg wrote:
> Do we want this to be named something besides the callback operator?
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?
================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:20-23
+ if sym_ctx.module.IsValid():
+ sym = sym_ctx.module.FindSymbol(sym_name, lldb.eSymbolTypeCode)
+ if sym.IsValid():
+ self.bkpt.AddLocation(sym.GetStartAddress())
----------------
clayborg wrote:
> 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?
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.
================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:28-31
+class ResolverModuleDepth(Resolver):
+ def __get_depth__ (self):
+ return lldb.eSearchDepthModule
+
----------------
clayborg wrote:
> 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?
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.
================
Comment at: scripts/Python/python-wrapper.swig:358
+
+ lldb::SBBreakpoint *bkpt_value = new lldb::SBBreakpoint(breakpoint_sp);
+
----------------
clayborg wrote:
> 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?
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.
================
Comment at: scripts/interface/SBBreakpoint.i:229-232
+ // Can only be called from a ScriptedBreakpointResolver...
+ SBError
+ AddLocation(SBAddress &address);
+
----------------
clayborg wrote:
> 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?
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.
================
Comment at: scripts/interface/SBStringList.i:42
Clear ();
+
};
----------------
clayborg wrote:
> remove whitespace change
Oops... Will do when I answer the other concerns.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D51830
More information about the lldb-commits
mailing list