[Lldb-commits] [PATCH] D51830: Add a way to make scripted breakpoints

[Greg Clayton via Phabricator via lldb-commits]

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