[2/3] gdb/python: deallocate tui window factories at Python shut down

Message ID 01961fb5bffc73841bc9782b819e19d2be5c039f.1673550880.git.aburgess@redhat.com
State Committed
Commit 9ae4519da9026cf1748a7e78783847de530de488
Headers
Series Python/TUI Window Creation / Destruction Fixes |

Commit Message

Andrew Burgess Jan. 12, 2023, 7:19 p.m. UTC
  The previous commit relied on detecting when a TUI window factory (as
defied in Python) was deleted.  I spotted that the window factories
are not deleted when GDB shuts down its Python environment.  Instead,
a reference to these window factories is retained, which forces the
Python object to remain live even after the Python interpreter itself
has been shut down.

We get away with this because the references themselves are held in a
dynamically allocated std::unordered_map (in tui/tui-layout.c) which
is never deallocated, thus the underlying Python references are never
decremented to zero.

This commit is the first half of the work to clean up this edge case.

All gdbpy_tui_window_maker objects, the objects which implement the
TUI window factory callback for Python defined TUI windows, are added
to a global list which lives in py-tui.c.  When GDB shuts down the
Python interpreter we now invalidate all of the existing
gdbpy_tui_window_maker objects (by releasing the reference to the
underlying Python object).  As a result, when GDB shuts down the
Python interpreter, all the existing TUI window factory Python objects
are deleted.

This commit does not update the std::unordered_map in tui-layout.c,
that will be done in the next commit.
---
 gdb/python/py-tui.c                           | 52 ++++++++++++++++++-
 gdb/python/python-internal.h                  |  1 +
 gdb/python/python.c                           |  1 +
 .../gdb.python/tui-window-factory.exp         | 32 ++++++++++++
 4 files changed, 84 insertions(+), 2 deletions(-)
  

Patch

diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index e9c91012ae9..9ce76659052 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -21,6 +21,7 @@ 
 #include "defs.h"
 #include "arch-utils.h"
 #include "python-internal.h"
+#include "gdbsupport/intrusive_list.h"
 
 #ifdef TUI
 
@@ -268,12 +269,14 @@  tui_py_window::output (const char *text, bool full_window)
    user-supplied window constructor.  */
 
 class gdbpy_tui_window_maker
+  : public intrusive_list_node<gdbpy_tui_window_maker>
 {
 public:
 
   explicit gdbpy_tui_window_maker (gdbpy_ref<> &&constr)
     : m_constr (std::move (constr))
   {
+    m_window_maker_list.push_back (*this);
   }
 
   ~gdbpy_tui_window_maker ();
@@ -281,12 +284,14 @@  class gdbpy_tui_window_maker
   gdbpy_tui_window_maker (gdbpy_tui_window_maker &&other) noexcept
     : m_constr (std::move (other.m_constr))
   {
+    m_window_maker_list.push_back (*this);
   }
 
   gdbpy_tui_window_maker (const gdbpy_tui_window_maker &other)
   {
     gdbpy_enter enter_py;
     m_constr = other.m_constr;
+    m_window_maker_list.push_back (*this);
   }
 
   gdbpy_tui_window_maker &operator= (gdbpy_tui_window_maker &&other)
@@ -304,16 +309,43 @@  class gdbpy_tui_window_maker
 
   tui_win_info *operator() (const char *name);
 
+  /* Reset the m_constr field of all gdbpy_tui_window_maker objects back to
+     nullptr, this will allow the Python object referenced to be
+     deallocated.  This function is intended to be called when GDB is
+     shutting down the Python interpreter to allow all Python objects to be
+     deallocated and cleaned up.  */
+  static void
+  invalidate_all ()
+  {
+    gdbpy_enter enter_py;
+    for (gdbpy_tui_window_maker &f : m_window_maker_list)
+      f.m_constr.reset (nullptr);
+  }
+
 private:
 
   /* A constructor that is called to make a TUI window.  */
   gdbpy_ref<> m_constr;
+
+  /* A global list of all gdbpy_tui_window_maker objects.  */
+  static intrusive_list<gdbpy_tui_window_maker> m_window_maker_list;
 };
 
