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

[Pavel Labath via lldb-commits]

labath added inline comments.

================
Comment at: source/API/SBCommandInterpreter.cpp:156
@@ -155,3 +155,3 @@
     }
-    lldb::SBCommandPluginInterface* m_backend;
+    std::shared_ptr<lldb::SBCommandPluginInterface> m_backend;
 };
----------------
abhishek.aggarwal wrote:
> 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.
As far as I know my ABI rules, this change is fine. The class is not mentioned anywhere in the header, so it's implementation details cannot leak to the users. This is actually the recommended approach for writing stable APIs - put the meat in the cpp file, and make a separate class in the header which is just  a thin wrapper around that. Also, since this class inherits from CommandObjectParsed, any changes there would change this class as well, and we certainly don't vet changes to those classes.


https://reviews.llvm.org/D22863