)]}'
{
  "commit": "91e3d1d1a5462e49bbf70e0ff2fa3bf3a6a80503",
  "tree": "217fedc2a38a86b1963d314aaf3b110e3c1df453",
  "parents": [
    "45664f164a0a46612d06e7a91b0b0454f670f8db"
  ],
  "author": {
    "name": "Andrew Burgess",
    "email": "aburgess@redhat.com",
    "time": "Thu Sep 22 11:17:39 2022 +0100"
  },
  "committer": {
    "name": "Andrew Burgess",
    "email": "aburgess@redhat.com",
    "time": "Wed Dec 14 13:57:21 2022 +0000"
  },
  "message": "gdb: have target_stack automate reference count handling\n\nThis commit changes the target_stack class from using a C style array\nof \u0027target_ops *\u0027 to using a C++ std::array\u003ctarget_ops_ref, ...\u003e.  The\nbenefit of this change is that some of the reference counting of\ntarget_ops objects is now done automatically.\n\nThis commit fixes a crash in gdb.python/py-inferior.exp where GDB\ncrashes at exit, leaving a core file behind.\n\nThe crash occurs in connpy_connection_dealloc, and is actually\ntriggered by this assert:\n\ngdb_assert (conn_obj-\u003etarget \u003d\u003d nullptr);\n\nNow a little aside...\n\n    ... the assert is never actually printed, instead GDB crashes due\n    to calling a pure virtual function.  The backtrace at the point of\n    crash looks like this:\n\n      #7  0x00007fef7e2cf747 in std::terminate() () from /lib64/libstdc++.so.6\n      #8  0x00007fef7e2d0515 in __cxa_pure_virtual () from /lib64/libstdc++.so.6\n      #9  0x0000000000de334d in target_stack::find_beneath (this\u003d0x4934d78, t\u003d0x2bda270 \u003cthe_dummy_target\u003e) at ../../s\u003e\n      #10 0x0000000000df4380 in inferior::find_target_beneath (this\u003d0x4934b50, t\u003d0x2bda270 \u003cthe_dummy_target\u003e) at ../.\u003e\n      #11 0x0000000000de2381 in target_ops::beneath (this\u003d0x2bda270 \u003cthe_dummy_target\u003e) at ../../src/gdb/target.c:3047\n      #12 0x0000000000de68aa in target_ops::supports_terminal_ours (this\u003d0x2bda270 \u003cthe_dummy_target\u003e) at ../../src/gd\u003e\n      #13 0x0000000000dde6b9 in target_supports_terminal_ours () at ../../src/gdb/target.c:1112\n      #14 0x0000000000ee55f1 in internal_vproblem(internal_problem *, const char *, int, const char *, typedef __va_li\u003e\n\n    Notice in frame #12 we called target_ops::supports_terminal_ours,\n    however, this is the_dummy_target, which is of type dummy_target,\n    and so we should have called dummy_target::supports_terminal_ours.\n    I believe the reason we ended up in the wrong implementation of\n    supports_terminal_ours (which is a virtual function) is because we\n    made the call during GDB\u0027s shut-down, and, I suspect, the vtables\n    were in a weird state.\n\n    Anyway, the point of this patch is not to fix GDB\u0027s ability to\n    print an assert during exit, but to address the root cause of the\n    assert.  With that aside out of the way, we can return to the main\n    story...\n\nConnections are represented in Python with gdb.TargetConnection\nobjects (or its sub-classes).  The assert in question confirms that\nwhen a gdb.TargetConnection is deallocated, the underlying GDB\nconnection has itself been removed from GDB.  If this is not true then\nwe risk creating multiple different gdb.TargetConnection objects for\nthe same connection, which would be bad.\n\nTo ensure that we have one gdb.TargetConnection object for each\nconnection, the all_connection_objects map exists, this maps the\nprocess_stratum_target object (the connection) to the\ngdb.TargetConnection object that represents the connection.\n\nWhen a connection is removed in GDB the connection_removed observer\nfires, which we catch with connpy_connection_removed, this function\nthen sets conn_obj-\u003etarget to nullptr, and removes the corresponding\nentry from the all_connection_objects map.\n\nThe first issue here is that connpy_connection_dealloc is being called\nas part of GDB\u0027s exit code, which is run after the Python interpreter\nhas been shut down.  The connpy_connection_dealloc function is used to\ndeallocate the gdb.TargetConnection Python object.  Surely it is\nwrong for us to be deallocating Python objects after the interpreter\nhas been shut down.\n\nThe reason why connpy_connection_dealloc is called during GDB\u0027s exit\nis that the global all_connection_objects map is still holding a\nreference to the gdb.TargetConnection object.  When the map is\ndestroyed during GDB\u0027s exit, the gdb.TargetConnection objects within\nthe map can finally be deallocated.\n\nThe reason why all_connection_objects has contents when GDB exits, and\nthe reason the assert fires, is that, when GDB exits, there are still\nsome connections that have not yet been removed from GDB, that is,\nthey have a non-zero reference count.\n\nIf we take a look at quit_force (top.c) you can see that, for each\ninferior, we call pop_all_targets before we (later in the function)\ncall do_final_cleanups.  It is the do_final_cleanups call that is\nresponsible for shutting down the Python interpreter.  The\npop_all_targets calls should, in theory, cause all the connections to\nbe removed from GDB.\n\nThat this isn\u0027t working indicates that some targets have a non-zero\nreference count even after this final pop_all_targets call, and\nindeed, when I debug GDB, that is what I see.\n\nI tracked the problem down to delete_inferior where we do some house\nkeeping, and then delete the inferior object, which calls\ninferior::~inferior.\n\nIn neither delete_inferior or inferior::~inferior do we call\npop_all_targets, and it is this missing call that means we leak some\nreferences to the target_ops objects on the inferior\u0027s target_stack.\n\nIn this commit I will provide a partial fix for the problem.  I say\npartial fix, but this will actually be enough to resolve the crash.\nIn a later commit I will provide the final part of the fix.\n\nAs mentioned at the start of the commit message, this commit changes\nthe m_stack in target_stack to hold target_ops_ref objects.  This\nmeans that when inferior::~inferior is called, and m_stack is\nreleased, we automatically decrement the target_ops reference count.\nWith this change in place we no longer leak any references, and now,\nin quit_force the final pop_all_targets calls will release the final\nreferences.  This means that the targets will be correctly closed at\nthis point, which means the connections will be removed from GDB and\nthe Python objects deallocated before the Python interpreter shuts\ndown.\n\nThere\u0027s a slight oddity in target_stack::unpush, where we std::move\nthe reference out of m_stack like this:\n\n  auto ref \u003d std::move (m_stack[stratum]);\n\nthe `ref\u0027 isn\u0027t used explicitly, but it serves to hold the\ntarget_ops_ref until the end of the scope while allowing the m_stack\nentry to be reset back to nullptr.  The alternative would be to\ndirectly set the m_stack entry to nullptr, like this:\n\n  m_stack[stratum] \u003d nullptr;\n\nThe problem here is that when we set the m_stack entry to nullptr we\nfirst decrement the target_ops reference count, and then set the array\nentry to nullptr.\n\nIf the decrement means that the target_ops object reaches a zero\nreference count then the target_ops object will be closed by calling\ntarget_close.  In target_close we ensure that the target being closed\nis not in any inferiors target_stack.\n\nAs we decrement before clearing, then this check in target_close will\nfail, and an assert will trigger.\n\nBy using std::move to move the reference out of m_stack, this clears\nthe m_stack entry, meaning the inferior no longer contains the\ntarget_ops in its target_stack.  Now when the REF object goes out of\nscope and the reference count is decremented, target_close can run\nsuccessfully.\n\nI\u0027ve made use of the Python connection_removed listener API to add a\ntest for this issue.  The test installs a listener and then causes\ndelete_inferior to be called, we can then see that the connection is\nthen correctly removed (because the listener triggers).\n",
  "tree_diff": [
    {
      "type": "modify",
      "old_id": "1dd0f42af7d3a916cea69c48b6af2ef8d0ba3cc5",
      "old_mode": 33188,
      "old_path": "gdb/target.c",
      "new_id": "b3fd2345b78e1541b54468635925e1382f2a64cc",
      "new_mode": 33188,
      "new_path": "gdb/target.c"
    },
    {
      "type": "modify",
      "old_id": "28aa92738930b1a6651b2a7784a3f7a4c9f9be85",
      "old_mode": 33188,
      "old_path": "gdb/target.h",
      "new_id": "b9888f851cc04f04007d063834deca391737e407",
      "new_mode": 33188,
      "new_path": "gdb/target.h"
    },
    {
      "type": "add",
      "old_id": "0000000000000000000000000000000000000000",
      "old_mode": 0,
      "old_path": "/dev/null",
      "new_id": "6a0dbd17fe073a0725661c2833db97ceca7e2288",
      "new_mode": 33188,
      "new_path": "gdb/testsuite/gdb.python/py-connection-removed.exp"
    }
  ]
}