* [PATCHv2 1/3] gdb/python: allow Python TUI windows to be replaced
2023-01-25 13:56 ` [PATCHv2 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
@ 2023-01-25 13:56 ` Andrew Burgess
2023-01-25 13:56 ` [PATCHv2 2/3] gdb/python: deallocate tui window factories at Python shut down Andrew Burgess
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Andrew Burgess @ 2023-01-25 13:56 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
The documentation for gdb.register_window_type says:
"... It's an error to try to replace one of the built-in windows,
but other window types can be replaced. ..."
I take this to mean that if I imported a Python script like this:
gdb.register_window_type('my_window', FactoryFunction)
Then GDB would have a new TUI window 'my_window', which could be
created by calling FactoryFunction(). If I then, in the same GDB
session imported a script which included:
gdb.register_window_type('my_window', UpdatedFactoryFunction)
Then GDB would replace the old 'my_window' factory with my new one,
GDB would now call UpdatedFactoryFunction().
This is pretty useful in practice, as it allows users to iterate on
their window implementation within a single GDB session.
However, right now, this is not how GDB operates. The second call to
register_window_type is basically ignored and the old window factory
is retained.
This is because in tui_register_window (tui/tui-layout.c) we use
std::unordered_map::emplace to insert the new factory function, and
emplace doesn't replace an existing element in an unordered_map.
In this commit, before the emplace call, I now search for an already
existing element, and delete any matching element from the map, the
emplace call will then add the new factory function.
---
.../gdb.python/tui-window-factory.exp | 73 +++++++++++++++++++
.../gdb.python/tui-window-factory.py | 48 ++++++++++++
gdb/tui/tui-layout.c | 8 ++
3 files changed, 129 insertions(+)
create mode 100644 gdb/testsuite/gdb.python/tui-window-factory.exp
create mode 100644 gdb/testsuite/gdb.python/tui-window-factory.py
diff --git a/gdb/testsuite/gdb.python/tui-window-factory.exp b/gdb/testsuite/gdb.python/tui-window-factory.exp
new file mode 100644
index 00000000000..99f9fbb1bc4
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-factory.exp
@@ -0,0 +1,73 @@
+# Copyright (C) 2023 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test that GDB correctly deallocates the window factory object (a)
+# when a window factory is replaced, and (b) during GDB shutdown.
+#
+# This test also ensures that when a new window is registered (via the
+# Python API) with the same name as an existing window, then the
+# previous window is replaced.
+
+load_lib gdb-python.exp
+
+tuiterm_env
+
+clean_restart
+
+require allow_tui_tests allow_python_tests
+
+set pyfile [gdb_remote_download host \
+ ${srcdir}/${subdir}/${gdb_test_file_name}.py]
+
+Term::clean_restart 24 80
+Term::prepare_for_tui
+
+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"]
+
+gdb_test_no_output "tui new-layout test test_window 1 cmd 1 status 1"
+
+# Load the custom window layout and ensure that the correct window
+# factory was used.
+with_test_prefix "msg_2" {
+ Term::command_no_prompt_prefix "layout test"
+ Term::check_box_contents "check test_window box" 0 0 80 15 \
+ "TestWindow \\(msg_2\\)"
+}
+
+# Replace the existing window factory with a new one, then switch
+# layouts so that GDB recreates the window, and check that the new
+# window factory was used.
+with_test_prefix "msg_3" {
+ Term::command "python register_window_factory('msg_3')"
+ Term::check_region_contents "check for python output" \
+ 0 18 80 2 \
+ [multi_line \
+ "Entering TestWindowFactory.__init__: msg_3\\s+" \
+ "Entering TestWindowFactory.__del__: msg_2"]
+ Term::command "layout src"
+ Term::command "layout test"
+
+ Term::check_box_contents "check test_window box" 0 0 80 15 \
+ "TestWindow \\(msg_3\\)"
+}
diff --git a/gdb/testsuite/gdb.python/tui-window-factory.py b/gdb/testsuite/gdb.python/tui-window-factory.py
new file mode 100644
index 00000000000..d1254e7e3a0
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-factory.py
@@ -0,0 +1,48 @@
+# Copyright (C) 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+
+class TestWindow:
+ def __init__(self, tui_win, msg):
+ self.msg = msg
+ self.tui_win = tui_win
+ print("Entering TestWindow.__init__: %s" % self.msg)
+
+ def render(self):
+ self.tui_win.erase()
+ self.tui_win.write("TestWindow (%s)" % self.msg)
+
+ def __del__(self):
+ print("Entering TestWindow.__del__: %s" % self.msg)
+
+
+class TestWindowFactory:
+ def __init__(self, msg):
+ self.msg = msg
+ print("Entering TestWindowFactory.__init__: %s" % self.msg)
+
+ def __call__(self, tui_win):
+ print("Entering TestWindowFactory.__call__: %s" % self.msg)
+ return TestWindow(tui_win, self.msg)
+
+ def __del__(self):
+ print("Entering TestWindowFactory.__del__: %s" % self.msg)
+
+
+def register_window_factory(msg):
+ gdb.register_window_type("test_window", TestWindowFactory(msg))
+
+
+print("Python script imported")
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index 27abee02087..b895e00a80d 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -418,6 +418,14 @@ tui_register_window (const char *name, window_factory &&factory)
if (!ISALPHA (name_copy[0]))
error (_("window name must start with a letter, not '%c'"), name_copy[0]);
+ /* We already check above for all the builtin window names. If we get
+ this far then NAME must be a user defined window. Remove any existing
+ factory and replace it with this new version. */
+
+ auto iter = known_window_types->find (name);
+ if (iter != known_window_types->end ())
+ known_window_types->erase (iter);
+
known_window_types->emplace (std::move (name_copy),
std::move (factory));
}
--
2.25.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCHv2 2/3] gdb/python: deallocate tui window factories at Python shut down
2023-01-25 13:56 ` [PATCHv2 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
2023-01-25 13:56 ` [PATCHv2 1/3] gdb/python: allow Python TUI windows to be replaced Andrew Burgess
@ 2023-01-25 13:56 ` Andrew Burgess
2023-01-25 13:56 ` [PATCHv2 3/3] gdb/tui: don't leak the known_window_types map Andrew Burgess
2023-01-27 16:34 ` [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
3 siblings, 0 replies; 15+ messages in thread
From: Andrew Burgess @ 2023-01-25 13:56 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
The previous commit relied on spotting when a Python defined TUI
window factory was deleted. I spotted that the window factories are
not deleted when GDB shuts down its Python environment, they are only
deleted when one window factory replaces another. Consider this
example Python script:
class TestWindowFactory:
def __init__(self, msg):
self.msg = msg
print("Entering TestWindowFactory.__init__: %s" % self.msg)
def __call__(self, tui_win):
print("Entering TestWindowFactory.__call__: %s" % self.msg)
return TestWindow(tui_win, self.msg)
def __del__(self):
print("Entering TestWindowFactory.__del__: %s" % self.msg)
gdb.register_window_type("test_window", TestWindowFactory("A"))
gdb.register_window_type("test_window", TestWindowFactory("B"))
And this GDB session:
(gdb) source tui.py
Entering TestWindowFactory.__init__: A
Entering TestWindowFactory.__init__: B
Entering TestWindowFactory.__del__: B
(gdb) quit
Notice that when the 'B' window replaces the 'A' window we see the 'A'
object being deleted. But, when Python is shut down (after the
'quit') the 'B' object is never deleted.
Instead, GDB retains a reference to the window factory object, which
forces the Python object to remain live even after the Python
interpreter itself has been shut down.
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,
and so GDB never tries to delete these Python objects.
This commit is the first half of the work to clean up this edge case.
All gdbpy_tui_window_maker objects (the objects that implement the
TUI window factory callback for Python defined TUI windows), are now
linked together into a global list using the intrusive list mechanism.
When GDB shuts down the Python interpreter we can now walk this global
list and release the reference that is held to the underlying Python
object. By releasing this reference the Python object will now be
deleted.
I've added a new assert in gdbpy_tui_window_maker::operator(), this
will catch the case where we somehow end up in here after having
reset the reference to the underlying Python object. I don't think
this should ever happen though as we only clear the references when
shutting down the Python interpreter, and the ::operator() function is
only called when trying to apply a new TUI layout - something that
shouldn't happen while GDB itself is shutting down.
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(-)
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 54623f445ed..ed466cc4511 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1950,6 +1950,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 99f9fbb1bc4..3e898d01c7b 100644
--- a/gdb/testsuite/gdb.python/tui-window-factory.exp
+++ b/gdb/testsuite/gdb.python/tui-window-factory.exp
@@ -71,3 +71,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
+ }
+ }
+}
--
2.25.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCHv2 3/3] gdb/tui: don't leak the known_window_types map
2023-01-25 13:56 ` [PATCHv2 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
2023-01-25 13:56 ` [PATCHv2 1/3] gdb/python: allow Python TUI windows to be replaced Andrew Burgess
2023-01-25 13:56 ` [PATCHv2 2/3] gdb/python: deallocate tui window factories at Python shut down Andrew Burgess
@ 2023-01-25 13:56 ` Andrew Burgess
2023-01-27 16:34 ` [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
3 siblings, 0 replies; 15+ messages in thread
From: Andrew Burgess @ 2023-01-25 13:56 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
This commit finishes the task that was started in the previous
commit.
Now that all Python TUI window factories are correctly deleted when
the Python interpreter is shut down, we no longer need to dynamically
allocate the known_window_types map in tui-layout.c
This commit changes known_window_types to a statically allocated data
structure, removes the dynamic allocation from
initialize_known_windows, and then replaces lots of '->' with '.'
throughout this file.
There should be no user visible changes after this commit.
---
gdb/tui/tui-layout.c | 37 +++++++++++++++----------------------
1 file changed, 15 insertions(+), 22 deletions(-)
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index b895e00a80d..b65314ec30b 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -344,14 +344,9 @@ make_standard_window (const char *)
return tui_win_list[V];
}
-/* A map holding all the known window types, keyed by name. Note that
- this is heap-allocated and "leaked" at gdb exit. This avoids
- ordering issues with destroying elements in the map at shutdown.
- In particular, destroying this map can occur after Python has been
- shut down, causing crashes if any window destruction requires
- running Python code. */
+/* A map holding all the known window types, keyed by name. */
-static std::unordered_map<std::string, window_factory> *known_window_types;
+static std::unordered_map<std::string, window_factory> known_window_types;
/* Helper function that returns a TUI window, given its name. */
@@ -362,8 +357,8 @@ tui_get_window_by_name (const std::string &name)
if (name == window->name ())
return window;
- auto iter = known_window_types->find (name);
- if (iter == known_window_types->end ())
+ auto iter = known_window_types.find (name);
+ if (iter == known_window_types.end ())
error (_("Unknown window type \"%s\""), name.c_str ());
tui_win_info *result = iter->second (name.c_str ());
@@ -377,20 +372,18 @@ tui_get_window_by_name (const std::string &name)
static void
initialize_known_windows ()
{
- known_window_types = new std::unordered_map<std::string, window_factory>;
-
- known_window_types->emplace (SRC_NAME,
+ known_window_types.emplace (SRC_NAME,
make_standard_window<SRC_WIN,
tui_source_window>);
- known_window_types->emplace (CMD_NAME,
+ known_window_types.emplace (CMD_NAME,
make_standard_window<CMD_WIN, tui_cmd_window>);
- known_window_types->emplace (DATA_NAME,
+ known_window_types.emplace (DATA_NAME,
make_standard_window<DATA_WIN,
tui_data_window>);
- known_window_types->emplace (DISASSEM_NAME,
+ known_window_types.emplace (DISASSEM_NAME,
make_standard_window<DISASSEM_WIN,
tui_disasm_window>);
- known_window_types->emplace (STATUS_NAME,
+ known_window_types.emplace (STATUS_NAME,
make_standard_window<STATUS_WIN,
tui_locator_window>);
}
@@ -422,11 +415,11 @@ tui_register_window (const char *name, window_factory &&factory)
this far then NAME must be a user defined window. Remove any existing
factory and replace it with this new version. */
- auto iter = known_window_types->find (name);
- if (iter != known_window_types->end ())
- known_window_types->erase (iter);
+ auto iter = known_window_types.find (name);
+ if (iter != known_window_types.end ())
+ known_window_types.erase (iter);
- known_window_types->emplace (std::move (name_copy),
+ known_window_types.emplace (std::move (name_copy),
std::move (factory));
}
@@ -1207,8 +1200,8 @@ initialize_layouts ()
static bool
validate_window_name (const std::string &name)
{
- auto iter = known_window_types->find (name);
- return iter != known_window_types->end ();
+ auto iter = known_window_types.find (name);
+ return iter != known_window_types.end ();
}
/* Implementation of the "tui new-layout" command. */
--
2.25.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes
2023-01-25 13:56 ` [PATCHv2 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
` (2 preceding siblings ...)
2023-01-25 13:56 ` [PATCHv2 3/3] gdb/tui: don't leak the known_window_types map Andrew Burgess
@ 2023-01-27 16:34 ` Andrew Burgess
2023-01-27 16:34 ` [PATCHv3 1/3] gdb/python: allow Python TUI windows to be replaced Andrew Burgess
` (3 more replies)
3 siblings, 4 replies; 15+ messages in thread
From: Andrew Burgess @ 2023-01-27 16:34 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
This series conflicted with my other TUI series, that I just merged.
No real changes in v3, just resolved the merge conflicts.
Changes since v2:
- Rebased onto HEAD of upsstream, resolved merged conflicts in patch
#3.
Changes since v1:
- Rebased onto HEAD of upstream,
- Reworded the commit message for patch #2.
---
Andrew Burgess (3):
gdb/python: allow Python TUI windows to be replaced
gdb/python: deallocate tui window factories at Python shut down
gdb/tui: don't leak the known_window_types map
gdb/python/py-tui.c | 52 ++++++++-
gdb/python/python-internal.h | 1 +
gdb/python/python.c | 1 +
.../gdb.python/tui-window-factory.exp | 105 ++++++++++++++++++
.../gdb.python/tui-window-factory.py | 48 ++++++++
gdb/tui/tui-layout.c | 43 +++----
6 files changed, 227 insertions(+), 23 deletions(-)
create mode 100644 gdb/testsuite/gdb.python/tui-window-factory.exp
create mode 100644 gdb/testsuite/gdb.python/tui-window-factory.py
base-commit: 2d46b103a52e4597ee60aa224ef4e5fb225ba893
--
2.25.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCHv3 1/3] gdb/python: allow Python TUI windows to be replaced
2023-01-27 16:34 ` [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
@ 2023-01-27 16:34 ` Andrew Burgess
2023-01-27 16:34 ` [PATCHv3 2/3] gdb/python: deallocate tui window factories at Python shut down Andrew Burgess
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Andrew Burgess @ 2023-01-27 16:34 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
The documentation for gdb.register_window_type says:
"... It's an error to try to replace one of the built-in windows,
but other window types can be replaced. ..."
I take this to mean that if I imported a Python script like this:
gdb.register_window_type('my_window', FactoryFunction)
Then GDB would have a new TUI window 'my_window', which could be
created by calling FactoryFunction(). If I then, in the same GDB
session imported a script which included:
gdb.register_window_type('my_window', UpdatedFactoryFunction)
Then GDB would replace the old 'my_window' factory with my new one,
GDB would now call UpdatedFactoryFunction().
This is pretty useful in practice, as it allows users to iterate on
their window implementation within a single GDB session.
However, right now, this is not how GDB operates. The second call to
register_window_type is basically ignored and the old window factory
is retained.
This is because in tui_register_window (tui/tui-layout.c) we use
std::unordered_map::emplace to insert the new factory function, and
emplace doesn't replace an existing element in an unordered_map.
In this commit, before the emplace call, I now search for an already
existing element, and delete any matching element from the map, the
emplace call will then add the new factory function.
---
.../gdb.python/tui-window-factory.exp | 73 +++++++++++++++++++
.../gdb.python/tui-window-factory.py | 48 ++++++++++++
gdb/tui/tui-layout.c | 8 ++
3 files changed, 129 insertions(+)
create mode 100644 gdb/testsuite/gdb.python/tui-window-factory.exp
create mode 100644 gdb/testsuite/gdb.python/tui-window-factory.py
diff --git a/gdb/testsuite/gdb.python/tui-window-factory.exp b/gdb/testsuite/gdb.python/tui-window-factory.exp
new file mode 100644
index 00000000000..99f9fbb1bc4
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-factory.exp
@@ -0,0 +1,73 @@
+# Copyright (C) 2023 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test that GDB correctly deallocates the window factory object (a)
+# when a window factory is replaced, and (b) during GDB shutdown.
+#
+# This test also ensures that when a new window is registered (via the
+# Python API) with the same name as an existing window, then the
+# previous window is replaced.
+
+load_lib gdb-python.exp
+
+tuiterm_env
+
+clean_restart
+
+require allow_tui_tests allow_python_tests
+
+set pyfile [gdb_remote_download host \
+ ${srcdir}/${subdir}/${gdb_test_file_name}.py]
+
+Term::clean_restart 24 80
+Term::prepare_for_tui
+
+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"]
+
+gdb_test_no_output "tui new-layout test test_window 1 cmd 1 status 1"
+
+# Load the custom window layout and ensure that the correct window
+# factory was used.
+with_test_prefix "msg_2" {
+ Term::command_no_prompt_prefix "layout test"
+ Term::check_box_contents "check test_window box" 0 0 80 15 \
+ "TestWindow \\(msg_2\\)"
+}
+
+# Replace the existing window factory with a new one, then switch
+# layouts so that GDB recreates the window, and check that the new
+# window factory was used.
+with_test_prefix "msg_3" {
+ Term::command "python register_window_factory('msg_3')"
+ Term::check_region_contents "check for python output" \
+ 0 18 80 2 \
+ [multi_line \
+ "Entering TestWindowFactory.__init__: msg_3\\s+" \
+ "Entering TestWindowFactory.__del__: msg_2"]
+ Term::command "layout src"
+ Term::command "layout test"
+
+ Term::check_box_contents "check test_window box" 0 0 80 15 \
+ "TestWindow \\(msg_3\\)"
+}
diff --git a/gdb/testsuite/gdb.python/tui-window-factory.py b/gdb/testsuite/gdb.python/tui-window-factory.py
new file mode 100644
index 00000000000..d1254e7e3a0
--- /dev/null
+++ b/gdb/testsuite/gdb.python/tui-window-factory.py
@@ -0,0 +1,48 @@
+# Copyright (C) 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+
+class TestWindow:
+ def __init__(self, tui_win, msg):
+ self.msg = msg
+ self.tui_win = tui_win
+ print("Entering TestWindow.__init__: %s" % self.msg)
+
+ def render(self):
+ self.tui_win.erase()
+ self.tui_win.write("TestWindow (%s)" % self.msg)
+
+ def __del__(self):
+ print("Entering TestWindow.__del__: %s" % self.msg)
+
+
+class TestWindowFactory:
+ def __init__(self, msg):
+ self.msg = msg
+ print("Entering TestWindowFactory.__init__: %s" % self.msg)
+
+ def __call__(self, tui_win):
+ print("Entering TestWindowFactory.__call__: %s" % self.msg)
+ return TestWindow(tui_win, self.msg)
+
+ def __del__(self):
+ print("Entering TestWindowFactory.__del__: %s" % self.msg)
+
+
+def register_window_factory(msg):
+ gdb.register_window_type("test_window", TestWindowFactory(msg))
+
+
+print("Python script imported")
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index ecdcd4884a7..ba7ec89973e 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -427,6 +427,14 @@ tui_register_window (const char *name, window_factory &&factory)
if (!ISALPHA (name_copy[0]))
error (_("window name must start with a letter, not '%c'"), name_copy[0]);
+ /* We already check above for all the builtin window names. If we get
+ this far then NAME must be a user defined window. Remove any existing
+ factory and replace it with this new version. */
+
+ auto iter = known_window_types->find (name);
+ if (iter != known_window_types->end ())
+ known_window_types->erase (iter);
+
known_window_types->emplace (std::move (name_copy),
std::move (factory));
}
--
2.25.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCHv3 2/3] gdb/python: deallocate tui window factories at Python shut down
2023-01-27 16:34 ` [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
2023-01-27 16:34 ` [PATCHv3 1/3] gdb/python: allow Python TUI windows to be replaced Andrew Burgess
@ 2023-01-27 16:34 ` Andrew Burgess
2023-01-27 16:34 ` [PATCHv3 3/3] gdb/tui: don't leak the known_window_types map Andrew Burgess
2023-02-13 12:32 ` [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
3 siblings, 0 replies; 15+ messages in thread
From: Andrew Burgess @ 2023-01-27 16:34 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
The previous commit relied on spotting when a Python defined TUI
window factory was deleted. I spotted that the window factories are
not deleted when GDB shuts down its Python environment, they are only
deleted when one window factory replaces another. Consider this
example Python script:
class TestWindowFactory:
def __init__(self, msg):
self.msg = msg
print("Entering TestWindowFactory.__init__: %s" % self.msg)
def __call__(self, tui_win):
print("Entering TestWindowFactory.__call__: %s" % self.msg)
return TestWindow(tui_win, self.msg)
def __del__(self):
print("Entering TestWindowFactory.__del__: %s" % self.msg)
gdb.register_window_type("test_window", TestWindowFactory("A"))
gdb.register_window_type("test_window", TestWindowFactory("B"))
And this GDB session:
(gdb) source tui.py
Entering TestWindowFactory.__init__: A
Entering TestWindowFactory.__init__: B
Entering TestWindowFactory.__del__: B
(gdb) quit
Notice that when the 'B' window replaces the 'A' window we see the 'A'
object being deleted. But, when Python is shut down (after the
'quit') the 'B' object is never deleted.
Instead, GDB retains a reference to the window factory object, which
forces the Python object to remain live even after the Python
interpreter itself has been shut down.
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,
and so GDB never tries to delete these Python objects.
This commit is the first half of the work to clean up this edge case.
All gdbpy_tui_window_maker objects (the objects that implement the
TUI window factory callback for Python defined TUI windows), are now
linked together into a global list using the intrusive list mechanism.
When GDB shuts down the Python interpreter we can now walk this global
list and release the reference that is held to the underlying Python
object. By releasing this reference the Python object will now be
deleted.
I've added a new assert in gdbpy_tui_window_maker::operator(), this
will catch the case where we somehow end up in here after having
reset the reference to the underlying Python object. I don't think
this should ever happen though as we only clear the references when
shutting down the Python interpreter, and the ::operator() function is
only called when trying to apply a new TUI layout - something that
shouldn't happen while GDB itself is shutting down.
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(-)
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 54623f445ed..ed466cc4511 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1950,6 +1950,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 99f9fbb1bc4..3e898d01c7b 100644
--- a/gdb/testsuite/gdb.python/tui-window-factory.exp
+++ b/gdb/testsuite/gdb.python/tui-window-factory.exp
@@ -71,3 +71,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
+ }
+ }
+}
--
2.25.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCHv3 3/3] gdb/tui: don't leak the known_window_types map
2023-01-27 16:34 ` [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
2023-01-27 16:34 ` [PATCHv3 1/3] gdb/python: allow Python TUI windows to be replaced Andrew Burgess
2023-01-27 16:34 ` [PATCHv3 2/3] gdb/python: deallocate tui window factories at Python shut down Andrew Burgess
@ 2023-01-27 16:34 ` Andrew Burgess
2023-02-13 12:32 ` [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
3 siblings, 0 replies; 15+ messages in thread
From: Andrew Burgess @ 2023-01-27 16:34 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
This commit finishes the task that was started in the previous
commit.
Now that all Python TUI window factories are correctly deleted when
the Python interpreter is shut down, we no longer need to dynamically
allocate the known_window_types map in tui-layout.c
This commit changes known_window_types to a statically allocated data
structure, removes the dynamic allocation from
initialize_known_windows, and then replaces lots of '->' with '.'
throughout this file.
There should be no user visible changes after this commit.
---
gdb/tui/tui-layout.c | 41 +++++++++++++++++------------------------
1 file changed, 17 insertions(+), 24 deletions(-)
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index ba7ec89973e..bb8356c5544 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -343,22 +343,17 @@ make_standard_window (const char *)
return tui_win_list[V];
}
-/* A map holding all the known window types, keyed by name. Note that
- this is heap-allocated and "leaked" at gdb exit. This avoids
- ordering issues with destroying elements in the map at shutdown.
- In particular, destroying this map can occur after Python has been
- shut down, causing crashes if any window destruction requires
- running Python code. */
+/* A map holding all the known window types, keyed by name. */
-static window_types_map *known_window_types;
+static window_types_map known_window_types;
/* See tui-layout.h. */
known_window_names_range
all_known_window_names ()
{
- auto begin = known_window_names_iterator (known_window_types->begin ());
- auto end = known_window_names_iterator (known_window_types->end ());
+ auto begin = known_window_names_iterator (known_window_types.begin ());
+ auto end = known_window_names_iterator (known_window_types.end ());
return known_window_names_range (begin, end);
}
@@ -371,8 +366,8 @@ tui_get_window_by_name (const std::string &name)
if (name == window->name ())
return window;
- auto iter = known_window_types->find (name);
- if (iter == known_window_types->end ())
+ auto iter = known_window_types.find (name);
+ if (iter == known_window_types.end ())
error (_("Unknown window type \"%s\""), name.c_str ());
tui_win_info *result = iter->second (name.c_str ());
@@ -386,20 +381,18 @@ tui_get_window_by_name (const std::string &name)
static void
initialize_known_windows ()
{
- known_window_types = new window_types_map;
-
- known_window_types->emplace (SRC_NAME,
+ known_window_types.emplace (SRC_NAME,
make_standard_window<SRC_WIN,
tui_source_window>);
- known_window_types->emplace (CMD_NAME,
+ known_window_types.emplace (CMD_NAME,
make_standard_window<CMD_WIN, tui_cmd_window>);
- known_window_types->emplace (DATA_NAME,
+ known_window_types.emplace (DATA_NAME,
make_standard_window<DATA_WIN,
tui_data_window>);
- known_window_types->emplace (DISASSEM_NAME,
+ known_window_types.emplace (DISASSEM_NAME,
make_standard_window<DISASSEM_WIN,
tui_disasm_window>);
- known_window_types->emplace (STATUS_NAME,
+ known_window_types.emplace (STATUS_NAME,
make_standard_window<STATUS_WIN,
tui_locator_window>);
}
@@ -431,11 +424,11 @@ tui_register_window (const char *name, window_factory &&factory)
this far then NAME must be a user defined window. Remove any existing
factory and replace it with this new version. */
- auto iter = known_window_types->find (name);
- if (iter != known_window_types->end ())
- known_window_types->erase (iter);
+ auto iter = known_window_types.find (name);
+ if (iter != known_window_types.end ())
+ known_window_types.erase (iter);
- known_window_types->emplace (std::move (name_copy),
+ known_window_types.emplace (std::move (name_copy),
std::move (factory));
}
@@ -1216,8 +1209,8 @@ initialize_layouts ()
static bool
validate_window_name (const std::string &name)
{
- auto iter = known_window_types->find (name);
- return iter != known_window_types->end ();
+ auto iter = known_window_types.find (name);
+ return iter != known_window_types.end ();
}
/* Implementation of the "tui new-layout" command. */
--
2.25.4
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes
2023-01-27 16:34 ` [PATCHv3 0/3] Python/TUI Window Creation / Destruction Fixes Andrew Burgess
` (2 preceding siblings ...)
2023-01-27 16:34 ` [PATCHv3 3/3] gdb/tui: don't leak the known_window_types map Andrew Burgess
@ 2023-02-13 12:32 ` Andrew Burgess
2023-02-13 14:11 ` Tom Tromey
3 siblings, 1 reply; 15+ messages in thread
From: Andrew Burgess @ 2023-02-13 12:32 UTC (permalink / raw)
To: gdb-patches
Andrew Burgess <aburgess@redhat.com> writes:
> This series conflicted with my other TUI series, that I just merged.
> No real changes in v3, just resolved the merge conflicts.
>
> Changes since v2:
>
> - Rebased onto HEAD of upsstream, resolved merged conflicts in patch
> #3.
>
> Changes since v1:
>
> - Rebased onto HEAD of upstream,
>
> - Reworded the commit message for patch #2.
>
Ping! If it helps at all, only patch #2 is really interesting.
Thanks,
Andrew
> ---
>
> Andrew Burgess (3):
> gdb/python: allow Python TUI windows to be replaced
> gdb/python: deallocate tui window factories at Python shut down
> gdb/tui: don't leak the known_window_types map
>
> gdb/python/py-tui.c | 52 ++++++++-
> gdb/python/python-internal.h | 1 +
> gdb/python/python.c | 1 +
> .../gdb.python/tui-window-factory.exp | 105 ++++++++++++++++++
> .../gdb.python/tui-window-factory.py | 48 ++++++++
> gdb/tui/tui-layout.c | 43 +++----
> 6 files changed, 227 insertions(+), 23 deletions(-)
> create mode 100644 gdb/testsuite/gdb.python/tui-window-factory.exp
> create mode 100644 gdb/testsuite/gdb.python/tui-window-factory.py
>
>
> base-commit: 2d46b103a52e4597ee60aa224ef4e5fb225ba893
> --
> 2.25.4
^ permalink raw reply [flat|nested] 15+ messages in thread