public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gdb: fix target_ops reference count for some cases
@ 2022-09-21 13:12 Andrew Burgess
  2022-09-21 15:30 ` Simon Marchi
                   ` (2 more replies)
  0 siblings, 3 replies; 37+ messages in thread
From: Andrew Burgess @ 2022-09-21 13:12 UTC (permalink / raw)
  To: gdb-patches

This commit started as an investigation into why the test
gdb.python/py-inferior.exp crashes when GDB exits, leaving a core file
behind.

The crash occurs in connpy_connection_dealloc, and is actually
triggered by this assert:

  gdb_assert (conn_obj->target == nullptr);

Now a little aside...

... the assert is never actually printed, instead GDB crashes due to
calling a pure virtual function.  The backtrace at the point of crash
looks like this:

  #7  0x00007fef7e2cf747 in std::terminate() () from /lib64/libstdc++.so.6
  #8  0x00007fef7e2d0515 in __cxa_pure_virtual () from /lib64/libstdc++.so.6
  #9  0x0000000000de334d in target_stack::find_beneath (this=0x4934d78, t=0x2bda270 <the_dummy_target>) at ../../src/gdb/target.c:3606
  #10 0x0000000000df4380 in inferior::find_target_beneath (this=0x4934b50, t=0x2bda270 <the_dummy_target>) at ../../src/gdb/inferior.h:377
  #11 0x0000000000de2381 in target_ops::beneath (this=0x2bda270 <the_dummy_target>) at ../../src/gdb/target.c:3047
  #12 0x0000000000de68aa in target_ops::supports_terminal_ours (this=0x2bda270 <the_dummy_target>) at ../../src/gdb/target-delegates.c:1223
  #13 0x0000000000dde6b9 in target_supports_terminal_ours () at ../../src/gdb/target.c:1112
  #14 0x0000000000ee55f1 in internal_vproblem(internal_problem *, const char *, int, const char *, typedef __va_list_tag __va_list_tag *) (problem=0x2bdab00 <internal_error_problem>, file=0x198acf0 "../../src/gdb/python/py-connection.c", line=193, fmt=0x198ac9f "%s: Assertion `%s' failed.", ap=0x7ffdc26109d8) at ../../src/gdb/utils.c:379

Notice in frame #12 we called target_ops::supports_terminal_ours,
however, this is the_dummy_target, which is of type dummy_target, and
so we should have called dummy_target::supports_terminal_ours.  I
believe the reason we ended up in the wrong implementation of
supports_terminal_ours (which is a virtual function) is because we
made the call during GDB's shut-down, and, I suspect, the vtables were
in a weird state.

Anyway, the point of this patch is not to fix GDB's ability to print
an assert during exit, but to address the root cause of the assert.
With that aside out of the way, we can return to the main story...

Connections are represented in Python with gdb.TargtetConnection
objects (or its sub-classes).  The assert in question confirms that
when a gdb.TargtetConnection is deallocated, the underlying GDB
connection has itself been removed from GDB.  If this is not true then
we risk creating multiple different gdb.TargtetConnection objects for
the same connection, which would be bad.

When a connection removed in GDB the connection_removed observer
fires, which we catch with connpy_connection_removed, this function
then sets conn_obj->target to nullptr.

The first issue here is that connpy_connection_dealloc is being called
as part of GDB's exit code, which is run after the Python interpreter
has been shut down.  The connpy_connection_dealloc function is used to
deallocate the gdb.TargtetConnection Python object.  Surely it is
wrong for us to be deallocating Python objects after the interpreter
has been shut down.

The reason why connpy_connection_dealloc is called during GDB's exit
is that the global all_connection_objects map is holding a reference
to the gdb.TargtetConnection object.  When the map is destroyed during
GDB's exit, the gdb.TargtetConnection objects within the map can
finally be deallocated.

Another job of connpy_connection_removed (the function we mentioned
earlier) is to remove connections from the all_connection_objects map
when the connection is removed from GDB.

And so, the reason why all_connection_objects has contents when GDB
exits, and the reason the assert fires, is that, when GDB exits, there
are still some connections that have not yet been removed from GDB,
that is, they have a non-zero reference count.

If we take a look at quit_force (top.c) you can see that, for each
inferior, we call pop_all_targets before we (later in the function)
call do_final_cleanups.  It is the do_final_cleanups call that is
responsible for shutting down the Python interpreter.

