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

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

[Simon Marchi]

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.

I asked Andrew about the original intent of using a Python dictionary
object to hold the command objects.  The reason was to make sure the
objects get destroyed when the Python runtime gets finalized, not later.
Holding the objects in global C++ data structures and not doing anything
more means that the held Python objects will be decref'd after the
Python interpreter has been finalized.  That's not desirable.  I tried
it and it indeed segfaults.

Handle this by adding a gdbpy_finalize_micommands function called in
finalize_python.  This is the mirror of gdbpy_initialize_micommands
called in do_start_initialization.  In there, delete all Python MI
commands.  I think it makes sense to do it this way: if it was somehow
possible to unload Python support from GDB in the middle of a session
we'd want to unregister any Python MI command.  Otherwise, these MI
commands would be backed with a stale PyObject or simply nothing.

Delete tests that were related to `gdb._mi_commands`.

Change-Id: I060d5ebc7a096c67487998a8a4ca1e8e56f12cd3
---
 gdb/mi/mi-cmds.c                       |  14 ++
 gdb/mi/mi-cmds.h                       |   7 +
 gdb/python/lib/gdb/__init__.py         |   4 -
 gdb/python/py-micmd.c                  | 223 +++++++++----------------
 gdb/python/python-internal.h           |   1 +
 gdb/python/python.c                    |   2 +
 gdb/testsuite/gdb.python/py-mi-cmd.exp |  53 +-----
 7 files changed, 101 insertions(+), 203 deletions(-)

diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 60fec0a0b85..05714693023 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -128,6 +128,20 @@ remove_mi_cmd_entry (const std::string &name)
   return true;
 }
 
+/* See mi-cmds.h.  */
+
+void
+remove_mi_cmd_entries (remove_mi_cmd_entries_ftype callback)
+{
+  for (auto it = mi_cmd_table.cbegin (); it != mi_cmd_table.cend (); )
+    {
+      if (callback (it->second.get ()))
+	it = mi_cmd_table.erase (it);
+      else
+	++it;
+    }
+}
+
 /* Create and register a new MI command with an MI specific implementation.
    NAME must name an MI command that does not already exist, otherwise an
    assertion will trigger.  */
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 05b702f83ec..9ffb11bf997 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -22,6 +22,7 @@
 #ifndef MI_MI_CMDS_H
 #define MI_MI_CMDS_H
 
+#include "gdbsupport/function-view.h"
 #include "gdbsupport/gdb_optional.h"
 #include "mi/mi-main.h"
 
@@ -218,5 +219,11 @@ extern bool insert_mi_cmd_entry (mi_command_up command);
 
 extern bool remove_mi_cmd_entry (const std::string &name);
 
+/* Call CALLBACK for each registered MI command.  Remove commands for which
+   CALLBACK returns true.  */
+
+using remove_mi_cmd_entries_ftype
+  = gdb::function_view<bool (mi_command *)>;
+extern void remove_mi_cmd_entries (remove_mi_cmd_entries_ftype callback);
 
 #endif /* MI_MI_CMDS_H */
diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index 32945838775..5f63bced320 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -82,10 +82,6 @@ frame_filters = {}
 # Initial frame unwinders.
 frame_unwinders = []
 
