Improve code of loading plugins that provide cmnds

[abhishek.aggarwal]

Summary

• Modified code that enables writing new user-defined commands and use them through LLDB CLI. Modifications are:
  • Define the 'syntax' for each user-defined command
    • Added an argument in SBCommandInterpreter::AddCommand() and SBCommand::AddCommand() API
    • Allow passing syntax for each user-defined command
    • Earlier, only 'help' could be defined and passed for commands
  • Passed 'number of arguments' entered on CLI for user-defined commands
    • Added an argument (number of options) in SBCommandPluginInterface::DoExecute() API to know the number of arguments passed for commands
  • In CommandPluginInterfaceImplementation class:
    • Make the data member m_backend a shared_ptr
    • Avoids memory leaks of dynamically allocated SBCommandPluginInterface instances created in lldb::PluginInitialize() API

Signed-off-by: Abhishek Aggarwal <[e-mail]>

[clayborg]

You can't change the exiting public API (anything in include/lldb/API/*). If you want to a add a new function, you must add a new version with the new argument, but leave the old one alone. See the following link for details:

http://lldb.llvm.org/architecture/index.html#api

SBCommandPluginInterface is special because it does have virtual functions. This means you can't change it or any of the virtual functions. So all changes to this class must be reverted since old code might have linked against the old version and still be compiled for the old version.

A simpler fix for all of this could be to just add a:

lldb::SBCommand::SetSyntax(const char *);

Then you wouldn't need to modify any other classes. But you can add new variants that include syntax to the AddCommand() function if you want.

include/lldb/API/SBCommandInterpreter.h

141–142

You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.

273

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.

312

You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.

packages/Python/lldbsuite/test/functionalities/plugins/commands/plugin.cpp

37 ↗

(On Diff #65733)

revert

source/API/SBCommandInterpreter.cpp

152

Remove count, revert this line.

156

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.

598

You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.

678

You can't change public API, you can only add to it. Just add another function with syntax and leave the other one alone.

[granata.enrico]

Everything Greg said makes total sense

In general dynamically loadable C++ plugins as a feature is a little rough around the edges. I am glad to see interest in improving it!

[abhishek.aggarwal]

Hi Greg

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

include/lldb/API/SBCommandInterpreter.h

141–142

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.

273

I will do it.

312

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.

packages/Python/lldbsuite/test/functionalities/plugins/commands/plugin.cpp

37 ↗

(On Diff #65733)

Will do it.

source/API/SBCommandInterpreter.cpp

152

Will do it.

156

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.

598

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.

678

Same Comment as in interface file of this class for this function.

[labath]

include/lldb/API/SBCommandInterpreter.h

141–142

We are trying to maintain binary compatibility. So, while your proposal maintains source-code level compatibilty, it will still break precompiled binaries, as the mangled function name changes.

</drive-by>

[abhishek.aggarwal]

include/lldb/API/SBCommandInterpreter.h

141–142

Thanks for the clarification. Any comments in SBCommandInterpreter.cpp file for changing m_backend data member of CommandPluginInterfaceImplementation class to a shared pointer?

[labath]

source/API/SBCommandInterpreter.cpp

156

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.

[abhishek.aggarwal]

New patch according to review feedback

• Added another variant of AddCommand API with syntax as an additional argument

[clayborg]

Looks good!

source/API/SBCommandInterpreter.cpp

156

Woops, never mind, if this isn't exposed via the public API, then this is fine... Sorry, thought this was in a header file... This is ok

include/lldb/API/SBCommandInterpreter.h

public:
     GetProcess ();
     
     lldb::SBDebugger
     GetDebugger ();
     
     lldb::SBCommand
     AddMultiwordCommand (const char* name, const char* help);
     
     lldb::SBCommand
     AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help);
 
+    lldb::SBCommand
+    AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help, const char* syntax);
+
     void
     SourceInitFileInHomeDirectory (lldb::SBCommandReturnObject &result);
 
     void
     SourceInitFileInCurrentWorkingDirectory (lldb::SBCommandReturnObject &result);
 
     lldb::ReturnStatus
     HandleCommand (const char *command_line, lldb::SBCommandReturnObject &result, bool add_to_history = false);

 {
 public:
     virtual
     ~SBCommandPluginInterface() = default;
 
     virtual bool
     DoExecute (lldb::SBDebugger /*debugger*/,
                char** /*command*/,
                lldb::SBCommandReturnObject & /*result*/)
     {
         return false;
     }
 };
     
 class SBCommand
 {
 public:
public:
     
     void
     SetFlags (uint32_t flags);
     
     lldb::SBCommand
     AddMultiwordCommand(const char* name, const char* help = nullptr);
     
     lldb::SBCommand
     AddCommand(const char* name, lldb::SBCommandPluginInterface* impl, const char* help = nullptr);
     
+    lldb::SBCommand
+    AddCommand(const char* name, lldb::SBCommandPluginInterface* impl, const char* help, const char* syntax);
+
 private:
     friend class SBDebugger;
     friend class SBCommandInterpreter;
     
     SBCommand (lldb::CommandObjectSP cmd_sp);
     
     lldb::CommandObjectSP m_opaque_sp;
 };
 
 } // namespace lldb
 
 #endif // LLDB_SBCommandInterpreter_h_