So, in theory, we should have popped all targets be the time GDB
exits, this should have reduced their reference counts to zero, which
in turn should have triggered the connection_removed observer, and
resulted in the connection being removed from all_connection_objects,
and the gdb.TargtetConnection object being deallocated.

That this is not happening indicates that earlier, somewhere else in
GDB, we are leaking references to GDB's connections.

I tracked the problem down to the 'remove-inferiors' command,
implemented with the remove_inferior_command function (in inferior.c).
This function calls delete_inferior for each inferior the user
specifies.

In delete_inferior we do some house keeping, and then delete the
inferior object, which calls inferior::~inferior.

In neither delete_inferior or inferior::~inferior do we call
pop_all_targets, and it is this missing call that means we leak some
references to the target_ops objects on the inferior's target_stack.

To fix this we need to add a pop_all_targets call either in
delete_inferior or in inferior::~inferior.  Currently, I think that we
should place the call in delete_inferior.

Before calling pop_all_targets the inferior for which we are popping
needs to be made current, along with the program_space associated with
the inferior.

At the moment the inferior's program_space is deleted in
delete_inferior before we call inferior::~inferior, so, I think, to
place the pop_all_targets call into inferior::~inferior would require
additional adjustment to GDB.  As delete_inferior already exists, and
includes various house keeping tasks, it doesn't seem unreasonable to
place the pop_all_targets call there.

Now when I run py-inferior.exp, by the time GDB exits, the reference
counts are correct.  The final pop_all_targets calls in quit_force
reduce the reference counts to zero, which means the connections are
removed before the Python interpreter is shut down.  When GDB actually
exits the all_connection_objects map is empty, and no further Python
objects are deallocated at that point.  The test now exits cleanly
without creating a core file.

I've made some additional, related, changes in this commit.

In inferior::~inferior I've added a new assert that ensures, by the
time the inferior is destructed, the inferior's target stack is
empty (with the exception of the dummy_target).  If this is not true
then we will be loosing a reference to a target_ops object.

It is worth noting that we are loosing references to the dummy_target
object, however, I've not tried to fix that problem in this patch, as
I don't think it is as important.  The dummy target is a global
singleton, there's no observer for when the dummy target is deleted,
so no other parts of GDB care when the object is deleted.  As a global
it is always just deleted as part of the exit code, and we never
really care what its reference count is.  So, though it is a little
annoying that its reference count is wrong, it doesn't really matter.
Maybe I'll come back in a later patch and try to clean that up... but
that's for another day.

When I tested the changes above I ran into a failure from 'maint
selftest infrun_thread_ptid_changed'.

The problem is with scoped_mock_context.  This object creates a new
inferior (called mock_inferior), with a thread, and some other
associated state, and then select this new inferior.  We also push a
process_stratum_target sub-class onto the new inferior's target stack.

In ~scoped_mock_context we call:

  pop_all_targets_at_and_above (process_stratum);

this will remove all target_ops objects from the mock_inferior's
target stack, but leaves anything at the dummy_stratum and the
file_stratum (which I find a little weird, but more on this later).

The problem though is that pop_all_targets_at_and_above, just like
pop_all_targets, removes things from the target stack of the current
inferior.  In ~scoped_mock_context we don't ensure that the
mock_inferior associated with the current scoped_mock_context is
actually selected.

In most tests we create a single scoped_mock_context, which
automatically selects its contained mock_inferior.  However, in the
test infrun_thread_ptid_changed, we create multiple
scoped_mock_context, and then change which inferior is currently
selected.

As a result, in one case, we end up in ~scoped_mock_context with the
wrong inferior selected.  The pop_all_targets_at_and_above call then
removes the target_ops from the wrong inferior's target stack.  This
leaves the target_ops on the scoped_mock_context::mock_inferior's
target stack, and, when the mock_inferior is destructed, we loose
some references, this triggers the assert I placed in
inferior::~inferior.

To fix this I added a switch_to_inferior_no_thread call within the
~scoped_mock_context function.

As I mention above, it seems weird that we call
pop_all_targets_at_and_above instead of pop_all_targets, so I've
changed that.  I didn't see any test regressions after this, so I'm
assuming this is fine.

