[Lldb-commits] [PATCH] D22863: Improve code of loading plugins that provide cmnds

[Abhishek via lldb-commits]

abhishek.aggarwal added a comment.

Hi Greg

Please find my comments inlined. Let me know if I am missing something here.


================
Comment at: include/lldb/API/SBCommandInterpreter.h:141-142
@@ -140,4 +140,4 @@
     
     lldb::SBCommand
-    AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help);
+    AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help, const char* syntax);
 
----------------
clayborg wrote:
> You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.
After reading your review, I suggest to keep the following prototype of this function:

AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help, const char* syntax = nullptr);

This will not break anyone's plugins written with old version of lldb shared library as syntax will be an optional argument.
This way, we will not need to add another API in this class thereby keeping the public APIs as minimal as possible and complete at the same time.

Please let me know if this fits our public API development conditions. Else I will add another variant of AddCommand API with syntax argument.

================
Comment at: include/lldb/API/SBCommandInterpreter.h:270
@@ -269,2 +269,3 @@
                char** /*command*/,
+               size_t /*number of arguments*/,
                lldb::SBCommandReturnObject & /*result*/)
----------------
clayborg wrote:
> Since this is a special class that does use virtual functions that is in the public API, you can't change it. Previous code might have linked against the LLDB shared library and changing this would break those plug-ins and they would install their new implementation in the vtable slot for this function and it would be called incorrectly. Revert this change. 
I will do it.

================
Comment at: include/lldb/API/SBCommandInterpreter.h:310
@@ -308,3 +309,3 @@
     lldb::SBCommand
-    AddCommand(const char* name, lldb::SBCommandPluginInterface* impl, const char* help = nullptr);
+    AddCommand(const char* name, lldb::SBCommandPluginInterface* impl, const char* help = nullptr, const char* syntax = nullptr);
     
----------------
clayborg wrote:
> You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.
Same comment as for SBCommandInterpreter::AddCommand API above

This will not break anyone's plugins written with old version of lldb shared library as syntax is an optional argument.
This way, we will not need to add another API in this class thereby keeping the public APIs as minimal as possible and complete at the same time.

================
Comment at: packages/Python/lldbsuite/test/functionalities/plugins/commands/plugin.cpp:37
@@ -36,2 +36,3 @@
                char** command,
+               size_t arg_count,
                lldb::SBCommandReturnObject &result)
----------------
clayborg wrote:
> revert
Will do it.

================
Comment at: source/API/SBCommandInterpreter.cpp:152
@@ -151,3 +151,3 @@
         SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this());
-        bool ret = m_backend->DoExecute (debugger_sb,(char**)command.GetArgumentVector(), sb_return);
+        bool ret = m_backend->DoExecute (debugger_sb,(char**)command.GetArgumentVector(),command.GetArgumentCount(), sb_return);
         sb_return.Release();
----------------
clayborg wrote:
> Remove count, revert this line.
Will do it.

================
Comment at: source/API/SBCommandInterpreter.cpp:156
@@ -155,3 +155,3 @@
     }
-    lldb::SBCommandPluginInterface* m_backend;
+    std::shared_ptr<lldb::SBCommandPluginInterface> m_backend;
 };
----------------
clayborg wrote:
> Can't change public API, revert this. This would break anyone's existing commands as the layout of this class would change and possibly break and code that links against this.
Few points which I want to say here:

1. class CommandPluginInterfaceImplementation is completely a part of implementation file and is not present in any public interface file. Hence, any API of this class will never be exposed to lldb shared library users.


2. I referring to  packages/Python/lldbsuite/test/functionalities/plugins/commands/plugin.cpp file as an example.
    In lldb::PluginInitialize() function the ChildCommand instance is dynamically allocated and a pointer to this instance will be copied to m_backend data member of  CommandPluginInterfaceImplementation class. However, no one is freeing this dynamically allocated memory leading to memory leaks. I believe anyone who would have written plugins using this approach will be leaking memory as I just explained. In my opinion it is a bug and for this we need to change this pointer to a shared pointer.

Please let me know if I am missing something here.

================
Comment at: source/API/SBCommandInterpreter.cpp:598
@@ -597,3 +597,3 @@
 lldb::SBCommand
-SBCommandInterpreter::AddCommand (const char* name, lldb::SBCommandPluginInterface* impl, const char* help)
+SBCommandInterpreter::AddCommand (const char* name, lldb::SBCommandPluginInterface* impl, const char* help, const char* syntax)
 {
----------------
clayborg wrote:
> You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.
Same Comment as in interface file of this class.

AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help, const char* syntax = nullptr); might solve this issue.

================
Comment at: source/API/SBCommandInterpreter.cpp:667
@@ -666,3 +666,3 @@
 lldb::SBCommand
-SBCommand::AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help)
+SBCommand::AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help, const char* syntax)
 {
----------------
clayborg wrote:
> You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.
Same Comment as in interface file of this class for this function.


https://reviews.llvm.org/D22863