Re: [PATCH] gdb/python: remove gdb._mi_commands dict

[Andrew Burgess <aburgess@redhat.com>]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2022-03-18 15:55:26 -0400]:

> The motivation for this patch is the fact that py-micmd.c doesn't build
> with Python 2, due to PyDict_GetItemWithError being a Python 3-only
> function:
> 
>       CXX    python/py-micmd.o
>     /home/smarchi/src/binutils-gdb/gdb/python/py-micmd.c: In function ‘int micmdpy_uninstall_command(micmdpy_object*)’:
>     /home/smarchi/src/binutils-gdb/gdb/python/py-micmd.c:430:20: error: ‘PyDict_GetItemWithError’ was not declared in this scope; did you mean ‘PyDict_GetItemString’?
>       430 |   PyObject *curr = PyDict_GetItemWithError (mi_cmd_dict.get (),
>           |                    ^~~~~~~~~~~~~~~~~~~~~~~
>           |                    PyDict_GetItemString
> 
> A first solution to fix this would be to try to replace
> PyDict_GetItemWithError equivalent Python 2 code.  But I looked at why
> we are doing this in the first place: it is to maintain the
> `gdb._mi_commands` Python dictionary that we use as a `name ->
> gdb.MICommand object` map.  Since the `gdb._mi_commands` dictionary is
> never actually used in Python, it seems like a lot of trouble to use a
> Python object for this.
> 
> My first idea was to replace it with a C++ map
> (std::unordered_map<std::string, gdbpy_ref<micmdpy_object>>).  While
> implementing this, I realized we don't really need this map at all.  The
> mi_command_py objects registered in the main MI command table can own
> their backing micmdpy_object (that's a gdb.MICommand, but seen from the
> C++ code).  To know whether an mi_command is an mi_command_py, we can
> use a dynamic cast.  Since there's one less data structure to maintain,
> there are less chances of messing things up.
> 
>  - Change mi_command_py::m_pyobj to a gdbpy_ref, the mi_command_py is
>    now what keeps the MICommand alive.
>  - Set micmdpy_object::mi_command in the constructor of mi_command_py.
>    If mi_command_py manages setting/clearing that field in
>    swap_python_object, I think it makes sense that it also takes care of
>    setting it initially.
>  - Move a bunch of checks from micmdpy_install_command to
>    swap_python_object, and make them gdb_asserts.
>  - In micmdpy_install_command, start by doing an mi_cmd_lookup.  This is
>    needed to know whether there's a Python MI command already registered
>    with that name.  But we can already tell if there's a non-Python
>    command registered with that name.  Return an error if that happens,
>    rather than waiting for insert_mi_cmd_entry to fail.  Change the
>    error message to "name is already in use" rather than "may already be
>    in use", since it's more precise.

This all looks good to me.  I think the small patch below should also
be included though.

Thanks,
Andrew

---

diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index 18a199de6b9..9b2b7863827 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -579,11 +579,10 @@ micmdpy_dealloc (PyObject *obj)
 			(cmd->mi_command_name == nullptr
 			 ? "(null)" : cmd->mi_command_name));
 
-  /* Remove the command from the MI command table if needed.  This will
-     cause the mi_command_py object to be deleted, which, in turn, will
-     clear the cmd->mi_command member variable, hence the assert.  */
-  if (cmd->mi_command != nullptr)
-    remove_mi_cmd_entry (cmd->mi_command->name ());
+  /* As the mi_command_py object holds a reference to the micmdpy_object,
+     the only way the dealloc function can be called is if he mi_command_py
+     object has been deleted, in which case the following assert will
+     hold.  */
   gdb_assert (cmd->mi_command == nullptr);
 
   /* Free the memory that holds the command name.  */