Finally, I've made use of the Python connection_removed event listener
API to add a test for this issue, but in addition the py-inferior.exp
test now runs without crashing (and creating a core file) on exit.
---
 gdb/inferior.c                                | 30 ++++++
 gdb/scoped-mock-context.h                     |  5 +-
 .../gdb.python/py-connection-removed.exp      | 92 +++++++++++++++++++
 3 files changed, 126 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.python/py-connection-removed.exp

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 7eb2bd97907..0c0b4b78ff5 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -70,6 +70,16 @@ inferior::~inferior ()
 {
   inferior *inf = this;
 
+  /* Before the inferior is deleted, all target_ops should be popped from
+     the target stack, this leaves just the dummy_target behind.  If this
+     is not done, then any target left in the target stack will be left
+     with an artificially high reference count.  As the dummy_target is
+     still on the target stack then we are about to loose a reference to
+     that target, leaving its reference count artificially high.  However,
+     this is not critical as the dummy_target is a singleton.  */
+  target_ops *ops = m_target_stack.top ();
+  gdb_assert (ops->stratum () == dummy_stratum);
+
   m_continuations.clear ();
   target_desc_info_free (inf->tdesc_info);
 }
@@ -191,6 +201,26 @@ delete_inferior (struct inferior *inf)
 
   gdb::observers::inferior_removed.notify (inf);
 
+  {
+    /* Limit the change of inferior to an inner scope so that the original
+       inferior and program space will have been restored by the time that
+       we delete the inferior INF and (possibly) its related  program
+       space.  */
+    scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
+    switch_to_inferior_no_thread (inf);
+    try
+      {
+	pop_all_targets ();
+      }
+    catch (const gdb_exception &ex)
+      {
+	/* If we get an exception then it's probably too late by this point
+	   to abandon deleting the inferior.  Instead, just print the
+	   exception and keep going.  */
+	exception_print (gdb_stderr, ex);
+      }
+  }
+
   /* If this program space is rendered useless, remove it. */
   if (inf->pspace->empty ())
     delete inf->pspace;
diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
index a9895303015..7240da9ef57 100644
--- a/gdb/scoped-mock-context.h
+++ b/gdb/scoped-mock-context.h
@@ -70,8 +70,11 @@ struct scoped_mock_context
 
   ~scoped_mock_context ()
   {
+    scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
+
     inferior_list.erase (inferior_list.iterator_to (mock_inferior));
-    pop_all_targets_at_and_above (process_stratum);
+    switch_to_inferior_no_thread (&mock_inferior);
+    pop_all_targets ();
   }
 };
 
diff --git a/gdb/testsuite/gdb.python/py-connection-removed.exp b/gdb/testsuite/gdb.python/py-connection-removed.exp
new file mode 100644
index 00000000000..1b139cedc0d
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-connection-removed.exp
@@ -0,0 +1,92 @@
+# Copyright (C) 2022 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/>.
+
+# Check that the gdb.connect_removed event triggers when we expect it
+# too.
+#
+# Checking this event has wider implications that simply some corner
+# of the Python API working or not.  The connection_removed event
+# triggers when the reference count of a process_stratum_target
+# reaches zero.  If these events stop triggering when expected then
+# GDB might be getting the reference counting on target_ops objects
+# wrong.
+
+load_lib gdb-python.exp
+
+standard_testfile py-connection.c
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+if ![runto_main] then {
+    return 0
+}
+
+# Register a callback that will trigger when a connection is removed
+# (deleted) within GDB.
+gdb_test_multiline "Add connection_removed event" \
+    "python" "" \
+    "def connection_removed_handler(event):" "" \
+    "   num = event.connection.num" "" \
+    "   type = event.connection.type" "" \
+    "   print(f'Connection {num} ({type}) removed')" "" \
+    "gdb.events.connection_removed.connect (connection_removed_handler)" "" \
+    "end" ""
+
+if { [target_info exists gdb_protocol] } {
+    if { [target_info gdb_protocol] == "extended-remote" } {
+	set connection_type "extended-remote"
+    } else {
+	set connection_type "remote"
+    }
+} else {
+    set connection_type "native"
+}
+
+# Add an inferior that shares a connection with inferior 1.
+gdb_test "add-inferior" "Added inferior 2 on connection 1 \[^\r\n\]+"
+
+# Add an inferior with no connection.
+gdb_test "add-inferior -no-connection" "Added inferior 3"
+
+# Kill the first inferior.  If we are using the plain 'remote' protocol then
+# it as this point that the remote target connection is removed.  For the
+# 'extended-remote' and 'native' targets the connection is removed later.
+if { $connection_type == "remote" } {
+    gdb_test "with confirm off -- kill" \
+	"Connection 1 \\(remote\\) removed\r\n.*" "kill inferior"
+} else {
+    gdb_test "with confirm off -- kill" "" "kill inferior"
+}
+
+# Switch to inferior 3 (the one with no connection).
+gdb_test "inferior 3"
+
+# Remove inferior 1, not expecting anything interesting at this point.
+gdb_test_no_output "remove-inferiors 1"
+
+# Now removed inferior 2.  For the 'remote' target the connection has
+# already been removed (see above), but for 'extended-remote' and 'native'
+# targets, it is at this point that the connection is removed.
+if { $connection_type == "remote" } {
+    gdb_test_no_output "remove-inferiors 2"
+} else {
+    gdb_test "remove-inferiors 2" \
+	"Connection 1 \\(${connection_type}\\) removed"
+}
-- 
2.25.4