source/API/SBCommandInterpreter.cpp

     
 protected:
     bool
     DoExecute(Args& command, CommandReturnObject &result) override
     {
         SBCommandReturnObject sb_return(&result);
         SBCommandInterpreter sb_interpreter(&m_interpreter);
         SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this());
         bool ret = m_backend->DoExecute (debugger_sb,(char**)command.GetArgumentVector(), sb_return);
         sb_return.Release();
         return ret;
     }
-    lldb::SBCommandPluginInterface* m_backend;
+    std::shared_ptr<lldb::SBCommandPluginInterface> m_backend;
 };
 
 SBCommandInterpreter::SBCommandInterpreter (CommandInterpreter *interpreter) :
     m_opaque_ptr (interpreter)
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));
 
     if (log)
SBCommandInterpreter::AddMultiwordCommand (const char* name, const char* help)
     new_command->SetRemovable (true);
     lldb::CommandObjectSP new_command_sp(new_command);
     if (new_command_sp && m_opaque_ptr->AddUserCommand(name, new_command_sp, true))
         return lldb::SBCommand(new_command_sp);
     return lldb::SBCommand();
 }
 
 lldb::SBCommand
 SBCommandInterpreter::AddCommand (const char* name, lldb::SBCommandPluginInterface* impl, const char* help)
 {
     lldb::CommandObjectSP new_command_sp;
     new_command_sp.reset(new CommandPluginInterfaceImplementation(*m_opaque_ptr,name, impl, help));
 
     if (new_command_sp && m_opaque_ptr->AddUserCommand(name, new_command_sp, true))
         return lldb::SBCommand(new_command_sp);
     return lldb::SBCommand();
 }
 
+lldb::SBCommand
+SBCommandInterpreter::AddCommand (const char* name, lldb::SBCommandPluginInterface* impl, const char* help, const char* syntax)
+{
+    lldb::CommandObjectSP new_command_sp;
+    new_command_sp.reset(new CommandPluginInterfaceImplementation(*m_opaque_ptr,name, impl, help, syntax));
+
+    if (new_command_sp && m_opaque_ptr->AddUserCommand(name, new_command_sp, true))
+        return lldb::SBCommand(new_command_sp);
+    return lldb::SBCommand();
+}
+
 SBCommand::SBCommand() = default;
 
 SBCommand::SBCommand (lldb::CommandObjectSP cmd_sp) : m_opaque_sp (cmd_sp)
 {}
 
 bool
 SBCommand::IsValid()
 {
SBCommand::AddMultiwordCommand (const char* name, const char* help)
     new_command->SetRemovable (true);
     lldb::CommandObjectSP new_command_sp(new_command);
     if (new_command_sp && m_opaque_sp->LoadSubCommand(name,new_command_sp))
         return lldb::SBCommand(new_command_sp);
     return lldb::SBCommand();
 }
 
 lldb::SBCommand
 SBCommand::AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help)
 {
     if (!IsValid ())
         return lldb::SBCommand();
     if (!m_opaque_sp->IsMultiwordObject())
         return lldb::SBCommand();
     lldb::CommandObjectSP new_command_sp;
     new_command_sp.reset(new CommandPluginInterfaceImplementation(m_opaque_sp->GetCommandInterpreter(),name,impl,help));
     if (new_command_sp && m_opaque_sp->LoadSubCommand(name,new_command_sp))
         return lldb::SBCommand(new_command_sp);
     return lldb::SBCommand();
 }
 
+lldb::SBCommand
+SBCommand::AddCommand (const char* name, lldb::SBCommandPluginInterface *impl, const char* help, const char* syntax)
+{
+    if (!IsValid ())
+        return lldb::SBCommand();
+    if (!m_opaque_sp->IsMultiwordObject())
+        return lldb::SBCommand();
+    lldb::CommandObjectSP new_command_sp;
+    new_command_sp.reset(new CommandPluginInterfaceImplementation(m_opaque_sp->GetCommandInterpreter(),name,impl,help, syntax));
+    if (new_command_sp && m_opaque_sp->LoadSubCommand(name,new_command_sp))
+        return lldb::SBCommand(new_command_sp);
+    return lldb::SBCommand();
+}
+
+
 uint32_t
 SBCommand::GetFlags ()
 {
     return (IsValid() ? m_opaque_sp->GetFlags().Get() : 0);
 }
 
 void
 SBCommand::SetFlags (uint32_t flags)
 {
     if (IsValid())
         m_opaque_sp->GetFlags().Set(flags);
 }