[Lldb-commits] [PATCH] D88123: Add the ability to write 'target stop-hooks' in Python

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I'm very excited about this feature. Great job on the documentation, both in the help output as for the website.  Do you have a potential use case in mind that we could add to the examples?



================
Comment at: lldb/docs/use/python-reference.rst:843
+|                    |                                       | **target** is the SBTarget to which the stop hook is added.                                                      |
+|                    |                                       |                                                                                                                  |
+|                    |                                       |                                                                                                                  |
----------------
Any reason for the double empty lines?


================
Comment at: lldb/docs/use/python-reference.rst:863
+
+::
+   (lldb) command script import MyModule.py
----------------
There needs to be a newline here for this to be rendered as code. 


================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:302
+  virtual StructuredData::GenericSP
+  CreateScriptedStopHook(lldb::TargetSP target_sp, const char *class_name,
+                         StructuredDataImpl *args_data, Status &error) {
----------------
Could this function return an `Expected<StructuredData::GenericSP>` instead?


================
Comment at: lldb/include/lldb/Target/Target.h:1198
+
+    StringList *GetCommandPointer() { return &m_commands; }
+    const StringList &GetCommands() { return m_commands; }
----------------
Can we drop this altogether? We can always get the address of the reference at the call site. 


================
Comment at: lldb/include/lldb/Target/Target.h:1200
+    const StringList &GetCommands() { return m_commands; }
+    void SetActionFromString(std::string &strings);
+    void SetActionFromStrings(std::vector<std::string> &strings);
----------------
can this be a const reference? Maybe even a `StringRef`? Similar question for the methods below.


================
Comment at: lldb/include/lldb/Target/Target.h:1231
+    std::string m_class_name;
+    StructuredDataImpl *m_extra_args; // We own this structured data,
+                                      // but the SD itself manages the UP.
----------------
Please make these Doxygen comments (`///`) and put them on the line above the variable. 


================
Comment at: lldb/include/lldb/Target/Target.h:1237
+    // Use CreateStopHook to make a new empty stop hook. The GetCommandPointer
+    // and fill it with commands, and SetSpecifier to set the specifier shared
+    // pointer (can be null, that will match anything.)
----------------
`///`


================
Comment at: lldb/include/lldb/Target/Target.h:1246
 
   // Add an empty stop hook to the Target's stop hook list, and returns a
   // shared pointer to it in new_hook. Returns the id of the new hook.
----------------
And you might as well fix that here too :-) 


================
Comment at: lldb/include/lldb/Target/Target.h:1248
   // shared pointer to it in new_hook. Returns the id of the new hook.
-  StopHookSP CreateStopHook();
+  StopHookSP CreateStopHook(bool use_commands);
+
----------------
I think we should make this a little enum that has the values command and scripted.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4691
+        // The IOHandler editor is only for command lines stop hooks:
+        Status error;
+        Target::StopHookCommandLine *hook_ptr =
----------------
Is this used?


================
Comment at: lldb/source/Target/Target.cpp:3149
+  SymbolContextSpecifier *specifier = GetSpecifier();
+  if (!specifier)
+    return true;
----------------
So many early returns in this patch 😍


================
Comment at: lldb/source/Target/Target.cpp:3269
+
+  m_extra_args = new StructuredDataImpl();
+
----------------
It looks like we will leak this if script_interp is null. One solution for that would be to wrap it in a unique_ptr and release it when passing it to CreateScriptedStopHook. If we do the `m_extra_args` assignment just before that call that also guarantees that the variable remains null until then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88123/new/

https://reviews.llvm.org/D88123