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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See inlined comments. Cool stuff.



================
Comment at: include/lldb/API/SBBreakpoint.h:26-27
 
+  SBBreakpoint(const lldb::BreakpointSP &bp_sp);
+
   ~SBBreakpoint();
----------------
Why does this need to be public?


================
Comment at: include/lldb/API/SBStructuredData.h:26-27
+  
+  SBStructuredData(lldb_private::StructuredDataImpl *impl);
 
   ~SBStructuredData();
----------------
Why does this need to be public?


================
Comment at: include/lldb/API/SBSymbolContext.h:29-30
 
+  SBSymbolContext(const lldb_private::SymbolContext *sc_ptr);
+
   ~SBSymbolContext();
----------------
Why does this need to be public?


================
Comment at: include/lldb/lldb-enumerations.h:260-267
+    eSearchDepthInvalid = 0,
+    eSearchDepthTarget,
     eSearchDepthModule,
     eSearchDepthCompUnit,
     eSearchDepthFunction,
     eSearchDepthBlock,
     eSearchDepthAddress,
----------------
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)?


================
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):
----------------
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


================
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
----------------
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
    


================
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"
----------------
Do we want this to be named something besides the callback operator?


================
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())
----------------
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?


================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:28-31
+class ResolverModuleDepth(Resolver):
+    def __get_depth__ (self):
+        return lldb.eSearchDepthModule
+
----------------
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?


================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:33
+class ResolverCUDepth(Resolver):
+    def __get_depth__ (self):
+        return lldb.eSearchDepthCompUnit
----------------
ditto


================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/resolver.py:37
+class ResolverBadDepth(Resolver):
+    def __get_depth__ (self):
+        return lldb.kLastSearchDepthKind + 1
----------------
ditto


================
Comment at: scripts/Python/python-wrapper.swig:358
+
+    lldb::SBBreakpoint *bkpt_value = new lldb::SBBreakpoint(breakpoint_sp);
+
----------------
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?


================
Comment at: scripts/Python/python-wrapper.swig:362
+
+    lldb::SBStructuredData *args_value = new lldb::SBStructuredData(args_impl);
+    PythonObject args_arg(PyRefType::Owned, SBTypeToSWIGWrapper(args_value));
----------------
Ditto about not making lldb::SBStructuredData(args_impl) public if possible?


================
Comment at: scripts/interface/SBBreakpoint.i:229-232
+    // Can only be called from a ScriptedBreakpointResolver...
+    SBError
+    AddLocation(SBAddress &address);
+
----------------
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? 


================
Comment at: scripts/interface/SBStringList.i:42
     Clear ();
+  
 };
----------------
remove whitespace change


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51830