gdb: have target_stack automate reference count handling

This commit changes the target_stack class from using a C style array
of 'target_ops *' to using a C++ std::array<target_ops_ref, ...>.  The
benefit of this change is that some of the reference counting of
target_ops objects is now done automatically.

This commit fixes a crash in gdb.python/py-inferior.exp where GDB
crashes at exit, 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 ../../s>
      #10 0x0000000000df4380 in inferior::find_target_beneath (this=0x4934b50, t=0x2bda270 <the_dummy_target>) at ../.>
      #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/gd>
      #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_li>

    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.TargetConnection
objects (or its sub-classes).  The assert in question confirms that
when a gdb.TargetConnection is deallocated, the underlying GDB
connection has itself been removed from GDB.  If this is not true then
we risk creating multiple different gdb.TargetConnection objects for
the same connection, which would be bad.

To ensure that we have one gdb.TargetConnection object for each
connection, the all_connection_objects map exists, this maps the
process_stratum_target object (the connection) to the
gdb.TargetConnection object that represents the connection.

When a connection is removed in GDB the connection_removed observer
fires, which we catch with connpy_connection_removed, this function
then sets conn_obj->target to nullptr, and removes the corresponding
entry from the all_connection_objects map.

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.TargetConnection 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 still holding a
reference to the gdb.TargetConnection object.  When the map is
destroyed during GDB's exit, the gdb.TargetConnection objects within
the map can finally be deallocated.

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.  The
pop_all_targets calls should, in theory, cause all the connections to
be removed from GDB.

That this isn't working indicates that some targets have a non-zero
reference count even after this final pop_all_targets call, and
indeed, when I debug GDB, that is what I see.

I tracked the problem down to delete_inferior where 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.

In this commit I will provide a partial fix for the problem.  I say
partial fix, but this will actually be enough to resolve the crash.
In a later commit I will provide the final part of the fix.

As mentioned at the start of the commit message, this commit changes
the m_stack in target_stack to hold target_ops_ref objects.  This
means that when inferior::~inferior is called, and m_stack is
released, we automatically decrement the target_ops reference count.
With this change in place we no longer leak any references, and now,
in quit_force the final pop_all_targets calls will release the final
references.  This means that the targets will be correctly closed at
this point, which means the connections will be removed from GDB and
the Python objects deallocated before the Python interpreter shuts
down.

There's a slight oddity in target_stack::unpush, where we std::move
the reference out of m_stack like this:

  auto ref = std::move (m_stack[stratum]);

