clayborg added a comment.
Only issue now is documentation and why are we requiring addresses to be in
compile unit. See inlined comments.
================
Comment at: include/lldb/API/SBTarget.h:667
+ lldb::SBBreakpoint BreakpointCreateFromScript(
+ const char *class_name,
+ SBStructuredData &extra_args,
----------------
Might be nice to add some documentation here for class_name, extra_args
================
Comment at: include/lldb/Breakpoint/BreakpointResolverScripted.h:34-37
+ const llvm::StringRef class_name,
+ lldb::SearchDepth depth,
+ StructuredDataImpl *args_data,
+ ScriptInterpreter &script_interp);
----------------
indentation is off
================
Comment at: include/lldb/lldb-enumerations.h:267
eSearchDepthAddress,
- kNumSearchDepthKinds = eSearchDepthAddress
+ kLastSearchDepthKind = eSearchDepthAddress
};
----------------
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?
================
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):
----------------
jingham wrote:
> 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.
Gotcha, no worries then.
================
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"
----------------
jingham wrote:
> 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?
It is is similar to other stuff lets leave this then. Just something I noticed.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:23
+ if sym.IsValid():
+ self.bkpt.AddLocation(sym.GetStartAddress())
+
----------------
Can locations have names at all? Do we want AddLocation to take an optional
name as a "const char *"?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:31
+ return lldb.eSearchDepthModule
+
+class ResolverCUDepth(Resolver):
----------------
No worries. If it is like other python plug-ins then this is fine.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:33
+class ResolverCUDepth(Resolver):
+ def __get_depth__ (self):
+ return lldb.eSearchDepthCompUnit
----------------
clayborg wrote:
> ditto
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.
================
Comment at: scripts/Python/python-wrapper.swig:358
+
+ lldb::SBBreakpoint *bkpt_value = new lldb::SBBreakpoint(breakpoint_sp);
+
----------------
jingham wrote:
> 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.
no worries then.
================
Comment at: scripts/interface/SBBreakpoint.i:232
+ AddLocation(SBAddress &address);
+
bool
----------------
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?
================
Comment at: scripts/interface/SBTarget.i:735
+
+ lldb::SBBreakpoint BreakpointCreateFromScript(
+ const char *class_name,
----------------
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.
================
Comment at: source/API/SBBreakpoint.cpp:512
+
+ if (bkpt_sp->GetSearchFilter()->AddressPasses(address.ref()))
+ bkpt_sp->AddLocation(address.ref());
----------------
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?
================
Comment at: source/API/SBStructuredData.cpp:83
+bool SBStructuredData::GetKeys(lldb::SBStringList &keys) const {
+ if (!m_impl_up)
----------------
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.
================
Comment at: source/Core/SearchFilter.cpp:751-759
+ SymbolContext sym_ctx;
+ address.CalculateSymbolContext(&sym_ctx, eSymbolContextEverything);
+ if (!sym_ctx.comp_unit) {
+ if (m_cu_spec_list.GetSize() != 0)
+ return false; // Has no comp_unit so can't pass the file check.
+ }
+ if (m_cu_spec_list.FindFileIndex(0, sym_ctx.comp_unit, false) == UINT32_MAX)
----------------
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?
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
