From patchwork Fri Jan 27 16:34:04 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 63799 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0E5F138555A0 for ; Fri, 27 Jan 2023 16:34:45 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0E5F138555A0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1674837285; bh=SaUFpB9/W3xhr/OgLrlCy5fTs/ugl6vIKqz33WV/8/A=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=VX7dx+IDcYk4/4BAF1olinKKQGN4ePs6zDCvy8o3bIM+Da+YEbYzXH+WVmUXpFeJ+ 9gj7aJyMi56eF3bv+oMLUAfuOiSR0jACC1KsAqB6WktAwpcO/9yJQ/ZOi9XJmRQZtB GvQq39eHqUaKng8SPlxkEEgUoVJ9IqEPZ0NNBPkY= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 39B843857BB2 for ; Fri, 27 Jan 2023 16:34:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 39B843857BB2 Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-120-_krUrqiyP2agElQbo8schw-1; Fri, 27 Jan 2023 11:34:13 -0500 X-MC-Unique: _krUrqiyP2agElQbo8schw-1 Received: by mail-qk1-f197.google.com with SMTP id x12-20020a05620a258c00b007051ae500a2so3283665qko.15 for ; Fri, 27 Jan 2023 08:34:12 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=SaUFpB9/W3xhr/OgLrlCy5fTs/ugl6vIKqz33WV/8/A=; b=g0jMuGV95QfWFiQdCwnEvAjkkuACoX+ItozhthA4SI5jKJpYEzLMylj58NVqafd8xP qX1r4KNt514KIWnXdBZVMvkhOR7vhzjYjBC3Bz3EZruhD7EWOW/yd6XpUk8uY2Oz+uo6 g7XgXz5wOBrIVkdDcfEjW0XlxkKgBflN6Heg2rP5K4VR/0fxNobQr3mUI6+I+Xkm2ZJS ElYz3ajCeoa1xov81XZP9MG4xysv6nEMTHehwG45ZlIhu8YUA/PBw6mI1FPu9aqglNSh zwmhNq5akVTLpDN7Ljsty7rDfVYtaYmdJQXb8D7Wmx+W09sVuQK8NRWomVxPlY1e9VXj AS6Q== X-Gm-Message-State: AFqh2kqzpUscx9ymezkuIqU280JJZkzKLS/8P+Ek6hgnPsSwfypAPzIC c4fwoNA3+EIsWh23d3Z6/wtjMdoLPpaO3Cysf7sv207Fr5RWQo5hT8dlv0afUfQNeGBKlNqMw3V 3qZz3oqj9E7SOnpUZZErPvi2BdIqL9yEc0G9GEWoLqRWaTl7MKjU4yACt6+F5kjT+/1cmygJn2w == X-Received: by 2002:ac8:5b90:0:b0:3b1:5df1:9167 with SMTP id a16-20020ac85b90000000b003b15df19167mr80694406qta.19.1674837252327; Fri, 27 Jan 2023 08:34:12 -0800 (PST) X-Google-Smtp-Source: AMrXdXvjjQ/+tCbOuzjJUYTH7RTNDt0iky9jLTDm7pwi7BEjZViXXgkKIUZ/Rg5TPjEair+sL/5UMg== X-Received: by 2002:ac8:5b90:0:b0:3b1:5df1:9167 with SMTP id a16-20020ac85b90000000b003b15df19167mr80694365qta.19.1674837251967; Fri, 27 Jan 2023 08:34:11 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id jt9-20020a05622aa00900b003b80fdaa14dsm2930112qtb.73.2023.01.27.08.34.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 27 Jan 2023 08:34:11 -0800 (PST) To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCHv3 2/3] gdb/python: deallocate tui window factories at Python shut down Date: Fri, 27 Jan 2023 16:34:04 +0000 Message-Id: X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Andrew Burgess via Gdb-patches From: Andrew Burgess Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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 { 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 m_window_maker_list; }; +/* See comment in class declaration above. */ + +intrusive_list + 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 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 + } + } +}