the `ref' isn't used explicitly, but it serves to hold the
target_ops_ref until the end of the scope while allowing the m_stack
entry to be reset back to nullptr.  The alternative would be to
directly set the m_stack entry to nullptr, like this:

  m_stack[stratum] = nullptr;

The problem here is that when we set the m_stack entry to nullptr we
first decrement the target_ops reference count, and then set the array
entry to nullptr.

If the decrement means that the target_ops object reaches a zero
reference count then the target_ops object will be closed by calling
target_close.  In target_close we ensure that the target being closed
is not in any inferiors target_stack.

As we decrement before clearing, then this check in target_close will
fail, and an assert will trigger.

By using std::move to move the reference out of m_stack, this clears
the m_stack entry, meaning the inferior no longer contains the
target_ops in its target_stack.  Now when the REF object goes out of
scope and the reference count is decremented, target_close can run
successfully.

I've made use of the Python connection_removed listener API to add a
test for this issue.  The test installs a listener and then causes
delete_inferior to be called, we can then see that the connection is
then correctly removed (because the listener triggers).

diff --git a/gdb/target.c b/gdb/target.c
index 1dd0f42af7d3a916cea69c48b6af2ef8d0ba3cc5..b3fd2345b78e1541b54468635925e1382f2a64cc 100644 (file)
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1182,23 +1182,28 @@ decref_target (target_ops *t)
 void
 target_stack::push (target_ops *t)
 {
-  t->incref ();
+  /* We must create a new reference first.  It is possible that T is
+     already pushed on this target stack, in which case we will first
+     unpush it below, before re-pushing it.  If we don't increment the
+     reference count now, then when we unpush it, we might end up deleting
+     T, which is not good.  */
+  auto ref = target_ops_ref::new_reference (t);
 
   strata stratum = t->stratum ();
 
-  if (stratum == process_stratum)
-    connection_list_add (as_process_stratum_target (t));
-
   /* If there's already a target at this stratum, remove it.  */
 
-  if (m_stack[stratum] != NULL)
-    unpush (m_stack[stratum]);
+  if (m_stack[stratum].get () != nullptr)
+    unpush (m_stack[stratum].get ());
 
   /* Now add the new one.  */
-  m_stack[stratum] = t;
+  m_stack[stratum] = std::move (ref);
 
   if (m_top < stratum)
     m_top = stratum;
+
+  if (stratum == process_stratum)
+    connection_list_add (as_process_stratum_target (t));
 }
 
 /* See target.h.  */
@@ -1223,19 +1228,19 @@ target_stack::unpush (target_ops *t)
       return false;
     }
 
-  /* Unchain the target.  */
-  m_stack[stratum] = NULL;
-
   if (m_top == stratum)
     m_top = this->find_beneath (t)->stratum ();
 
-  /* Finally close the target, if there are no inferiors
-     referencing this target still.  Note we do this after unchaining,
-     so any target method calls from within the target_close
-     implementation don't end up in T anymore.  Do leave the target
-     open if we have are other inferiors referencing this target
-     still.  */
-  decref_target (t);
+  /* Move the target reference off the target stack, this sets the pointer
+     held in m_stack to nullptr, and places the reference in ref.  When
+     ref goes out of scope its reference count will be decremented, which
+     might cause the target to close.
+
+     We have to do it this way, and not just set the value in m_stack to
+     nullptr directly, because doing so would decrement the reference
+     count first, which might close the target, and closing the target
+     does a check that the target is not on any inferiors target_stack.  */
+  auto ref = std::move (m_stack[stratum]);
 
   return true;
 }
@@ -3612,8 +3617,8 @@ target_stack::find_beneath (const target_ops *t) const
 {
   /* Look for a non-empty slot at stratum levels beneath T's.  */
   for (int stratum = t->stratum () - 1; stratum >= 0; --stratum)
-    if (m_stack[stratum] != NULL)
-      return m_stack[stratum];
+    if (m_stack[stratum].get () != NULL)
+      return m_stack[stratum].get ();
 
   return NULL;
 }

diff --git a/gdb/target.h b/gdb/target.h
index 28aa92738930b1a6651b2a7784a3f7a4c9f9be85..b9888f851cc04f04007d063834deca391737e407 100644 (file)
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1389,7 +1389,7 @@ public:
   { return at (t->stratum ()) == t; }
 
   /* Return the target at STRATUM.  */
-  target_ops *at (strata stratum) const { return m_stack[stratum]; }
+  target_ops *at (strata stratum) const { return m_stack[stratum].get (); }
 
   /* Return the target at the top of the stack.  */
   target_ops *top () const { return at (m_top); }
@@ -1404,7 +1404,7 @@ private:
   /* The stack, represented as an array, with one slot per stratum.
      If no target is pushed at some stratum, the corresponding slot is
      null.  */
-  target_ops *m_stack[(int) debug_stratum + 1] {};
+  std::array<target_ops_ref, (int) debug_stratum + 1> m_stack;
 };
 
 /* Return the dummy target.  */

diff --git a/gdb/testsuite/gdb.python/py-connection-removed.exp b/gdb/testsuite/gdb.python/py-connection-removed.exp
new file mode 100644 (file)
index 0000000..6a0dbd1
--- /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('Connection %d (%s) removed' % (num, type))" "" \
+    "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"
+}