-# Dictionary containing all user created MI commands, the key is the
-# command name, and the value is the gdb.MICommand object.
-_mi_commands = {}
-
 
 def _execute_unwinders(pending_frame):
     """Internal function called from GDB to execute all unwinders.
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index 399e89b0298..18a199de6b9 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -88,9 +88,10 @@ struct mi_command_py : public mi_command
 
   mi_command_py (const char *name, micmdpy_object *object)
     : mi_command (name, nullptr),
-      m_pyobj (object)
+      m_pyobj (gdbpy_ref<micmdpy_object>::new_reference (object))
   {
     pymicmd_debug_printf ("this = %p", this);
+    m_pyobj->mi_command = this;
   }
 
   ~mi_command_py ()
@@ -117,21 +118,42 @@ struct mi_command_py : public mi_command
      then always returns true.  */
   static void validate_installation (micmdpy_object *cmd_obj);
 
-  /* Update m_pyobj to NEW_PYOBJ.  The pointer from M_PYOBJ that points
+  /* Update M_PYOBJ to NEW_PYOBJ.  The pointer from M_PYOBJ that points
      back to this object is swapped with the pointer in NEW_PYOBJ, which
      must be nullptr, so that NEW_PYOBJ now points back to this object.
-     Additionally our parent's name string is stored in m_pyobj, so we
+     Additionally our parent's name string is stored in M_PYOBJ, so we
      swap the name string with NEW_PYOBJ.
 
-     Before this call m_pyobj is the Python object representing this MI
+     Before this call M_PYOBJ is the Python object representing this MI
      command object.  After this call has completed, NEW_PYOBJ now
      represents this MI command object.  */
   void swap_python_object (micmdpy_object *new_pyobj)
   {
+    /* Current object has a backlink, new object doesn't have a backlink.  */
+    gdb_assert (m_pyobj->mi_command != nullptr);
     gdb_assert (new_pyobj->mi_command == nullptr);
+
+    /* Clear the current M_PYOBJ's backlink, set NEW_PYOBJ's backlink.  */
     std::swap (new_pyobj->mi_command, m_pyobj->mi_command);
+
+    /* Both object have names.  */
+    gdb_assert (m_pyobj->mi_command_name != nullptr);
+    gdb_assert (new_pyobj->mi_command_name != nullptr);
+
+    /* mi_command::m_name is the string owned by the current object.  */
+    gdb_assert (m_pyobj->mi_command_name == this->name ());
+
+    /* The name in mi_command::m_name is owned by the current object.  Rather
+       than changing the value of mi_command::m_name (which is not accessible
+       from here) to point to the name owned by the new object, swap the names
+       of the two objects, since we know they are identical strings.  */
+    gdb_assert (strcmp (new_pyobj->mi_command_name,
+			m_pyobj->mi_command_name) == 0);
     std::swap (new_pyobj->mi_command_name, m_pyobj->mi_command_name);
-    m_pyobj = new_pyobj;
+
+    /* Take a reference to the new object, drop the reference to the current
+       object.  */
+    m_pyobj = gdbpy_ref<micmdpy_object>::new_reference (new_pyobj);
   }
 
   /* Called when the MI command is invoked.  */
@@ -139,9 +161,11 @@ struct mi_command_py : public mi_command
 
 private:
   /* The Python object representing this MI command.  */
-  micmdpy_object *m_pyobj;
+  gdbpy_ref<micmdpy_object> m_pyobj;
 };
 
+using mi_command_py_up = std::unique_ptr<mi_command_py>;
+
 extern PyTypeObject micmdpy_object_type
 	CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("micmdpy_object");
 
@@ -321,8 +345,6 @@ mi_command_py::invoke (struct mi_parse *parse) const
   if (parse->argv == nullptr)
     error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
 
-  PyObject *obj = (PyObject *) this->m_pyobj;
-  gdb_assert (obj != nullptr);
 
   gdbpy_enter enter_py;
 
@@ -341,9 +363,11 @@ mi_command_py::invoke (struct mi_parse *parse) const
 	gdbpy_handle_exception ();
     }
 
+  gdb_assert (this->m_pyobj != nullptr);
   gdb_assert (PyErr_Occurred () == nullptr);
-  gdbpy_ref<> result (PyObject_CallMethodObjArgs (obj, invoke_cst,
-						  argobj.get (), nullptr));
+  gdbpy_ref<> result
+    (PyObject_CallMethodObjArgs ((PyObject *) this->m_pyobj.get (), invoke_cst,
+				 argobj.get (), nullptr));
   if (result == nullptr)
     gdbpy_handle_exception ();
 
@@ -367,32 +391,13 @@ mi_command_py::validate_installation (micmdpy_object *cmd_obj)
   gdb_assert (cmd->m_pyobj == cmd_obj);
 }
 
-/* Return a reference to the gdb._mi_commands dictionary.  If the
-   dictionary can't be found for any reason then nullptr is returned, and
-   a Python exception will be set.  */
+/* Return CMD as an mi_command_py if it is a Python MI command, else
+   nullptr.  */
 
-static gdbpy_ref<>
-micmdpy_global_command_dictionary ()
+static mi_command_py *
+as_mi_command_py (mi_command *cmd)
 {
-  if (gdb_python_module == nullptr)
-    {
-      PyErr_SetString (PyExc_RuntimeError, _("unable to find gdb module"));
-      return nullptr;
-    }
-
-  gdbpy_ref<> mi_cmd_dict (PyObject_GetAttrString (gdb_python_module,
-						   "_mi_commands"));
-  if (mi_cmd_dict == nullptr)
-    return nullptr;
-
-  if (!PyDict_Check (mi_cmd_dict.get ()))
-    {
-      PyErr_SetString (PyExc_RuntimeError,
-		       _("gdb._mi_commands is not a dictionary as expected"));
-      return nullptr;
-    }
-
-  return mi_cmd_dict;
+  return dynamic_cast<mi_command_py *> (cmd);
 }
 
 /* Uninstall OBJ, making the MI command represented by OBJ unavailable for
@@ -409,41 +414,13 @@ micmdpy_uninstall_command (micmdpy_object *obj)
 
   pymicmd_debug_printf ("name = %s", obj->mi_command_name);
 
-  /* Remove the command from the internal MI table of commands, this will
-     cause the c++ object to be deleted, which will clear the mi_command
-     member variable within the Python object.  */
-  remove_mi_cmd_entry (obj->mi_command->name ());
+  /* Remove the command from the internal MI table of commands.  This will
+     cause the mi_command_py object to be deleted, which will clear the
+     backlink in OBJ.  */
+  bool removed = remove_mi_cmd_entry (obj->mi_command->name ());
+  gdb_assert (removed);
   gdb_assert (obj->mi_command == nullptr);
 
-  gdbpy_ref<> mi_cmd_dict = micmdpy_global_command_dictionary ();
-  if (mi_cmd_dict == nullptr)
-    return -1;
-
-  /* Grab the name for this command.  */
-  gdbpy_ref<> name_obj
-    = host_string_to_python_string (obj->mi_command_name);
-  if (name_obj == nullptr)
-    return -1;
-
-  /* Lookup the gdb.MICommand object in the dictionary of all Python MI
-     commands, this is gdb._mi_command, and remove it.  */
-  PyObject *curr = PyDict_GetItemWithError (mi_cmd_dict.get (),
-					    name_obj.get ());
-
-  /* Did we encounter an error?  Failing to find the object in the
-     dictionary isn't an error, that's fine.  */
-  if (curr == nullptr && PyErr_Occurred ())
-    return -1;
-
-  /* Did we find this command in the gdb._mi_commands dictionary?  If so,
-     then remove it.  */
-  if (curr != nullptr)
-    {
-      /* Yes we did!  Remove it.  */
-      if (PyDict_DelItem (mi_cmd_dict.get (), name_obj.get ()) < 0)
-	return -1;
-    }
-
   return 0;
 }
 
@@ -467,96 +444,36 @@ micmdpy_install_command (micmdpy_object *obj)
 
   pymicmd_debug_printf ("name = %s", obj->mi_command_name);
 
-  gdbpy_ref<> mi_cmd_dict = micmdpy_global_command_dictionary ();
-  if (mi_cmd_dict == nullptr)
-    return -1;
-
-  /* Look up this command name in the gdb._mi_commands dictionary, a
-     command with this name may already exist.  */
-  gdbpy_ref<> name_obj
-    = host_string_to_python_string (obj->mi_command_name);
+  /* Look up this command name in MI_COMMANDS, a command with this name may
+     already exist.  */
+  mi_command *cmd = mi_cmd_lookup (obj->mi_command_name);
+  mi_command_py *cmd_py = as_mi_command_py (cmd);
 
-  PyObject *curr = PyDict_GetItemWithError (mi_cmd_dict.get (),
-					    name_obj.get ());
-  if (curr == nullptr && PyErr_Occurred ())
-    return -1;
-  if (curr != nullptr)
+  if (cmd != nullptr && cmd_py == nullptr)
     {
-      /* There is a command with this name already in the gdb._mi_commands
-	 dictionary.  First, validate that the object in the dictionary is
-	 of the expected type, just in case something weird has happened.  */
-      if (!PyObject_IsInstance (curr, (PyObject *) &micmdpy_object_type))
-	{
-	  PyErr_SetString (PyExc_RuntimeError,
-			   _("unexpected object in gdb._mi_commands dictionary"));
-	  return -1;
-	}
-
-      /* To get to this function OBJ should not be installed, which should
-	 mean OBJ is not in the gdb._mi_commands dictionary.  If we find
-	 that OBJ is the thing in the dictionary, then something weird is
-	 going on, we should throw an error.  */
-      micmdpy_object *other = (micmdpy_object *) curr;
-      if (other == obj || other->mi_command == nullptr)
-	{
-	  PyErr_SetString (PyExc_RuntimeError,
-			   _("uninstalled command found in gdb._mi_commands dictionary"));
-	  return -1;
-	}
-
-      /* All Python mi command object should always have a name set.  */
-      gdb_assert (other->mi_command_name != nullptr);
-
-      /* We always insert commands into the gdb._mi_commands dictionary
-	 using their name as a key, if this check fails then the dictionary
-	 is in some weird state.  */
-      if (other->mi_command_name != other->mi_command->name ()
-	  || strcmp (other->mi_command_name, obj->mi_command_name) != 0)
-	{
-	  PyErr_SetString (PyExc_RuntimeError,
-			   _("gdb._mi_commands dictionary is corrupted"));
-	  return -1;
-	}
+      /* There is already an MI command registered with that name, and it's not
+         a Python one.  Forbid replacing a non-Python MI command.  */
+      PyErr_SetString (PyExc_RuntimeError,
+		       _("unable to add command, name is already in use"));
+      return -1;
+    }
 
-      /* Switch the state of the c++ object held in the MI command table
-	 so that it now references OBJ.  After this action the old Python
-	 object that used to be referenced from the MI command table will
-	 now show as uninstalled, while the new Python object will show as
-	 installed.  */
-      other->mi_command->swap_python_object (obj);
-
-      gdb_assert (other->mi_command == nullptr);
-      gdb_assert (obj->mi_command != nullptr);
-      gdb_assert (obj->mi_command->name () == obj->mi_command_name);
-
-      /* Remove the previous Python object from the gdb._mi_commands
-	 dictionary, we'll install the new object below.  */
-      if (PyDict_DelItem (mi_cmd_dict.get (), name_obj.get ()) < 0)
-       return -1;
+  if (cmd_py != nullptr)
+    {
+      /* There is already a Python MI command registered with that name, swap
+         in the new gdb.MICommand implementation.  */
+      cmd_py->swap_python_object (obj);
     }
   else
     {
-      /* There's no Python object for this command name in the
-	 gdb._mi_commands dictionary from which we can steal an existing
-	 object already held in the MI commands table, and so, we now
-	 create a new c++ object, and install it into the MI table.  */
-      obj->mi_command = new mi_command_py (obj->mi_command_name, obj);
-      mi_command_up micommand (obj->mi_command);
+      /* There's no MI command registered with that name at all, create one.  */
+      mi_command_py_up mi_cmd (new mi_command_py (obj->mi_command_name, obj));
 
       /* Add the command to the gdb internal MI command table.  */
-      bool result = insert_mi_cmd_entry (std::move (micommand));
-      if (!result)
-	{
-	  PyErr_SetString (PyExc_RuntimeError,
-			   _("unable to add command, name may already be in use"));
-	  return -1;
-	}
+      bool result = insert_mi_cmd_entry (std::move (mi_cmd));
+      gdb_assert (result);
     }
 
-  /* Finally, add the Python object to the gdb._mi_commands dictionary.  */
-  if (PyDict_SetItem (mi_cmd_dict.get (), name_obj.get (), (PyObject *) obj) < 0)
-    return -1;
-
   return 0;
 }
 
@@ -698,6 +615,18 @@ gdbpy_initialize_micommands ()
   return 0;
 }
 
+void
+gdbpy_finalize_micommands ()
+{
+  /* mi_command_py objects hold references to micmdpy_object objects.  They must
+     be dropped before the Python interpreter is finalized.  Do so by removing
+     those MI command entries, thus deleting the mi_command_py objects.  */
+  remove_mi_cmd_entries ([] (mi_command *cmd)
+    {
+      return as_mi_command_py (cmd) != nullptr;
+    });
+}
+
 /* Get the gdb.MICommand.name attribute, returns a string, the name of this
    MI command.  */
 
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 083c4dbdbc3..098d9130bf5 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -564,6 +564,7 @@ int gdbpy_initialize_connection ()
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_micommands (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
+void gdbpy_finalize_micommands ();
 
 /* A wrapper for PyErr_Fetch that handles reference counting for the
    caller.  */
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 97955881627..91636ef0225 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1795,6 +1795,8 @@ finalize_python (void *ignore)
   (void) PyGILState_Ensure ();
   gdbpy_enter::finalize ();
 
+  gdbpy_finalize_micommands ();
+
   Py_Finalize ();
 
   gdb_python_initialized = false;
diff --git a/gdb/testsuite/gdb.python/py-mi-cmd.exp b/gdb/testsuite/gdb.python/py-mi-cmd.exp
index c102efb4932..300ab956892 100644
--- a/gdb/testsuite/gdb.python/py-mi-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-mi-cmd.exp
@@ -289,31 +289,6 @@ mi_gdb_test "-aa" \
     ".*\\^done,zzz={msg=\"message three\"}" \
     "call the -aa command looking for message three"
 
-# Remove the gdb._mi_commands dictionary, then try to register a new
-# command.
-mi_gdb_test "python del(gdb._mi_commands)" ".*\\^done"
-mi_gdb_test "python pycmd3('-hello', 'Hello', 'msg')" \
-    [multi_line \
-	 ".*" \
-	 "&\"AttributeError: module 'gdb' has no attribute '_mi_commands'..\"" \
-	 "&\"Error while executing Python code\\...\"" \
-	 "\\^error,msg=\"Error while executing Python code\\.\""] \
-    "register a command with no gdb._mi_commands available"
-
-# Set gdb._mi_commands to be something other than a dictionary, and
-# try to register a command.
-mi_gdb_test "python gdb._mi_commands = 'string'" ".*\\^done"
-mi_gdb_test "python pycmd3('-hello', 'Hello', 'msg')" \
-    [multi_line \
-	 ".*" \
-	 "&\"RuntimeError: gdb._mi_commands is not a dictionary as expected..\"" \
-	 "&\"Error while executing Python code\\...\"" \
-	 "\\^error,msg=\"Error while executing Python code\\.\""] \
-    "register a command when gdb._mi_commands is not a dictionary"
-
-# Restore gdb._mi_commands to a dictionary.
-mi_gdb_test "python gdb._mi_commands = {}" ".*\\^done"
-
 # Try to register a command object that is missing an invoke method.
 # This is accepted, but will give an error when the user tries to run
 # the command.
@@ -346,37 +321,11 @@ mi_gdb_test "-hello" \
 	 "\\^error,msg=\"Error occurred in Python: 'str' object is not callable\""] \
     "execute command with invoke set to a string"
 
-# Further checking of corruption to the gdb._mi_commands dictionary.
-#
-# First, insert an object of the wrong type, then try to register an
-# MI command that will go into that same dictionary slot.
-mi_gdb_test "python gdb._mi_commands\['blah'\] = 'blah blah blah'" ".*\\^done"
-mi_gdb_test "python pycmd2('-blah')" \
-    [multi_line \
-	 ".*" \
-	 "&\"RuntimeError: unexpected object in gdb\\._mi_commands dictionary..\"" \
-	 "&\"Error while executing Python code\\...\"" \
-	 "\\^error,msg=\"Error while executing Python code\\.\""] \
-    "hit unexpected object in gdb._mi_commands dictionary"
-
-# Next, create a command, uninstall it, then force the command back
-# into the dictionary.
-mi_gdb_test "python cmd = pycmd2('-foo')" ".*\\^done"
-mi_gdb_test "python cmd.installed = False" ".*\\^done"
-mi_gdb_test "python gdb._mi_commands\['foo'\] = cmd" ".*\\^done"
-mi_gdb_test "python cmd.installed = True" \
-    [multi_line \
-	 ".*" \
-	 "&\"RuntimeError: uninstalled command found in gdb\\._mi_commands dictionary..\"" \
-	 "&\"Error while executing Python code\\...\"" \
-	 "\\^error,msg=\"Error while executing Python code\\.\""] \
-    "found uninstalled command in gdb._mi_commands dictionary"
-
 # Try to create a new MI command that uses the name of a builtin MI command.
 mi_gdb_test "python cmd = pycmd2('-data-disassemble')" \
     [multi_line \
 	 ".*" \
-	 "&\"RuntimeError: unable to add command, name may already be in use..\"" \
+	 "&\"RuntimeError: unable to add command, name is already in use..\"" \
 	 "&\"Error while executing Python code\\...\"" \
 	 "\\^error,msg=\"Error while executing Python code\\.\""] \
     "try to register a command that replaces -data-disassemble"
-- 
2.35.1

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

[Andrew Burgess]

* 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.  */

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

[Simon Marchi]

On 2022-03-18 18:44, Andrew Burgess via Gdb-patches wrote:
> * 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.  */
> 

Good catch.  Ammended and pushed, thanks!

Simon