From 91e3d1d1a5462e49bbf70e0ff2fa3bf3a6a80503 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Thu, 22 Sep 2022 11:17:39 +0100 Subject: [PATCH] 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. 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 ) at ../../s> #10 0x0000000000df4380 in inferior::find_target_beneath (this=0x4934b50, t=0x2bda270 ) at ../.> #11 0x0000000000de2381 in target_ops::beneath (this=0x2bda270 ) at ../../src/gdb/target.c:3047 #12 0x0000000000de68aa in target_ops::supports_terminal_ours (this=0x2bda270 ) 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). --- gdb/target.c | 43 +++++---- gdb/target.h | 4 +- .../gdb.python/py-connection-removed.exp | 92 +++++++++++++++++++ 3 files changed, 118 insertions(+), 21 deletions(-) create mode 100644 gdb/testsuite/gdb.python/py-connection-removed.exp diff --git a/gdb/target.c b/gdb/target.c index 1dd0f42af7d..b3fd2345b78 100644 --- 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 28aa9273893..b9888f851cc 100644 --- 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 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 index 00000000000..6a0dbd17fe0 --- /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 . + +# 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" +} -- 2.30.2