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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