+/* See comment in class declaration above.  */
+
+intrusive_list<gdbpy_tui_window_maker>
+  gdbpy_tui_window_maker::m_window_maker_list;
+
 gdbpy_tui_window_maker::~gdbpy_tui_window_maker ()
 {
-  gdbpy_enter enter_py;
-  m_constr.reset (nullptr);
+  /* Remove this gdbpy_tui_window_maker from the global list.  */
+  m_window_maker_list.erase (m_window_maker_list.iterator_to (*this));
+
+  if (m_constr != nullptr)
+    {
+      gdbpy_enter enter_py;
+      m_constr.reset (nullptr);
+    }
 }
 
 tui_win_info *
@@ -332,6 +364,14 @@  gdbpy_tui_window_maker::operator() (const char *win_name)
   std::unique_ptr<tui_py_window> window
     (new tui_py_window (win_name, wrapper));
 
+  /* There's only two ways that m_constr can be reset back to nullptr,
+     first when the parent gdbpy_tui_window_maker object is deleted, in
+     which case it should be impossible to call this method, or second, as
+     a result of a gdbpy_tui_window_maker::invalidate_all call, but this is
+     only called when GDB's Python interpreter is being shut down, after
+     which, this method should not be called.  */
+  gdb_assert (m_constr != nullptr);
+
   gdbpy_ref<> user_window
     (PyObject_CallFunctionObjArgs (m_constr.get (),
 				   (PyObject *) wrapper.get (),
@@ -572,3 +612,11 @@  gdbpy_initialize_tui ()
 
   return 0;
 }
+
+/* Finalize this module.  */
+
+void
+gdbpy_finalize_tui ()
+{
+  gdbpy_tui_window_maker::invalidate_all ();
+}
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index c41a43bac96..8fb09796b15 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -550,6 +550,7 @@  int gdbpy_initialize_unwind (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_tui ()
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
+void gdbpy_finalize_tui ();
 int gdbpy_initialize_membuf ()
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_connection ()
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 9d558119e09..3f5169251fc 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1932,6 +1932,7 @@  finalize_python (void *ignore)
   gdbpy_enter::finalize ();
 
   gdbpy_finalize_micommands ();
+  gdbpy_finalize_tui ();
 
   Py_Finalize ();
 
diff --git a/gdb/testsuite/gdb.python/tui-window-factory.exp b/gdb/testsuite/gdb.python/tui-window-factory.exp
index b2d6153c947..e18392b3f51 100644
--- a/gdb/testsuite/gdb.python/tui-window-factory.exp
+++ b/gdb/testsuite/gdb.python/tui-window-factory.exp
@@ -78,3 +78,35 @@  with_test_prefix "msg_3" {
     Term::check_box_contents "check test_window box" 0 0 80 15 \
 	"TestWindow \\(msg_3\\)"
 }
+
+# Restart GDB, setup a TUI window factory, and then check that the
+# Python object is deallocated when GDB exits.
+with_test_prefix "call __del__ at exit" {
+    clean_restart
+
+    gdb_test "source ${pyfile}" "Python script imported" \
+	"import python scripts"
+
+    gdb_test "python register_window_factory('msg_1')" \
+	"Entering TestWindowFactory\\.__init__: msg_1"
+
+    gdb_test "python register_window_factory('msg_2')" \
+	[multi_line \
+	     "Entering TestWindowFactory\\.__init__: msg_2" \
+	     "Entering TestWindowFactory\\.__del__: msg_1"]
+
+    set saw_window_factory_del 0
+    gdb_test_multiple "quit" "" {
+	-re "^quit\r\n" {
+	    exp_continue
+	}
+	-re "^Entering TestWindowFactory.__del__: msg_2\r\n" {
+	    incr saw_window_factory_del
+	    exp_continue
+	}
+	eof {
+	    gdb_assert { $saw_window_factory_del == 1 }
+	    pass $gdb_test_name
+	}
+    }
+}