^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, other threads:[~2022-12-14 13:57 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-21 13:12 [PATCH] gdb: fix target_ops reference count for some cases Andrew Burgess
2022-09-21 15:30 ` Simon Marchi
2022-09-22 14:21   ` Andrew Burgess
2022-09-22 14:52     ` Simon Marchi
2022-09-22 15:00 ` Simon Marchi
2022-09-22 17:24   ` Andrew Burgess
2022-09-26 14:16     ` Simon Marchi
2022-10-01 20:58       ` Andrew Burgess
2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
2022-10-02 17:04   ` [PATCHv2 1/7] gdb/remote: remove some manual reference count handling Andrew Burgess
2022-10-02 17:04   ` [PATCHv2 2/7] gdb: remove decref_target Andrew Burgess
2022-10-02 17:04   ` [PATCHv2 3/7] gdb: have target_stack automate reference count handling Andrew Burgess
2022-10-02 17:04   ` [PATCHv2 4/7] gdb: remove the pop_all_targets (and friends) global functions Andrew Burgess
2022-10-05 20:49     ` Lancelot SIX
2022-10-06 11:14       ` Andrew Burgess
2022-10-02 17:04   ` [PATCHv2 5/7] gdb: ensure all targets are popped before an inferior is destructed Andrew Burgess
2022-10-02 17:04   ` [PATCHv2 6/7] gdb/maint: add core file name to 'maint info program-spaces' output Andrew Burgess
2022-10-02 17:21     ` Eli Zaretskii
2022-10-02 17:04   ` [PATCHv2 7/7] gdb: some process_stratum_target should not be shared Andrew Burgess
2022-10-02 17:21     ` Eli Zaretskii
2022-10-05 21:15     ` Lancelot SIX
2022-10-06 11:44       ` Andrew Burgess
2022-11-18 16:42   ` [PATCHv3 0/7] gdb: fix target_ops reference count for some cases Andrew Burgess
2022-11-18 16:42     ` [PATCHv3 1/7] gdb/remote: remove some manual reference count handling Andrew Burgess
2022-11-18 16:42     ` [PATCHv3 2/7] gdb: remove decref_target Andrew Burgess
2022-11-18 17:22       ` Tom Tromey
2022-11-18 16:42     ` [PATCHv3 3/7] gdb: have target_stack automate reference count handling Andrew Burgess
2022-11-18 17:25       ` Tom Tromey
2022-11-18 16:42     ` [PATCHv3 4/7] gdb: remove the pop_all_targets (and friends) global functions Andrew Burgess
2022-11-18 17:29       ` Tom Tromey
2022-11-18 16:42     ` [PATCHv3 5/7] gdb: ensure all targets are popped before an inferior is destructed Andrew Burgess
2022-11-18 16:42     ` [PATCHv3 6/7] gdb/maint: add core file name to 'maint info program-spaces' output Andrew Burgess
2022-11-18 17:03       ` Eli Zaretskii
2022-11-18 16:42     ` [PATCHv3 7/7] gdb: some process_stratum_target should not be shared Andrew Burgess
2022-11-18 17:02       ` Eli Zaretskii
2022-11-18 18:04       ` Tom Tromey
2022-12-14 13:57         ` Andrew Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).