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; 36+ 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] 36+ messages in thread

* Re: [PATCH] gdb: fix target_ops reference count for some cases
  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 15:00 ` Simon Marchi
  2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
  2 siblings, 1 reply; 36+ messages in thread
From: Simon Marchi @ 2022-09-21 15:30 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches



On 2022-09-21 09:12, Andrew Burgess via Gdb-patches wrote:
> 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

Missing "is".

> 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

Typo in "TargtetConnection".

> 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

be -> before?

> 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.

"TargtetConnection"

> 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.

Why does the inferior and program_space need to be made current in order
to pop the targets?  I understand that pop_all_targets_above and other
functions use `current_inferior`, but could we convert them (or add new
versions) so they don't?  Off-hand I don't see why they couldn't receive
the inferior as a parameter (or be made methods of inferior and/or
target_stack).

It shouldn't be important which inferior is the current one when calling
target_close on a target.  If we are closing a target, it means it is no
longer controlling any 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.

I don't object to fixing it like this.  I'm just wondering, did you
consider changing target_stack::m_stack to make it hold string
references, something like std::vector<target_ops_ref>?  I haven't tried
so maybe this doesn't make sense / is too difficult.  But if it does, I
guess the problem would take care of itself.  When deleting an inferior
that still has some targets pushed, they would be automatically decref'd
and closed if needed.

> 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.

Good catch.  Although, if that could be fixed by making
pop_all_targets_at_and_above not use the current_inferior, I think it
would be nicer.  And if the target stack could take care of managing the
refcount, as mentioned above, even nicer.

> 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.

Seems fine to me (this is essentially what a target stack holding
target_ops_refs would do).

Simon

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

* Re: [PATCH] gdb: fix target_ops reference count for some cases
  2022-09-21 15:30 ` Simon Marchi
@ 2022-09-22 14:21   ` Andrew Burgess
  2022-09-22 14:52     ` Simon Marchi
  0 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-09-22 14:21 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

Simon Marchi <simark@simark.ca> writes:

> On 2022-09-21 09:12, Andrew Burgess via Gdb-patches wrote:
>> 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
>
> Missing "is".
>
>> 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
>
> Typo in "TargtetConnection".
>
>> 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
>
> be -> before?
>
>> 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.
>
> "TargtetConnection"
>
>> 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.
>
> Why does the inferior and program_space need to be made current in order
> to pop the targets?  I understand that pop_all_targets_above and other
> functions use `current_inferior`, but could we convert them (or add new
> versions) so they don't?  Off-hand I don't see why they couldn't receive
> the inferior as a parameter (or be made methods of inferior and/or
> target_stack).
>
> It shouldn't be important which inferior is the current one when calling
> target_close on a target.  If we are closing a target, it means it is no
> longer controlling any inferior.

I agree with you 100%.  Unfortunately, the following targets all seem to
depend on current_inferior being set (in their ::close method):

  bsd_kvm_target
  core_target
  darwin_nat_target
  record_btrace_target
  ctf_target
  tfile_target
  windows_nat_target (though this is only for debug output)

I suspect that this means these targets only really work when GDB has a
single inferior maybe?  In most cases GDB seems to be clearing out some
per-inferior state relating to the target... I need to investigate more,
but I guess I wanted to raise this in case you (or anyone) had thoughts.


>
>> 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.
>
> I don't object to fixing it like this.  I'm just wondering, did you
> consider changing target_stack::m_stack to make it hold string
> references, something like std::vector<target_ops_ref>?  I haven't tried
> so maybe this doesn't make sense / is too difficult.  But if it does, I
> guess the problem would take care of itself.  When deleting an inferior
> that still has some targets pushed, they would be automatically decref'd
> and closed if needed.

I did think about this.  I think in the end the fix I proposed here
was just less churn.

I've revisited the idea of holding target_ops_ref objects, and I have
some patches that move GDB in that direction, though I haven't yet
figured out if we can get rid of the whole pop_all_targets API, which I
think is what you're hinting at.

>
>> 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.
>
> Good catch.  Although, if that could be fixed by making
> pop_all_targets_at_and_above not use the current_inferior, I think it
> would be nicer.  And if the target stack could take care of managing the
> refcount, as mentioned above, even nicer.

As I mention above, right now it seems we do need th correct inferior
selected, so we might need something like this, I'll see how my new
patches work out.

Thanks,
Andrew


>
>> 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.
>
> Seems fine to me (this is essentially what a target stack holding
> target_ops_refs would do).
>
> Simon


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

* Re: [PATCH] gdb: fix target_ops reference count for some cases
  2022-09-22 14:21   ` Andrew Burgess
@ 2022-09-22 14:52     ` Simon Marchi
  0 siblings, 0 replies; 36+ messages in thread
From: Simon Marchi @ 2022-09-22 14:52 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches



> I agree with you 100%.  Unfortunately, the following targets all seem to
> depend on current_inferior being set (in their ::close method):
> 
>   bsd_kvm_target
>   core_target
>   darwin_nat_target
>   record_btrace_target
>   ctf_target
>   tfile_target
>   windows_nat_target (though this is only for debug output)
> 
> I suspect that this means these targets only really work when GDB has a
> single inferior maybe?  In most cases GDB seems to be clearing out some
> per-inferior state relating to the target... I need to investigate more,
> but I guess I wanted to raise this in case you (or anyone) had thoughts.

Ah, ok.  Yeah I see some targets calling exit_inferior_silent on the
current inferior, among other things.  I guess I misunderstand what
target_ops::close is meant to do, I thought this would have been done
earlier.  Different targets seem to use the close method for different
things.

So, in any case, my proposition falls into "no doable right now".

>>> 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.
>>
>> I don't object to fixing it like this.  I'm just wondering, did you
>> consider changing target_stack::m_stack to make it hold string
>> references, something like std::vector<target_ops_ref>?  I haven't tried
>> so maybe this doesn't make sense / is too difficult.  But if it does, I
>> guess the problem would take care of itself.  When deleting an inferior
>> that still has some targets pushed, they would be automatically decref'd
>> and closed if needed.
> 
> I did think about this.  I think in the end the fix I proposed here
> was just less churn.

Agreed.

> As I mention above, right now it seems we do need th correct inferior
> selected, so we might need something like this, I'll see how my new
> patches work out.

I think your approach is ok, it fixes a real bug, so big refactorings
should not get in the way of that.

I'll take a look at the code now.

Simon

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

* Re: [PATCH] gdb: fix target_ops reference count for some cases
  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 15:00 ` Simon Marchi
  2022-09-22 17:24   ` Andrew Burgess
  2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
  2 siblings, 1 reply; 36+ messages in thread
From: Simon Marchi @ 2022-09-22 15:00 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches

> 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
loose -> lose?

> @@ -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

two spaces between related and program.

> +       space.  */
> +    scoped_restore_current_pspace_and_thread restore_pspace_and_thread;

Just wondering, why do we need to restore explicitly the current
pspace, instead of using just scoped_restore_current_thread?

scoped_restore_current_pspace_and_thread's doc says:

/* Save/restore the current program space, thread, inferior and frame.
   Use this when you need to call
   switch_to_program_space_and_thread.  */

... but you are not using switch_to_program_space_and_thread here.
Maybe it's ok and I just don't understand.  Same in
~scoped_mock_context.

> 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.

too -> to

> +#
> +# Checking this event has wider implications that simply some corner

simply -> imply?  Or I don't get what you mean.

> +# 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')" "" \

I think unfortunately need to support Python versions that don't know
about f-strings.

Simon


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

* Re: [PATCH] gdb: fix target_ops reference count for some cases
  2022-09-22 15:00 ` Simon Marchi
@ 2022-09-22 17:24   ` Andrew Burgess
  2022-09-26 14:16     ` Simon Marchi
  0 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-09-22 17:24 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

Simon Marchi <simark@simark.ca> writes:

>> 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
> loose -> lose?
>
>> @@ -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
>
> two spaces between related and program.
>
>> +       space.  */
>> +    scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
>
> Just wondering, why do we need to restore explicitly the current
> pspace, instead of using just scoped_restore_current_thread?
>
> scoped_restore_current_pspace_and_thread's doc says:
>
> /* Save/restore the current program space, thread, inferior and frame.
>    Use this when you need to call
>    switch_to_program_space_and_thread.  */
>
> ... but you are not using switch_to_program_space_and_thread here.
> Maybe it's ok and I just don't understand.  Same in
> ~scoped_mock_context.

I suspect the comment you quote is just out of date.

switch_to_program_space_and_thread can end up calling
switch_to_inferior_no_thread if there are no running threads in the
program space being switched too.  But, even if switch_to_thread does
end up being called we:

  - set the program space,
  - set the inferior,
  - set the current thread,
  - reinit the frame cache,

By comparison, switch_to_inferior_no_thread does:

  - sets the program space,
  - sets the inferior,
  - sets the current thread (to nullptr this time though),
  - reinits the frame cache,

As you can see they do the same set of things, all of which I think
should be reverted once we leave the scope, hence
scoped_restore_current_pspace_and_thread seems like the way to go.

>
>> 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.
>
> too -> to
>
>> +#
>> +# Checking this event has wider implications that simply some corner
>
> simply -> imply?  Or I don't get what you mean.
>
>> +# 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')" "" \
>
> I think unfortunately need to support Python versions that don't know
> about f-strings.

OK.

Thanks,
Andrew


>
> Simon


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

* Re: [PATCH] gdb: fix target_ops reference count for some cases
  2022-09-22 17:24   ` Andrew Burgess
@ 2022-09-26 14:16     ` Simon Marchi
  2022-10-01 20:58       ` Andrew Burgess
  0 siblings, 1 reply; 36+ messages in thread
From: Simon Marchi @ 2022-09-26 14:16 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches


>> Just wondering, why do we need to restore explicitly the current
>> pspace, instead of using just scoped_restore_current_thread?
>>
>> scoped_restore_current_pspace_and_thread's doc says:
>>
>> /* Save/restore the current program space, thread, inferior and frame.
>>    Use this when you need to call
>>    switch_to_program_space_and_thread.  */
>>
>> ... but you are not using switch_to_program_space_and_thread here.
>> Maybe it's ok and I just don't understand.  Same in
>> ~scoped_mock_context.
> 
> I suspect the comment you quote is just out of date.
> 
> switch_to_program_space_and_thread can end up calling
> switch_to_inferior_no_thread if there are no running threads in the
> program space being switched too.  But, even if switch_to_thread does
> end up being called we:
> 
>   - set the program space,
>   - set the inferior,
>   - set the current thread,
>   - reinit the frame cache,
> 
> By comparison, switch_to_inferior_no_thread does:
> 
>   - sets the program space,
>   - sets the inferior,
>   - sets the current thread (to nullptr this time though),
>   - reinits the frame cache,
> 
> As you can see they do the same set of things, all of which I think
> should be reverted once we leave the scope, hence
> scoped_restore_current_pspace_and_thread seems like the way to go.

Hmm okay, but won't scoped_restore_current_thread always restore the
pspace one way or another?  scoped_restore_current_thread::restore will
either call switch_to_thread or switch_to_inferior_no_thread, which both
end up setting the pspace.  I just don't understand why
scoped_restore_current_pspace_and_thread exists, it seems like
scoped_restore_current_thread would always work where
scoped_restore_current_pspace_and_thread is used.  I must be missing
something, scoped_restore_current_pspace_and_thread must have been added
for a reason.

Simon

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

* Re: [PATCH] gdb: fix target_ops reference count for some cases
  2022-09-26 14:16     ` Simon Marchi
@ 2022-10-01 20:58       ` Andrew Burgess
  0 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-01 20:58 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

Simon Marchi <simark@simark.ca> writes:

>>> Just wondering, why do we need to restore explicitly the current
>>> pspace, instead of using just scoped_restore_current_thread?
>>>
>>> scoped_restore_current_pspace_and_thread's doc says:
>>>
>>> /* Save/restore the current program space, thread, inferior and frame.
>>>    Use this when you need to call
>>>    switch_to_program_space_and_thread.  */
>>>
>>> ... but you are not using switch_to_program_space_and_thread here.
>>> Maybe it's ok and I just don't understand.  Same in
>>> ~scoped_mock_context.
>> 
>> I suspect the comment you quote is just out of date.
>> 
>> switch_to_program_space_and_thread can end up calling
>> switch_to_inferior_no_thread if there are no running threads in the
>> program space being switched too.  But, even if switch_to_thread does
>> end up being called we:
>> 
>>   - set the program space,
>>   - set the inferior,
>>   - set the current thread,
>>   - reinit the frame cache,
>> 
>> By comparison, switch_to_inferior_no_thread does:
>> 
>>   - sets the program space,
>>   - sets the inferior,
>>   - sets the current thread (to nullptr this time though),
>>   - reinits the frame cache,
>> 
>> As you can see they do the same set of things, all of which I think
>> should be reverted once we leave the scope, hence
>> scoped_restore_current_pspace_and_thread seems like the way to go.
>
> Hmm okay, but won't scoped_restore_current_thread always restore the
> pspace one way or another?  scoped_restore_current_thread::restore will
> either call switch_to_thread or switch_to_inferior_no_thread, which both
> end up setting the pspace.

That sounds completely correct.  So I did an experiment.  I added
~scoped_restore_current_pspace_and_thread, and with a little hacking,
had it check: is the program_space in m_restore_pspace the same
program_space as would be restored by m_restore_thread?

Turns out that it _isn't_ always the case!

Just to be clear on what this means: sometimes GDB is in a state where
the currently selected program_space is NOT the program_space for the
currently selected inferior!

One example of when this occurs is create_longjmp_master_breakpoint in
breakpoint.c, in here we do this:

  scoped_restore_current_program_space restore_pspace;

  for (struct program_space *pspace : program_spaces)
    {
      set_current_program_space (pspace);

      ...
    }

Obviously we enter this code with some inferior selected, but, within
the body of this loop we select each program_space in turn.  All but one
of these will be the wrong program_space for the current inferior.

This feels (to me) like a problem waiting to happen.  I wonder if we
should have some better approach here.  Some ideas would be:

  1. Change to always require switch_to_program_space_and_thread and
  remove set_current_program_space.  This would then require that we use
  scoped_restore_current_thread and remove
  scoped_restore_current_program_space.  This would mean GDB always had
  a sane thread/inferior/program_space combo selected,

  2. Allow GDB to have no inferior selected, then, in the above code,
  where (I guess) we are claiming that everything in the for loop only
  cares about the program_space, and not the inferior, we could
  temporarily switch to no inferior.  This would suggest adding a
  set_current_program_space_no_inferior function, and again, we would
  use scoped_restore_current_thread and remove
  scoped_restore_current_program_space from GDB,

  3. Embrace the mismatch and have scoped_restore_current_thread capture
  the current_program_space as well as the current thread and inferior.
  It would then restore the program space after restoring the
  thread/inferior, thus ensuring that we always restore the
  program_space correctly.  This would allow for
  scoped_restore_current_pspace_and_thread to be removed and just use
  scoped_restore_current_thread in all cases.

I rather like #2, but I worry that too much of GDB would just assume
there's always a current inferior, so I'm tempted to think #1 is
probably the best approach.  My concern is mostly the performance impact
of searching for a suitable inferior for a given program_space (this
currently involves a linear search through all inferiors).  With a low
number of inferiors this shouldn't be an issue, but maybe if we started
to deal with large numbers of inferiors we'd need to be smarter?

Ideally I think we shouldn't just live with the mismatch, though as a
short term stop-gap, maybe we could implement #3?

Thoughts?

Thanks,
Andrew


>                             I just don't understand why
> scoped_restore_current_pspace_and_thread exists, it seems like
> scoped_restore_current_thread would always work where
> scoped_restore_current_pspace_and_thread is used.  I must be missing
> something, scoped_restore_current_pspace_and_thread must have been added
> for a reason.
>
> Simon


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

* [PATCHv2 0/7] gdb: fix target_ops reference count for some cases
  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 15:00 ` Simon Marchi
@ 2022-10-02 17:04 ` Andrew Burgess
  2022-10-02 17:04   ` [PATCHv2 1/7] gdb/remote: remove some manual reference count handling Andrew Burgess
                     ` (7 more replies)
  2 siblings, 8 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-02 17:04 UTC (permalink / raw)
  To: gdb-patches

A complete rewrite (and expansion) of this patch after Simon's
feedback.

The original idea (from v1) is really covered by patches #3 and #5.
There's still (maybe) an open question about the use of
scoped_restore_pspace_and_thread vs scoped_restore_thread, but I hope
I've addressed this question in the v1 thread.  Do let me know if
there's still any questions on this though.

Patches #1 and #2 are target_ops ref-count related cleanups that I
spotted while working on this updated series.

Patch #4 is also a cleanup, moving some global functions to become
member functions on the inferior class.

Patch #6 is a completely random addition to add some extra maintenance
output, I ended up using this while debugging patch #7, but this
change isn't actually required at all.

Patch #7 does relate to target_ops, and target_ops sharing, which is
kind-of related to reference counting... maybe?  I can easily drop
this patch from the series if it turns out the idea in here is not
taking GDB in the right direction.

Thoughts,

Thanks,
Andrew

---

Andrew Burgess (7):
  gdb/remote: remove some manual reference count handling
  gdb: remove decref_target
  gdb: have target_stack automate reference count handling
  gdb: remove the pop_all_targets (and friends) global functions
  gdb: ensure all targets are popped before an inferior is destructed
  gdb/maint: add core file name to 'maint info program-spaces' output
  gdb: some process_stratum_target should not be shared

 gdb/NEWS                                      |  11 ++
 gdb/corelow.c                                 |   5 +
 gdb/doc/gdb.texinfo                           |  45 ++++-
 gdb/event-top.c                               |   3 +-
 gdb/inferior.c                                |  71 ++++++++
 gdb/inferior.h                                |  26 ++-
 gdb/progspace.c                               |  18 +-
 gdb/remote.c                                  |  40 +++--
 gdb/scoped-mock-context.h                     |   2 +-
 gdb/target.c                                  | 100 +++++------
 gdb/target.h                                  |  33 ++--
 gdb/testsuite/gdb.multi/multi-core-files-1.c  |  37 +++++
 gdb/testsuite/gdb.multi/multi-core-files-2.c  |  31 ++++
 gdb/testsuite/gdb.multi/multi-core-files.exp  | 156 ++++++++++++++++++
 .../gdb.python/py-connection-removed.exp      |  92 +++++++++++
 gdb/top.c                                     |   3 +-
 16 files changed, 563 insertions(+), 110 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-1.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-2.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files.exp
 create mode 100644 gdb/testsuite/gdb.python/py-connection-removed.exp

-- 
2.25.4


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

* [PATCHv2 1/7] gdb/remote: remove some manual reference count handling
  2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
@ 2022-10-02 17:04   ` Andrew Burgess
  2022-10-02 17:04   ` [PATCHv2 2/7] gdb: remove decref_target Andrew Burgess
                     ` (6 subsequent siblings)
  7 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-02 17:04 UTC (permalink / raw)
  To: gdb-patches

While working on some other target_ops reference count related code, I
spotted that in remote.c we do some manual reference count handling,
i.e. we call target_ops::incref and decref_target (which wraps
target_ops::decref).

I think it would be better to make use of gdb::ref_ptr to automate the
reference count management.

So, this commit updates scoped_mark_target_starting in two ways,
first, I use gdb::ref_ptr to handle the reference counts.  Then,
instead of using the scoped_mark_target_starting constructor and
destructor to set and reset the starting_up flag, I now use a
scoped_restore_tmpl object to set and restore the flag.

The above changes mean that the scoped_mark_target_starting destructor
can be completely removed, and the constructor body is now empty.

I've also fixed a typo in the class comment.

The only change in behaviour after this commit is that previously we
didn't care what the value of starting_up was, we just set it to true
in the constructor and false in the destructor.

Now, I assert that the flag is initially false, then set the flag true
when the scoped_mark_target_starting is created.

As the starting_up flag is initialized to false then, for the assert
to fire, we would need to recursively enter
remote_target::start_remote_1, which I don't think is something we
should be doing, so I think the new assert is an improvement.
---
 gdb/remote.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 2f6cb2d01ee..5a71c41d61e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4706,7 +4706,7 @@ remote_target::process_initial_stop_replies (int from_tty)
     }
 }
 
-/* Mark a remote_target as marking (by setting the starting_up flag within
+/* Mark a remote_target as starting (by setting the starting_up flag within
    its remote_state) for the lifetime of this object.  The reference count
    on the remote target is temporarily incremented, to prevent the target
    being deleted under our feet.  */
@@ -4716,26 +4716,32 @@ struct scoped_mark_target_starting
   /* Constructor, TARGET is the target to be marked as starting, its
      reference count will be incremented.  */
   scoped_mark_target_starting (remote_target *target)
-    : m_remote_target (target)
-  {
-    m_remote_target->incref ();
-    remote_state *rs = m_remote_target->get_remote_state ();
-    rs->starting_up = true;
-  }
+    : m_remote_target (remote_target_ref::new_reference (target)),
+      m_restore_starting_up (set_starting_up_flag (target))
+  { /* Nothing.  */ }
+
+private:
 
-  /* Destructor, mark the target being worked on as no longer starting, and
-     decrement the reference count.  */
-  ~scoped_mark_target_starting ()
+  /* Helper function, set the starting_up flag on TARGET and return an
+     object which, when it goes out of scope, will restore the previous
+     value of the starting_up flag.  */
+  static scoped_restore_tmpl<bool>
+  set_starting_up_flag (remote_target *target)
   {
-    remote_state *rs = m_remote_target->get_remote_state ();
-    rs->starting_up = false;
-    decref_target (m_remote_target);
+    remote_state *rs = target->get_remote_state ();
+    gdb_assert (!rs->starting_up);
+    return make_scoped_restore (&rs->starting_up, true);
   }
 
-private:
+  /* A gdb::ref_ptr pointer to a remote_target.  */
+  using remote_target_ref = gdb::ref_ptr<remote_target, target_ops_ref_policy>;
+
+  /* A reference to the target on which we are operating.  */
+  remote_target_ref m_remote_target;
 
-  /* The target on which we are operating.  */
-  remote_target *m_remote_target;
+  /* An object which restores the previous value of the starting_up flag
+     when it goes out of scope.  */
+  scoped_restore_tmpl<bool> m_restore_starting_up;
 };
 
 /* Helper for remote_target::start_remote, start the remote connection and
-- 
2.25.4


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

* [PATCHv2 2/7] gdb: remove decref_target
  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   ` Andrew Burgess
  2022-10-02 17:04   ` [PATCHv2 3/7] gdb: have target_stack automate reference count handling Andrew Burgess
                     ` (5 subsequent siblings)
  7 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-02 17:04 UTC (permalink / raw)
  To: gdb-patches

The decref_target function is not really needed.  Calling
target_ops::decref will just redirect to decref_target anyway, so why
not just rename decref_target to target_ops::decref?

That's what this commit does.

It's not exactly renaming to target_ops::decref, because the decref
functionality is handled by a policy class, so the new name is now
target_ops_ref_policy::decref.

There should be no user visible change after this commit.
---
 gdb/target.c |  2 +-
 gdb/target.h | 10 +++-------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 28560983625..9db44a83d55 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1158,7 +1158,7 @@ to_execution_direction must be implemented for reverse async");
 /* See target.h.  */
 
 void
-decref_target (target_ops *t)
+target_ops_ref_policy::decref (target_ops *t)
 {
   t->decref ();
   if (t->refcount () == 0)
diff --git a/gdb/target.h b/gdb/target.h
index 28aa9273893..0f5038fb9b2 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1337,9 +1337,6 @@ struct target_ops_deleter
 /* A unique pointer for target_ops.  */
 typedef std::unique_ptr<target_ops, target_ops_deleter> target_ops_up;
 
-/* Decref a target and close if, if there are no references left.  */
-extern void decref_target (target_ops *t);
-
 /* A policy class to interface gdb::ref_ptr with target_ops.  */
 
 struct target_ops_ref_policy
@@ -1349,10 +1346,9 @@ struct target_ops_ref_policy
     t->incref ();
   }
 
-  static void decref (target_ops *t)
-  {
-    decref_target (t);
-  }
+  /* Decrement the reference count on T, and, if the reference count
+     reaches zero, close the target.  */
+  static void decref (target_ops *t);
 };
 
 /* A gdb::ref_ptr pointer to a target_ops.  */
-- 
2.25.4


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

* [PATCHv2 3/7] gdb: have target_stack automate reference count handling
  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   ` Andrew Burgess
  2022-10-02 17:04   ` [PATCHv2 4/7] gdb: remove the pop_all_targets (and friends) global functions Andrew Burgess
                     ` (4 subsequent siblings)
  7 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-02 17:04 UTC (permalink / raw)
  To: gdb-patches

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.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.

To ensure that we have one gdb.TargtetConnection object for each
connection, the all_connection_objects map exists, this maps the
process_stratum_target object (the connection) to the
gdb.TargtetConnection 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.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 still 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.

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 9db44a83d55..0f4d6d01057 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1174,23 +1174,28 @@ target_ops_ref_policy::decref (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.  */
@@ -1216,19 +1221,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;
 }
@@ -3604,8 +3609,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 0f5038fb9b2..68446a39c1b 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1385,7 +1385,7 @@ class target_stack
   { 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); }
@@ -1400,7 +1400,7 @@ class target_stack
   /* 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
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 <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"
+}
-- 
2.25.4


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

* [PATCHv2 4/7] gdb: remove the pop_all_targets (and friends) global functions
  2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
                     ` (2 preceding siblings ...)
  2022-10-02 17:04   ` [PATCHv2 3/7] gdb: have target_stack automate reference count handling Andrew Burgess
@ 2022-10-02 17:04   ` Andrew Burgess
  2022-10-05 20:49     ` Lancelot SIX
  2022-10-02 17:04   ` [PATCHv2 5/7] gdb: ensure all targets are popped before an inferior is destructed Andrew Burgess
                     ` (3 subsequent siblings)
  7 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-10-02 17:04 UTC (permalink / raw)
  To: gdb-patches

This commit removes the global functions pop_all_targets,
pop_all_targets_above, and pop_all_targets_at_and_above, and makes
them methods on the inferior class.

As the pop_all_targets functions will unpush each target, which
decrements the targets reference count, it is possible that the target
might be closed.

Right now, closing a target, in some cases, depends on the current
inferior being set correctly, that is, to the inferior from which the
target was popped.

To facilitate this I have used switch_to_inferior_no_thread within the
new methods.  Previously it was the responsibility of the caller to
ensure that the correct inferior was selected.

In a couple of places (event-top.c and top.c) I have been able to
remove a previous switch_to_inferior_no_thread call.

In remote_unpush_target (remote.c) I have left the
switch_to_inferior_no_thread call as it is required for the
generic_mourn_inferior call.
---
 gdb/event-top.c           |  3 +--
 gdb/inferior.c            | 40 ++++++++++++++++++++++++++++++++++++++
 gdb/inferior.h            | 20 +++++++++++++++++++
 gdb/remote.c              |  2 +-
 gdb/scoped-mock-context.h |  2 +-
 gdb/target.c              | 41 +--------------------------------------
 gdb/target.h              | 11 -----------
 gdb/top.c                 |  3 +--
 8 files changed, 65 insertions(+), 57 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 88c53b720a9..836e6b935e3 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1293,10 +1293,9 @@ async_disconnect (gdb_client_data arg)
 
   for (inferior *inf : all_inferiors ())
     {
-      switch_to_inferior_no_thread (inf);
       try
 	{
-	  pop_all_targets ();
+	  inf->pop_all_targets ();
 	}
       catch (const gdb_exception &exception)
 	{
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 7eb2bd97907..2014bf926b7 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -103,6 +103,46 @@ inferior::unpush_target (struct target_ops *t)
   return m_target_stack.unpush (t);
 }
 
+/* See inferior.h.  */
+
+void inferior::unpush_target_and_assert (struct target_ops *target)
+{
+  gdb_assert (current_inferior () == this);
+
+  if (!unpush_target (target))
+    internal_error (__FILE__, __LINE__,
+		    "pop_all_targets couldn't find target %s\n",
+		    target->shortname ());
+}
+
+/* See inferior.h.  */
+
+void inferior::pop_all_targets_above (enum strata stratum)
+{
+  /* Unpushing a target might cause it to close.  Some targets currently
+     rely on the current_inferior being set for their ::close method, so we
+     temporarily switch inferior now.  */
+  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
+  switch_to_inferior_no_thread (this);
+
+  while ((int) (top_target ()->stratum ()) > (int) stratum)
+    unpush_target_and_assert (top_target ());
+}
+
+/* See inferior.h.  */
+
+void inferior::pop_all_targets_at_and_above (enum strata stratum)
+{
+  /* Unpushing a target might cause it to close.  Some targets currently
+     rely on the current_inferior being set for their ::close method, so we
+     temporarily switch inferior now.  */
+  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
+  switch_to_inferior_no_thread (this);
+
+  while ((int) (top_target ()->stratum ()) >= (int) stratum)
+    unpush_target_and_assert (top_target ());
+}
+
 void
 inferior::set_tty (std::string terminal_name)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index bb3aec1e8f8..344974c4811 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -380,6 +380,22 @@ class inferior : public refcounted_object,
   target_ops *top_target ()
   { return m_target_stack.top (); }
 
+  /* Unpush all targets except the dummy target from m_target_stack.  As
+     targets are removed from m_target_stack their reference count is
+     decremented, which may cause a target to close.  */
+  void pop_all_targets ()
+  { pop_all_targets_above (dummy_stratum); }
+
+  /* Unpush all targets above STRATUM from m_target_stack.  As targets are
+     removed from m_target_stack their reference count is decremented,
+     which may cause a target to close.  */
+  void pop_all_targets_above (enum strata stratum);
+
+  /* Unpush all targets at and above STRATUM from m_target_stack.  As
+     targets are removed from m_target_stack their reference count is
+     decremented, which may cause a target to close.  */
+  void pop_all_targets_at_and_above (enum strata stratum);
+
   /* Return the target at process_stratum level in this inferior's
      target stack.  */
   struct process_stratum_target *process_target ()
@@ -598,6 +614,10 @@ class inferior : public refcounted_object,
   registry<inferior> registry_fields;
 
 private:
+
+  /* Unpush TARGET and assert that it worked.  */
+  void unpush_target_and_assert (struct target_ops *target);
+
   /* The inferior's target stack.  */
   target_stack m_target_stack;
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 5a71c41d61e..4864e9a55c3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5721,7 +5721,7 @@ remote_unpush_target (remote_target *target)
   for (inferior *inf : all_inferiors (target))
     {
       switch_to_inferior_no_thread (inf);
-      pop_all_targets_at_and_above (process_stratum);
+      inf->pop_all_targets_at_and_above (process_stratum);
       generic_mourn_inferior ();
     }
 
diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
index a9895303015..87c1df0d206 100644
--- a/gdb/scoped-mock-context.h
+++ b/gdb/scoped-mock-context.h
@@ -71,7 +71,7 @@ struct scoped_mock_context
   ~scoped_mock_context ()
   {
     inferior_list.erase (inferior_list.iterator_to (mock_inferior));
-    pop_all_targets_at_and_above (process_stratum);
+    mock_inferior.pop_all_targets_at_and_above (process_stratum);
   }
 };
 
diff --git a/gdb/target.c b/gdb/target.c
index 0f4d6d01057..1e447f604d9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1238,45 +1238,6 @@ target_stack::unpush (target_ops *t)
   return true;
 }
 
-/* Unpush TARGET and assert that it worked.  */
-
-static void
-unpush_target_and_assert (struct target_ops *target)
-{
-  if (!current_inferior ()->unpush_target (target))
-    {
-      gdb_printf (gdb_stderr,
-		  "pop_all_targets couldn't find target %s\n",
-		  target->shortname ());
-      internal_error (__FILE__, __LINE__,
-		      _("failed internal consistency check"));
-    }
-}
-
-void
-pop_all_targets_above (enum strata above_stratum)
-{
-  while ((int) (current_inferior ()->top_target ()->stratum ())
-	 > (int) above_stratum)
-    unpush_target_and_assert (current_inferior ()->top_target ());
-}
-
-/* See target.h.  */
-
-void
-pop_all_targets_at_and_above (enum strata stratum)
-{
-  while ((int) (current_inferior ()->top_target ()->stratum ())
-	 >= (int) stratum)
-    unpush_target_and_assert (current_inferior ()->top_target ());
-}
-
-void
-pop_all_targets (void)
-{
-  pop_all_targets_above (dummy_stratum);
-}
-
 void
 target_unpusher::operator() (struct target_ops *ops) const
 {
@@ -2533,7 +2494,7 @@ target_preopen (int from_tty)
      it doesn't (which seems like a win for UDI), remove it now.  */
   /* Leave the exec target, though.  The user may be switching from a
      live process to a core of the same program.  */
-  pop_all_targets_above (file_stratum);
+  current_inferior ()->pop_all_targets_above (file_stratum);
 
   target_pre_inferior (from_tty);
 }
diff --git a/gdb/target.h b/gdb/target.h
index 68446a39c1b..547ee8a3bbd 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2389,17 +2389,6 @@ extern void target_pre_inferior (int);
 
 extern void target_preopen (int);
 
-/* Does whatever cleanup is required to get rid of all pushed targets.  */
-extern void pop_all_targets (void);
-
-/* Like pop_all_targets, but pops only targets whose stratum is at or
-   above STRATUM.  */
-extern void pop_all_targets_at_and_above (enum strata stratum);
-
-/* Like pop_all_targets, but pops only targets whose stratum is
-   strictly above ABOVE_STRATUM.  */
-extern void pop_all_targets_above (enum strata above_stratum);
-
 extern CORE_ADDR target_translate_tls_address (struct objfile *objfile,
 					       CORE_ADDR offset);
 
diff --git a/gdb/top.c b/gdb/top.c
index 54c7c922142..5f64c6bddf0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1845,10 +1845,9 @@ quit_force (int *exit_arg, int from_tty)
      them all out.  */
   for (inferior *inf : all_inferiors ())
     {
-      switch_to_inferior_no_thread (inf);
       try
 	{
-	  pop_all_targets ();
+	  inf->pop_all_targets ();
 	}
       catch (const gdb_exception &ex)
 	{
-- 
2.25.4


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

* [PATCHv2 5/7] gdb: ensure all targets are popped before an inferior is destructed
  2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
                     ` (3 preceding siblings ...)
  2022-10-02 17:04   ` [PATCHv2 4/7] gdb: remove the pop_all_targets (and friends) global functions Andrew Burgess
@ 2022-10-02 17:04   ` Andrew Burgess
  2022-10-02 17:04   ` [PATCHv2 6/7] gdb/maint: add core file name to 'maint info program-spaces' output Andrew Burgess
                     ` (2 subsequent siblings)
  7 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-02 17:04 UTC (permalink / raw)
  To: gdb-patches

Now that the inferiors target_stack automatically manages target
reference counts, we might think that we don't need to unpush targets
when an inferior is deleted...

...unfortunately that is not the case.  The inferior::unpush function
can do some work depending on the type of target, so it is important
that we still pass through this function.

To ensure that this is the case, in this commit I've added an assert
to inferior::~inferior that ensures the inferior's target_stack is
empty (except for the ever present dummy_target).

I've then added a pop_all_targets call to delete_inferior, otherwise
the new assert will fire in, e.g. the gdb.python/py-inferior.exp test.
---
 gdb/inferior.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 2014bf926b7..a498b9d493c 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -70,6 +70,15 @@ 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.  */
+  gdb_assert (m_target_stack.top ()->stratum () == dummy_stratum);
+
   m_continuations.clear ();
   target_desc_info_free (inf->tdesc_info);
 }
@@ -231,6 +240,12 @@ delete_inferior (struct inferior *inf)
 
   gdb::observers::inferior_removed.notify (inf);
 
+  /* Pop all targets now, this ensures that inferior::unpush is called
+     correctly.  As pop_all_targets ends up making a temporary switch to
+     inferior INF then we need to make this call before we delete the
+     program space, which we do below.  */
+  inf->pop_all_targets ();
+
   /* If this program space is rendered useless, remove it. */
   if (inf->pspace->empty ())
     delete inf->pspace;
-- 
2.25.4


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

* [PATCHv2 6/7] gdb/maint: add core file name to 'maint info program-spaces' output
  2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
                     ` (4 preceding siblings ...)
  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   ` 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-11-18 16:42   ` [PATCHv3 0/7] gdb: fix target_ops reference count for some cases Andrew Burgess
  7 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-10-02 17:04 UTC (permalink / raw)
  To: gdb-patches

Each program space can have an associated core file.  Include this
information in the output of 'maint info program-spaces'.
---
 gdb/NEWS            |  4 ++++
 gdb/doc/gdb.texinfo |  8 ++++++--
 gdb/progspace.c     | 18 ++++++++++++++++--
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 796a4ef8072..a6ea7c9f98f 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -128,6 +128,10 @@ maintenance info line-table
   entry corresponds to an address where a breakpoint should be placed
   to be at the first instruction past a function's prologue.
 
+maintenance info program-spaces
+  This command now includes a 'Core File' column which indicates the
+  name of the core file associated with each program space.
+
 * New targets
 
 GNU/Linux/LoongArch (gdbserver)	loongarch*-*-linux*
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 44d87e95748..107df84d108 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3465,6 +3465,10 @@
 the name of the executable loaded into the program space, with e.g.,
 the @code{file} command.
 
+@item
+the name of the core file loaded into the program space, with e.g.,
+the @code{core-file} command.
+
 @end enumerate
 
 @noindent
@@ -3477,7 +3481,7 @@
 
 @smallexample
 (@value{GDBP}) maint info program-spaces
-  Id   Executable
+  Id   Executable        Core File
 * 1    hello
   2    goodbye
         Bound inferiors: ID 1 (process 21561)
@@ -3491,7 +3495,7 @@
 
 @smallexample
 (@value{GDBP}) maint info program-spaces
-  Id   Executable
+  Id   Executable        Core File
 * 1    vfork-test
         Bound inferiors: ID 2 (process 18050), ID 1 (process 18045)
 @end smallexample
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 90003d964fe..4f58d44a0e6 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -251,22 +251,30 @@ print_program_space (struct ui_out *uiout, int requested)
 {
   int count = 0;
 
+  /* Start with a minimum width of 17 for the executable name column.  */
+  size_t longest_exec_name = 17;
+
   /* Compute number of pspaces we will print.  */
   for (struct program_space *pspace : program_spaces)
     {
       if (requested != -1 && pspace->num != requested)
 	continue;
 
+      if (pspace->exec_filename != nullptr)
+	longest_exec_name = std::max (strlen (pspace->exec_filename.get ()),
+				      longest_exec_name);
+
       ++count;
     }
 
   /* There should always be at least one.  */
   gdb_assert (count > 0);
 
-  ui_out_emit_table table_emitter (uiout, 3, count, "pspaces");
+  ui_out_emit_table table_emitter (uiout, 4, count, "pspaces");
   uiout->table_header (1, ui_left, "current", "");
   uiout->table_header (4, ui_left, "id", "Id");
-  uiout->table_header (17, ui_left, "exec", "Executable");
+  uiout->table_header (longest_exec_name, ui_left, "exec", "Executable");
+  uiout->table_header (17, ui_left, "core", "Core File");
   uiout->table_body ();
 
   for (struct program_space *pspace : program_spaces)
@@ -291,6 +299,12 @@ print_program_space (struct ui_out *uiout, int requested)
       else
 	uiout->field_skip ("exec");
 
+      if (pspace->cbfd != nullptr)
+	uiout->field_string ("core", bfd_get_filename (pspace->cbfd.get ()),
+			     file_name_style.style ());
+      else
+	uiout->field_skip ("core");
+
       /* Print extra info that doesn't really fit in tabular form.
 	 Currently, we print the list of inferiors bound to a pspace.
 	 There can be more than one inferior bound to the same pspace,
-- 
2.25.4


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

* [PATCHv2 7/7] gdb: some process_stratum_target should not be shared
  2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
                     ` (5 preceding siblings ...)
  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:04   ` Andrew Burgess
  2022-10-02 17:21     ` Eli Zaretskii
  2022-10-05 21:15     ` Lancelot SIX
  2022-11-18 16:42   ` [PATCHv3 0/7] gdb: fix target_ops reference count for some cases Andrew Burgess
  7 siblings, 2 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-02 17:04 UTC (permalink / raw)
  To: gdb-patches

When multi-target support was added to GDB, an assumption was made
that all process_stratum_target sub-classes could be shared by
multiple inferiors.

For things like the Linux and FreeBSD native targets, as well as the
remote target, this is absolutely true (or was made true).  But some
targets were never updated to be shareable, for example, the
core_target, which is used when reading core-files, stores some of its
state in the program_space, but also, the core-file and the executable
being debugged are closely related.

As each add-inferior call creates an inferior with a new
program_space, and doesn't automatically copy the executable, or the
current core-file, I don't think it really makes sense to "share"
core_target objects between inferiors.

Consider this session:

  $ gdb -q
  (gdb) file test1
  Reading symbols from test1...
  (gdb) core-file core.test1.433190
  [New LWP 433190]
  Core was generated by `./test1'.
  Program terminated with signal SIGSEGV, Segmentation fault.
  #0  0x0000000000401111 in foo () at test1.c:6
  6	  return *global_ptr;
  (gdb) add-inferior
  [New inferior 2]
  Added inferior 2 on connection 1 (core)
  (gdb) info inferiors
    Num  Description       Connection           Executable
  * 1    process 433190    1 (core)             /tmp/multi-core/test1
    2    <null>            1 (core)
  (gdb) info connections
    Num  What  Description
  * 1    core  Local core dump file
  (gdb) inferior 2
  [Switching to inferior 2 [<null>] (<noexec>)]
  (gdb) file test2
  Reading symbols from test2...
  (gdb) core-file core.test2.433203
  [New LWP 433203]
  Core was generated by `./test2'.
  Program terminated with signal SIGSEGV, Segmentation fault.
  #0  0x0000000000401111 in main () at test2.c:6
  6	  return *global_ptr;
  (gdb) info inferiors
    Num  Description       Connection           Executable
    1    process 433190    1 (core)             /tmp/multi-core/test1
  * 2    process 433203    2 (core)             /tmp/multi-core/test2
  (gdb) info connections
    Num  What  Description
    1    core  Local core dump file
  * 2    core  Local core dump file
  (gdb)

After the 'add-inferior' the core_target connection is shared between
the inferiors.  However, as soon as the user sets up the core-file and
executable in the new inferior a new core connection has been created.

I think this behaviour might be confusing, so I'd like to have GDB not
initially share the core connection.  Instead, when the user tries to
add the new inferior a warning is given, and the new inferior is
created without a connection, like this:

  $ gdb -q
  (gdb) file test1
  Reading symbols from test1...
  (gdb) core-file core.test1.433190
  [New LWP 433190]
  Core was generated by `./test1'.
  Program terminated with signal SIGSEGV, Segmentation fault.
  #0  0x0000000000401111 in foo () at test1.c:6
  6	  return *global_ptr;
  (gdb) add-inferior
  [New inferior 2]
  warning: can't share connection 1 (core) between inferiors
  Added inferior 2
  (gdb) info inferiors
    Num  Description       Connection           Executable
  * 1    process 433190    1 (core)             /tmp/multi-core/test1
    2    <null>
  (gdb)

If the user explicitly asks for the new inferior to be created without
a connection, then no warning will be given.

At this point the user is free to setup inferior 2 with a different
executable and core file (or to do anything they like with the
inferior).

In an earlier version of this patch I had GDB error instead of giving
a warning.  However, the error message ended up being something like:

  can't share connection ..... between inferiors, use -no-connection
      option to create an inferior without sharing a connection.

but it seemed better to just create the inferior.

I've updated the docs, and added a NEWS entry for the new warning.  In
the docs for clone-inferior I've added reference to -no-connection,
which was previously missing.
---
 gdb/NEWS                                     |   7 +
 gdb/corelow.c                                |   5 +
 gdb/doc/gdb.texinfo                          |  37 ++++-
 gdb/inferior.c                               |  16 ++
 gdb/inferior.h                               |   6 +-
 gdb/target.c                                 |  14 ++
 gdb/target.h                                 |   8 +
 gdb/testsuite/gdb.multi/multi-core-files-1.c |  37 +++++
 gdb/testsuite/gdb.multi/multi-core-files-2.c |  31 ++++
 gdb/testsuite/gdb.multi/multi-core-files.exp | 156 +++++++++++++++++++
 10 files changed, 313 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-1.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-2.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index a6ea7c9f98f..cb576edd203 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -71,6 +71,13 @@
   For both /r and /b GDB is now better at using whitespace in order to
   align the disassembled instruction text.
 
+* The add-inferior, clone-inferior, and MI -add-inferior commands will
+  now give a warning, and create the new inferior without a
+  connection, when the current inferior, at the time the command is
+  given, is a core-file target.  The core-file target could never
+  really be shared between inferiors, GDB is now more vocal about what
+  is going on.
+
 * New commands
 
 maintenance set ignore-prologue-end-flag on|off
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 293bc8d4f59..4d8393a3587 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -128,6 +128,11 @@ class core_target final : public process_stratum_target
   /* See definition.  */
   void info_proc_mappings (struct gdbarch *gdbarch);
 
+  /* The core_target only works for the inferior in which it was initially
+     opened, and can't be copied to some other inferior's target_stack.  */
+  bool is_shareable () override
+  { return false; }
+
 private: /* per-core data */
 
   /* Get rid of the core inferior.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 107df84d108..a558fc4de38 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3358,8 +3358,31 @@
 remote} command to connect to some other @code{gdbserver} instance,
 use @code{run} to spawn a local program, etc.
 
+Not all connections can be shared between inferiors.  For example, the
+@code{target core} target is unique for each inferior.  That is,
+multiple inferiors can use @code{target core} at the same time, but
+each @code{target core} is different.  If you try to
+@code{add-inferior}, and the current inferior is @code{target core},
+then @value{GDBN} will give a warning and create the new inferior
+without a connection, like this:
+
+@smallexample
+(@value{GDBP}) file test1
+Reading symbols from test1...
+(@value{GDBP}) target core core.test1.433190
+[New LWP 433190]
+Core was generated by `./test1'.
+Program terminated with signal SIGSEGV, Segmentation fault.
+#0  0x0000000000401111 in foo () at test1.c:6
+6	  return *global_ptr;
+(@value{GDBP}) add-inferior
+[New inferior 2]
+warning: can't share connection 1 (core) between inferiors
+Added inferior 2
+@end smallexample
+
 @kindex clone-inferior
-@item clone-inferior [ -copies @var{n} ] [ @var{infno} ]
+@item clone-inferior [ -copies @var{n} ] [ -no-connection ] [ @var{infno} ]
 Adds @var{n} inferiors ready to execute the same program as inferior
 @var{infno}; @var{n} defaults to 1, and @var{infno} defaults to the
 number of the current inferior.  This command copies the values of the
@@ -3384,6 +3407,13 @@
 
 You can now simply switch focus to inferior 2 and run it.
 
+Like @code{add-inferior}, @code{clone-inferior} shares the connection
+with the inferior @var{infno}.  If the @var{-no-connection} option is
+given then the new inferior will be created without a connection.  If
+the connection of inferior @var{infno} can't be shared, then
+@value{GDBN} will give a warning, and the new inferior will be created
+without a connection.
+
 @kindex remove-inferiors
 @item remove-inferiors @var{infno}@dots{}
 Removes the inferior or inferiors @var{infno}@dots{}.  It is not
@@ -37786,6 +37816,11 @@
 @code{gdbserver} instance, use @code{-exec-run} to spawn a local
 program, etc.
 
+If the connection of the current inferior cannot be shared, e.g.@: the
+@code{-target-select core} target cannot be shared between inferiors,
+then @value{GDBN} will give a warning and create the new inferior
+without a connection.
+
 The command response always has a field, @var{inferior}, whose value
 is the identifier of the thread group corresponding to the new
 inferior.
diff --git a/gdb/inferior.c b/gdb/inferior.c
index a498b9d493c..0868a55593a 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -817,6 +817,22 @@ switch_to_inferior_and_push_target (inferior *new_inf,
      symbols.  */
   switch_to_inferior_no_thread (new_inf);
 
+  if (!no_connection && proc_target != nullptr
+      && !proc_target->is_shareable ())
+    {
+      if (proc_target->connection_string () != nullptr)
+	warning (_("can't share connection %d (%s %s) between inferiors"),
+		 proc_target->connection_number,
+		 proc_target->shortname (),
+		 proc_target->connection_string ());
+      else
+	warning (_("can't share connection %d (%s) between inferiors"),
+		 proc_target->connection_number,
+		 proc_target->shortname ());
+
+      proc_target = nullptr;
+    }
+
   /* Reuse the target for new inferior.  */
   if (!no_connection && proc_target != NULL)
     {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 344974c4811..639364e5de3 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -762,9 +762,9 @@ extern struct inferior *add_inferior_with_spaces (void);
 /* Print the current selected inferior.  */
 extern void print_selected_inferior (struct ui_out *uiout);
 
-/* Switch to inferior NEW_INF, a new inferior, and unless
-   NO_CONNECTION is true, push the process_stratum_target of ORG_INF
-   to NEW_INF.  */
+/* Switch to inferior NEW_INF, a new inferior, and unless NO_CONNECTION is
+   true, or the process_stratum_target of ORG_INF is not shareable, push
+   the process_stratum_target of ORG_INF to NEW_INF.  */
 
 extern void switch_to_inferior_and_push_target
   (inferior *new_inf, bool no_connection, inferior *org_inf);
diff --git a/gdb/target.c b/gdb/target.c
index 1e447f604d9..65bc0e7246e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1188,6 +1188,12 @@ target_stack::push (target_ops *t)
   if (m_stack[stratum].get () != nullptr)
     unpush (m_stack[stratum].get ());
 
+  /* If this target can't be shared, then check that the target doesn't
+     already appear on some other target stack.  */
+  if (!t->is_shareable ())
+    for (inferior *inf : all_inferiors ())
+      gdb_assert (!inf->target_is_pushed (t));
+
   /* Now add the new one.  */
   m_stack[stratum] = std::move (ref);
 
@@ -3226,6 +3232,14 @@ target_ops::fileio_readlink (struct inferior *inf, const char *filename,
 
 /* See target.h.  */
 
+bool
+target_ops::is_shareable ()
+{
+  return true;
+}
+
+/* See target.h.  */
+
 int
 target_fileio_open (struct inferior *inf, const char *filename,
 		    int flags, int mode, bool warn_if_slow, fileio_error *target_errno)
diff --git a/gdb/target.h b/gdb/target.h
index 547ee8a3bbd..30e5085a543 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1321,6 +1321,14 @@ struct target_ops
     virtual bool store_memtags (CORE_ADDR address, size_t len,
 				const gdb::byte_vector &tags, int type)
       TARGET_DEFAULT_NORETURN (tcomplain ());
+
+    /* Return true if this target can be shared on multiple target_stacks,
+       or false if this target should only appear on a single target_stack.
+       When this function returns false multiple separate instances of the
+       same target_ops sub-class can still appear on different
+       target_stacks, but the same concrete instance can only appear on a
+       single target_stack.  */
+    virtual bool is_shareable ();
   };
 
 /* Deleter for std::unique_ptr.  See comments in
diff --git a/gdb/testsuite/gdb.multi/multi-core-files-1.c b/gdb/testsuite/gdb.multi/multi-core-files-1.c
new file mode 100644
index 00000000000..f996973023e
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files-1.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <stdlib.h>
+
+int
+bar ()
+{
+  abort ();
+  return 0;
+}
+
+int
+baz ()
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  return baz ();
+}
diff --git a/gdb/testsuite/gdb.multi/multi-core-files-2.c b/gdb/testsuite/gdb.multi/multi-core-files-2.c
new file mode 100644
index 00000000000..fb99e137c3f
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files-2.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <stdlib.h>
+
+int
+foo ()
+{
+  abort ();
+  return 0;
+}
+
+int
+main ()
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.multi/multi-core-files.exp b/gdb/testsuite/gdb.multi/multi-core-files.exp
new file mode 100644
index 00000000000..9cf188b0caa
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files.exp
@@ -0,0 +1,156 @@
+# Copyright 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/>.
+
+# This script runs some basic tests that GDB can support multiple
+# inferiors each debugging different core files.
+#
+# We also check the behaviour of GDB if the user attempts to clone or
+# duplicate an inferior that is debugging a core file.
+
+standard_testfile -1.c -2.c
+
+set binfile1 "${binfile}-1"
+set binfile2 "${binfile}-2"
+
+if {[build_executable "build first executable" $binfile1 $srcfile \
+	 debug] == -1} {
+    untested "failed to compile first executable"
+    return -1
+}
+
+if {[build_executable "build first executable" $binfile2 $srcfile2 \
+	 debug] == -1} {
+    untested "failed to compile second executable"
+    return -1
+}
+
+set corefile1 [core_find $binfile1]
+set corefile2 [core_find $binfile2]
+
+# Start GDB, and load the first executable and corefile into the first
+# inferior.
+clean_restart ${binfile1}
+gdb_test "core-file $corefile1" "Program terminated with .*" \
+    "load core file"
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 1"
+
+# Try to use add-inferior and clone-inferior to create new
+# inferiors. In both cases this will try to share the core_target
+# between inferior 1 and the new inferior.  As the core_target can't
+# be shared we should get a warning, and the inferior should be
+# created without a connection.
+gdb_test "add-inferior" \
+    [multi_line \
+	 "\\\[New inferior 2\\\]" \
+	 "warning: can't share connection 1 \\(core\\) between inferiors" \
+	 "Added inferior 2"]
+gdb_test "clone-inferior" \
+    [multi_line \
+	 "\\\[New inferior 3\\\]" \
+	 "warning: can't share connection 1 \\(core\\) between inferiors" \
+	 "Added inferior 3"]
+
+# Check the MI -add-inferior command.  Do this using interpreter-exec.
+# We're not doing a full MI test here, just checking this one command.
+gdb_test "interpreter-exec mi \"-add-inferior\"" \
+    [multi_line \
+	 "~\"\\\[New inferior 4\\\]..\"" \
+	 "&\"warning: can't share connection 1 \\(core\\) between inferiors..\"" \
+	 "~\"Added inferior 4..\"" \
+	 "\\^done,inferior=\"\[^\"\]+\""]
+
+# Now check that non of the new inferiors have a connection.
+gdb_test "info inferiors" \
+    [multi_line \
+	 "\\*\\s+1\\s+\[^\r\n\]+\\s+1 \\(core\\)\\s+\[^\r\n\]+.*" \
+	 "\\s+2\\s+<null>\\s+" \
+	 "\\s+3\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+4\\s+<null>\\s+"] \
+    "first info inferiors call"
+
+# Now use add-inferior and clone-inferior but this time with the
+# -no-connection option, this should avoid issuing the warning.  We
+# also use interpreter-exec to test the MI version of this command.
+gdb_test "add-inferior -no-connection" \
+    [multi_line \
+	 "\\\[New inferior 5\\\]" \
+	 "Added inferior 5"]
+gdb_test "clone-inferior -no-connection" \
+    [multi_line \
+	 "\\\[New inferior 6\\\]" \
+	 "Added inferior 6"]
+gdb_test "interpreter-exec mi \"-add-inferior --no-connection\"" \
+    "\\\[New inferior 7\\\].*Added inferior 7.*"
+
+# Now check that non of the new inferiors have a connection.
+gdb_test "info inferiors" \
+    [multi_line \
+	 "\\*\\s+1\\s+\[^\r\n\]+\\s+1 \\(core\\)\\s+\[^\r\n\]+.*" \
+	 "\\s+2\\s+<null>\\s+" \
+	 "\\s+3\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+4\\s+<null>\\s+" \
+	 "\\s+5\\s+<null>\\s+" \
+	 "\\s+6\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+7\\s+<null>\\s+"] \
+    "second info inferiors call"
+
+# Check after all the new inferiors have been created that we still
+# only have a single connection.
+gdb_test "info connections" \
+    "\\*\\s+1\\s+\\s+core\\s+Local core dump file\\s*"
+
+# Now switch to inferior 2 and load the second executable and core
+# file.  Check the backtrace for the presence of function 'foo', this
+# indicates we are seeing the correct core file.
+gdb_test "inferior 2" "Switching to inferior 2 .*"
+gdb_test "file $binfile2" \
+    "Reading symbols from .*" \
+    "Loaded second test binary"
+gdb_test "core-file $corefile2" \
+    "Program terminated with signal SIGABRT, Aborted.*" \
+    "Loaded second core file"
+gdb_test "bt" "foo \\(\\) at .*" \
+    "check backtrace in inferior 2"
+
+# Switch to inferior 3, this one was cloned from inferior 1, so is
+# already debugging the first binary file.  Check its backtrace for
+# 'bar', which indicates we are debugging the correct core file.
+gdb_test "inferior 3" "Switching to inferior 3 .*"
+gdb_test "core-file $corefile1" \
+    "Program terminated with signal SIGABRT, Aborted.*" \
+    "Loaded first core file into inferior 3"
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 3"
+
+# Detach from some of the core files and delete some of the inferiors.
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 3 core file"
+gdb_test "inferior 2" "Switching to inferior 2 .*" \
+    "switch back to inferior 2"
+gdb_test_no_output "remove-inferiors 3 4"
+
+# Now detach in inferior 2, and delete the inferior.
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 2 core file"
+gdb_test "inferior 1" "Switching to inferior 1 .*" \
+    "switch back to inferior 1"
+gdb_test_no_output "remove-inferiors 2"
+
+# Finally, check that inferior 1 backtrace is still working.
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 1 again"
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 1 core file"
-- 
2.25.4


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

* Re: [PATCHv2 7/7] gdb: some process_stratum_target should not be shared
  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
  1 sibling, 0 replies; 36+ messages in thread
From: Eli Zaretskii @ 2022-10-02 17:21 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

> Date: Sun,  2 Oct 2022 18:04:48 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
>  gdb/NEWS                                     |   7 +
>  gdb/corelow.c                                |   5 +
>  gdb/doc/gdb.texinfo                          |  37 ++++-
>  gdb/inferior.c                               |  16 ++
>  gdb/inferior.h                               |   6 +-
>  gdb/target.c                                 |  14 ++
>  gdb/target.h                                 |   8 +
>  gdb/testsuite/gdb.multi/multi-core-files-1.c |  37 +++++
>  gdb/testsuite/gdb.multi/multi-core-files-2.c |  31 ++++
>  gdb/testsuite/gdb.multi/multi-core-files.exp | 156 +++++++++++++++++++
>  10 files changed, 313 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-1.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-2.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files.exp

Thanks, the documentation parts are OK, with this minor nit:

> +Like @code{add-inferior}, @code{clone-inferior} shares the connection
> +with the inferior @var{infno}.  If the @var{-no-connection} option is
> +given then the new inferior will be created without a connection.  If
        ^
A comma is missing there.

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

* Re: [PATCHv2 6/7] gdb/maint: add core file name to 'maint info program-spaces' output
  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
  0 siblings, 0 replies; 36+ messages in thread
From: Eli Zaretskii @ 2022-10-02 17:21 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

> Date: Sun,  2 Oct 2022 18:04:47 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> Each program space can have an associated core file.  Include this
> information in the output of 'maint info program-spaces'.
> ---
>  gdb/NEWS            |  4 ++++
>  gdb/doc/gdb.texinfo |  8 ++++++--
>  gdb/progspace.c     | 18 ++++++++++++++++--
>  3 files changed, 26 insertions(+), 4 deletions(-)

Thanks, the documentation parts are OK.

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

* Re: [PATCHv2 4/7] gdb: remove the pop_all_targets (and friends) global functions
  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
  0 siblings, 1 reply; 36+ messages in thread
From: Lancelot SIX @ 2022-10-05 20:49 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

Hi,

I have a minor comment inline below.

On Sun, Oct 02, 2022 at 06:04:45PM +0100, Andrew Burgess via Gdb-patches wrote:
> This commit removes the global functions pop_all_targets,
> pop_all_targets_above, and pop_all_targets_at_and_above, and makes
> them methods on the inferior class.
> 
> As the pop_all_targets functions will unpush each target, which
> decrements the targets reference count, it is possible that the target
> might be closed.
> 
> Right now, closing a target, in some cases, depends on the current
> inferior being set correctly, that is, to the inferior from which the
> target was popped.
> 
> To facilitate this I have used switch_to_inferior_no_thread within the
> new methods.  Previously it was the responsibility of the caller to
> ensure that the correct inferior was selected.
> 
> In a couple of places (event-top.c and top.c) I have been able to
> remove a previous switch_to_inferior_no_thread call.
> 
> In remote_unpush_target (remote.c) I have left the
> switch_to_inferior_no_thread call as it is required for the
> generic_mourn_inferior call.
> ---
>  gdb/event-top.c           |  3 +--
>  gdb/inferior.c            | 40 ++++++++++++++++++++++++++++++++++++++
>  gdb/inferior.h            | 20 +++++++++++++++++++
>  gdb/remote.c              |  2 +-
>  gdb/scoped-mock-context.h |  2 +-
>  gdb/target.c              | 41 +--------------------------------------
>  gdb/target.h              | 11 -----------
>  gdb/top.c                 |  3 +--
>  8 files changed, 65 insertions(+), 57 deletions(-)
> 
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 88c53b720a9..836e6b935e3 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -1293,10 +1293,9 @@ async_disconnect (gdb_client_data arg)
>  
>    for (inferior *inf : all_inferiors ())
>      {
> -      switch_to_inferior_no_thread (inf);
>        try
>  	{
> -	  pop_all_targets ();
> +	  inf->pop_all_targets ();
>  	}
>        catch (const gdb_exception &exception)
>  	{
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 7eb2bd97907..2014bf926b7 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -103,6 +103,46 @@ inferior::unpush_target (struct target_ops *t)
>    return m_target_stack.unpush (t);
>  }
>  
> +/* See inferior.h.  */
> +
> +void inferior::unpush_target_and_assert (struct target_ops *target)
> +{
> +  gdb_assert (current_inferior () == this);
> +
> +  if (!unpush_target (target))
> +    internal_error (__FILE__, __LINE__,
> +		    "pop_all_targets couldn't find target %s\n",
> +		    target->shortname ());
> +}
> +
> +/* See inferior.h.  */
> +
> +void inferior::pop_all_targets_above (enum strata stratum)
> +{
> +  /* Unpushing a target might cause it to close.  Some targets currently
> +     rely on the current_inferior being set for their ::close method, so we
> +     temporarily switch inferior now.  */
> +  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
> +  switch_to_inferior_no_thread (this);
> +
> +  while ((int) (top_target ()->stratum ()) > (int) stratum)

Seems to me that the casts to int are not necessary here.  Any reason to
keep those?

> +    unpush_target_and_assert (top_target ());
> +}
> +
> +/* See inferior.h.  */
> +
> +void inferior::pop_all_targets_at_and_above (enum strata stratum)
> +{
> +  /* Unpushing a target might cause it to close.  Some targets currently
> +     rely on the current_inferior being set for their ::close method, so we
> +     temporarily switch inferior now.  */
> +  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
> +  switch_to_inferior_no_thread (this);
> +
> +  while ((int) (top_target ()->stratum ()) >= (int) stratum)

Same here.

Best,
Lancelot.
> +    unpush_target_and_assert (top_target ());
> +}
> +
>  void
>  inferior::set_tty (std::string terminal_name)
>  {
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index bb3aec1e8f8..344974c4811 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -380,6 +380,22 @@ class inferior : public refcounted_object,
>    target_ops *top_target ()
>    { return m_target_stack.top (); }
>  
> +  /* Unpush all targets except the dummy target from m_target_stack.  As
> +     targets are removed from m_target_stack their reference count is
> +     decremented, which may cause a target to close.  */
> +  void pop_all_targets ()
> +  { pop_all_targets_above (dummy_stratum); }
> +
> +  /* Unpush all targets above STRATUM from m_target_stack.  As targets are
> +     removed from m_target_stack their reference count is decremented,
> +     which may cause a target to close.  */
> +  void pop_all_targets_above (enum strata stratum);
> +
> +  /* Unpush all targets at and above STRATUM from m_target_stack.  As
> +     targets are removed from m_target_stack their reference count is
> +     decremented, which may cause a target to close.  */
> +  void pop_all_targets_at_and_above (enum strata stratum);
> +
>    /* Return the target at process_stratum level in this inferior's
>       target stack.  */
>    struct process_stratum_target *process_target ()
> @@ -598,6 +614,10 @@ class inferior : public refcounted_object,
>    registry<inferior> registry_fields;
>  
>  private:
> +
> +  /* Unpush TARGET and assert that it worked.  */
> +  void unpush_target_and_assert (struct target_ops *target);
> +
>    /* The inferior's target stack.  */
>    target_stack m_target_stack;
>  
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5a71c41d61e..4864e9a55c3 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5721,7 +5721,7 @@ remote_unpush_target (remote_target *target)
>    for (inferior *inf : all_inferiors (target))
>      {
>        switch_to_inferior_no_thread (inf);
> -      pop_all_targets_at_and_above (process_stratum);
> +      inf->pop_all_targets_at_and_above (process_stratum);
>        generic_mourn_inferior ();
>      }
>  
> diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
> index a9895303015..87c1df0d206 100644
> --- a/gdb/scoped-mock-context.h
> +++ b/gdb/scoped-mock-context.h
> @@ -71,7 +71,7 @@ struct scoped_mock_context
>    ~scoped_mock_context ()
>    {
>      inferior_list.erase (inferior_list.iterator_to (mock_inferior));
> -    pop_all_targets_at_and_above (process_stratum);
> +    mock_inferior.pop_all_targets_at_and_above (process_stratum);
>    }
>  };
>  
> diff --git a/gdb/target.c b/gdb/target.c
> index 0f4d6d01057..1e447f604d9 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1238,45 +1238,6 @@ target_stack::unpush (target_ops *t)
>    return true;
>  }
>  
> -/* Unpush TARGET and assert that it worked.  */
> -
> -static void
> -unpush_target_and_assert (struct target_ops *target)
> -{
> -  if (!current_inferior ()->unpush_target (target))
> -    {
> -      gdb_printf (gdb_stderr,
> -		  "pop_all_targets couldn't find target %s\n",
> -		  target->shortname ());
> -      internal_error (__FILE__, __LINE__,
> -		      _("failed internal consistency check"));
> -    }
> -}
> -
> -void
> -pop_all_targets_above (enum strata above_stratum)
> -{
> -  while ((int) (current_inferior ()->top_target ()->stratum ())
> -	 > (int) above_stratum)
> -    unpush_target_and_assert (current_inferior ()->top_target ());
> -}
> -
> -/* See target.h.  */
> -
> -void
> -pop_all_targets_at_and_above (enum strata stratum)
> -{
> -  while ((int) (current_inferior ()->top_target ()->stratum ())
> -	 >= (int) stratum)
> -    unpush_target_and_assert (current_inferior ()->top_target ());
> -}
> -
> -void
> -pop_all_targets (void)
> -{
> -  pop_all_targets_above (dummy_stratum);
> -}
> -
>  void
>  target_unpusher::operator() (struct target_ops *ops) const
>  {
> @@ -2533,7 +2494,7 @@ target_preopen (int from_tty)
>       it doesn't (which seems like a win for UDI), remove it now.  */
>    /* Leave the exec target, though.  The user may be switching from a
>       live process to a core of the same program.  */
> -  pop_all_targets_above (file_stratum);
> +  current_inferior ()->pop_all_targets_above (file_stratum);
>  
>    target_pre_inferior (from_tty);
>  }
> diff --git a/gdb/target.h b/gdb/target.h
> index 68446a39c1b..547ee8a3bbd 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -2389,17 +2389,6 @@ extern void target_pre_inferior (int);
>  
>  extern void target_preopen (int);
>  
> -/* Does whatever cleanup is required to get rid of all pushed targets.  */
> -extern void pop_all_targets (void);
> -
> -/* Like pop_all_targets, but pops only targets whose stratum is at or
> -   above STRATUM.  */
> -extern void pop_all_targets_at_and_above (enum strata stratum);
> -
> -/* Like pop_all_targets, but pops only targets whose stratum is
> -   strictly above ABOVE_STRATUM.  */
> -extern void pop_all_targets_above (enum strata above_stratum);
> -
>  extern CORE_ADDR target_translate_tls_address (struct objfile *objfile,
>  					       CORE_ADDR offset);
>  
> diff --git a/gdb/top.c b/gdb/top.c
> index 54c7c922142..5f64c6bddf0 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -1845,10 +1845,9 @@ quit_force (int *exit_arg, int from_tty)
>       them all out.  */
>    for (inferior *inf : all_inferiors ())
>      {
> -      switch_to_inferior_no_thread (inf);
>        try
>  	{
> -	  pop_all_targets ();
> +	  inf->pop_all_targets ();
>  	}
>        catch (const gdb_exception &ex)
>  	{
> -- 
> 2.25.4
> 

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

* Re: [PATCHv2 7/7] gdb: some process_stratum_target should not be shared
  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
  1 sibling, 1 reply; 36+ messages in thread
From: Lancelot SIX @ 2022-10-05 21:15 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches

Hi,

Thanks for doing this!

I have a couple of comments below.

On Sun, Oct 02, 2022 at 06:04:48PM +0100, Andrew Burgess via Gdb-patches wrote:
> When multi-target support was added to GDB, an assumption was made
> that all process_stratum_target sub-classes could be shared by
> multiple inferiors.
> 
> For things like the Linux and FreeBSD native targets, as well as the
> remote target, this is absolutely true (or was made true).  But some
> targets were never updated to be shareable, for example, the
> core_target, which is used when reading core-files, stores some of its
> state in the program_space, but also, the core-file and the executable
> being debugged are closely related.
> 
> As each add-inferior call creates an inferior with a new
> program_space, and doesn't automatically copy the executable, or the
> current core-file, I don't think it really makes sense to "share"
> core_target objects between inferiors.
> 
> Consider this session:
> 
>   $ gdb -q
>   (gdb) file test1
>   Reading symbols from test1...
>   (gdb) core-file core.test1.433190
>   [New LWP 433190]
>   Core was generated by `./test1'.
>   Program terminated with signal SIGSEGV, Segmentation fault.
>   #0  0x0000000000401111 in foo () at test1.c:6
>   6	  return *global_ptr;
>   (gdb) add-inferior
>   [New inferior 2]
>   Added inferior 2 on connection 1 (core)
>   (gdb) info inferiors
>     Num  Description       Connection           Executable
>   * 1    process 433190    1 (core)             /tmp/multi-core/test1
>     2    <null>            1 (core)
>   (gdb) info connections
>     Num  What  Description
>   * 1    core  Local core dump file
>   (gdb) inferior 2
>   [Switching to inferior 2 [<null>] (<noexec>)]
>   (gdb) file test2
>   Reading symbols from test2...
>   (gdb) core-file core.test2.433203
>   [New LWP 433203]
>   Core was generated by `./test2'.
>   Program terminated with signal SIGSEGV, Segmentation fault.
>   #0  0x0000000000401111 in main () at test2.c:6
>   6	  return *global_ptr;
>   (gdb) info inferiors
>     Num  Description       Connection           Executable
>     1    process 433190    1 (core)             /tmp/multi-core/test1
>   * 2    process 433203    2 (core)             /tmp/multi-core/test2
>   (gdb) info connections
>     Num  What  Description
>     1    core  Local core dump file
>   * 2    core  Local core dump file
>   (gdb)
> 
> After the 'add-inferior' the core_target connection is shared between
> the inferiors.  However, as soon as the user sets up the core-file and
> executable in the new inferior a new core connection has been created.
> 
> I think this behaviour might be confusing, so I'd like to have GDB not
> initially share the core connection.  Instead, when the user tries to
> add the new inferior a warning is given, and the new inferior is
> created without a connection, like this:
> 
>   $ gdb -q
>   (gdb) file test1
>   Reading symbols from test1...
>   (gdb) core-file core.test1.433190
>   [New LWP 433190]
>   Core was generated by `./test1'.
>   Program terminated with signal SIGSEGV, Segmentation fault.
>   #0  0x0000000000401111 in foo () at test1.c:6
>   6	  return *global_ptr;
>   (gdb) add-inferior
>   [New inferior 2]
>   warning: can't share connection 1 (core) between inferiors
>   Added inferior 2
>   (gdb) info inferiors
>     Num  Description       Connection           Executable
>   * 1    process 433190    1 (core)             /tmp/multi-core/test1
>     2    <null>
>   (gdb)
> 
> If the user explicitly asks for the new inferior to be created without
> a connection, then no warning will be given.
> 
> At this point the user is free to setup inferior 2 with a different
> executable and core file (or to do anything they like with the
> inferior).
> 
> In an earlier version of this patch I had GDB error instead of giving
> a warning.  However, the error message ended up being something like:
> 
>   can't share connection ..... between inferiors, use -no-connection
>       option to create an inferior without sharing a connection.
> 
> but it seemed better to just create the inferior.
> 
> I've updated the docs, and added a NEWS entry for the new warning.  In
> the docs for clone-inferior I've added reference to -no-connection,
> which was previously missing.
> ---
>  gdb/NEWS                                     |   7 +
>  gdb/corelow.c                                |   5 +
>  gdb/doc/gdb.texinfo                          |  37 ++++-
>  gdb/inferior.c                               |  16 ++
>  gdb/inferior.h                               |   6 +-
>  gdb/target.c                                 |  14 ++
>  gdb/target.h                                 |   8 +
>  gdb/testsuite/gdb.multi/multi-core-files-1.c |  37 +++++
>  gdb/testsuite/gdb.multi/multi-core-files-2.c |  31 ++++
>  gdb/testsuite/gdb.multi/multi-core-files.exp | 156 +++++++++++++++++++
>  10 files changed, 313 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-1.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-2.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a6ea7c9f98f..cb576edd203 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -71,6 +71,13 @@
>    For both /r and /b GDB is now better at using whitespace in order to
>    align the disassembled instruction text.
>  
> +* The add-inferior, clone-inferior, and MI -add-inferior commands will
> +  now give a warning, and create the new inferior without a
> +  connection, when the current inferior, at the time the command is
> +  given, is a core-file target.  The core-file target could never
> +  really be shared between inferiors, GDB is now more vocal about what
> +  is going on.
> +
>  * New commands
>  
>  maintenance set ignore-prologue-end-flag on|off
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 293bc8d4f59..4d8393a3587 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -128,6 +128,11 @@ class core_target final : public process_stratum_target
>    /* See definition.  */
>    void info_proc_mappings (struct gdbarch *gdbarch);
>  
> +  /* The core_target only works for the inferior in which it was initially
> +     opened, and can't be copied to some other inferior's target_stack.  */
> +  bool is_shareable () override
> +  { return false; }
> +
>  private: /* per-core data */
>  
>    /* Get rid of the core inferior.  */
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 107df84d108..a558fc4de38 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -3358,8 +3358,31 @@
>  remote} command to connect to some other @code{gdbserver} instance,
>  use @code{run} to spawn a local program, etc.
>  
> +Not all connections can be shared between inferiors.  For example, the
> +@code{target core} target is unique for each inferior.  That is,
> +multiple inferiors can use @code{target core} at the same time, but
> +each @code{target core} is different.  If you try to
> +@code{add-inferior}, and the current inferior is @code{target core},
> +then @value{GDBN} will give a warning and create the new inferior
> +without a connection, like this:
> +
> +@smallexample
> +(@value{GDBP}) file test1
> +Reading symbols from test1...
> +(@value{GDBP}) target core core.test1.433190
> +[New LWP 433190]
> +Core was generated by `./test1'.
> +Program terminated with signal SIGSEGV, Segmentation fault.
> +#0  0x0000000000401111 in foo () at test1.c:6
> +6	  return *global_ptr;
> +(@value{GDBP}) add-inferior
> +[New inferior 2]
> +warning: can't share connection 1 (core) between inferiors
> +Added inferior 2
> +@end smallexample
> +
>  @kindex clone-inferior
> -@item clone-inferior [ -copies @var{n} ] [ @var{infno} ]
> +@item clone-inferior [ -copies @var{n} ] [ -no-connection ] [ @var{infno} ]
>  Adds @var{n} inferiors ready to execute the same program as inferior
>  @var{infno}; @var{n} defaults to 1, and @var{infno} defaults to the
>  number of the current inferior.  This command copies the values of the
> @@ -3384,6 +3407,13 @@
>  
>  You can now simply switch focus to inferior 2 and run it.
>  
> +Like @code{add-inferior}, @code{clone-inferior} shares the connection
> +with the inferior @var{infno}.  If the @var{-no-connection} option is
> +given then the new inferior will be created without a connection.  If
> +the connection of inferior @var{infno} can't be shared, then
> +@value{GDBN} will give a warning, and the new inferior will be created
> +without a connection.
> +
>  @kindex remove-inferiors
>  @item remove-inferiors @var{infno}@dots{}
>  Removes the inferior or inferiors @var{infno}@dots{}.  It is not
> @@ -37786,6 +37816,11 @@
>  @code{gdbserver} instance, use @code{-exec-run} to spawn a local
>  program, etc.
>  
> +If the connection of the current inferior cannot be shared, e.g.@: the
> +@code{-target-select core} target cannot be shared between inferiors,
> +then @value{GDBN} will give a warning and create the new inferior
> +without a connection.
> +
>  The command response always has a field, @var{inferior}, whose value
>  is the identifier of the thread group corresponding to the new
>  inferior.
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index a498b9d493c..0868a55593a 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -817,6 +817,22 @@ switch_to_inferior_and_push_target (inferior *new_inf,
>       symbols.  */
>    switch_to_inferior_no_thread (new_inf);
>  
> +  if (!no_connection && proc_target != nullptr
> +      && !proc_target->is_shareable ())
> +    {
> +      if (proc_target->connection_string () != nullptr)
> +	warning (_("can't share connection %d (%s %s) between inferiors"),
> +		 proc_target->connection_number,
> +		 proc_target->shortname (),
> +		 proc_target->connection_string ());
> +      else
> +	warning (_("can't share connection %d (%s) between inferiors"),
> +		 proc_target->connection_number,
> +		 proc_target->shortname ());
> +
> +      proc_target = nullptr;
> +    }
> +
>    /* Reuse the target for new inferior.  */
>    if (!no_connection && proc_target != NULL)
>      {
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 344974c4811..639364e5de3 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -762,9 +762,9 @@ extern struct inferior *add_inferior_with_spaces (void);
>  /* Print the current selected inferior.  */
>  extern void print_selected_inferior (struct ui_out *uiout);
>  
> -/* Switch to inferior NEW_INF, a new inferior, and unless
> -   NO_CONNECTION is true, push the process_stratum_target of ORG_INF
> -   to NEW_INF.  */
> +/* Switch to inferior NEW_INF, a new inferior, and unless NO_CONNECTION is
> +   true, or the process_stratum_target of ORG_INF is not shareable, push
> +   the process_stratum_target of ORG_INF to NEW_INF.  */
>  
>  extern void switch_to_inferior_and_push_target
>    (inferior *new_inf, bool no_connection, inferior *org_inf);
> diff --git a/gdb/target.c b/gdb/target.c
> index 1e447f604d9..65bc0e7246e 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1188,6 +1188,12 @@ target_stack::push (target_ops *t)
>    if (m_stack[stratum].get () != nullptr)
>      unpush (m_stack[stratum].get ());
>  
> +  /* If this target can't be shared, then check that the target doesn't
> +     already appear on some other target stack.  */
> +  if (!t->is_shareable ())
> +    for (inferior *inf : all_inferiors ())
> +      gdb_assert (!inf->target_is_pushed (t));
> +
>    /* Now add the new one.  */
>    m_stack[stratum] = std::move (ref);
>  
> @@ -3226,6 +3232,14 @@ target_ops::fileio_readlink (struct inferior *inf, const char *filename,
>  
>  /* See target.h.  */
>  
> +bool
> +target_ops::is_shareable ()
> +{
> +  return true;
> +}
> +
> +/* See target.h.  */
> +
>  int
>  target_fileio_open (struct inferior *inf, const char *filename,
>  		    int flags, int mode, bool warn_if_slow, fileio_error *target_errno)
> diff --git a/gdb/target.h b/gdb/target.h
> index 547ee8a3bbd..30e5085a543 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1321,6 +1321,14 @@ struct target_ops
>      virtual bool store_memtags (CORE_ADDR address, size_t len,
>  				const gdb::byte_vector &tags, int type)
>        TARGET_DEFAULT_NORETURN (tcomplain ());
> +
> +    /* Return true if this target can be shared on multiple target_stacks,
> +       or false if this target should only appear on a single target_stack.
> +       When this function returns false multiple separate instances of the
> +       same target_ops sub-class can still appear on different
> +       target_stacks, but the same concrete instance can only appear on a
> +       single target_stack.  */
> +    virtual bool is_shareable ();
>    };
>  
>  /* Deleter for std::unique_ptr.  See comments in
> diff --git a/gdb/testsuite/gdb.multi/multi-core-files-1.c b/gdb/testsuite/gdb.multi/multi-core-files-1.c
> new file mode 100644
> index 00000000000..f996973023e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/multi-core-files-1.c
> @@ -0,0 +1,37 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 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/>.  */
> +
> +#include <stdlib.h>
> +
> +int
> +bar ()
> +{
> +  abort ();
> +  return 0;
> +}
> +
> +int
> +baz ()
> +{
> +  return bar ();
> +}
> +
> +int
> +main ()
> +{
> +  return baz ();
> +}
> diff --git a/gdb/testsuite/gdb.multi/multi-core-files-2.c b/gdb/testsuite/gdb.multi/multi-core-files-2.c
> new file mode 100644
> index 00000000000..fb99e137c3f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/multi-core-files-2.c
> @@ -0,0 +1,31 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 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/>.  */
> +
> +#include <stdlib.h>
> +
> +int
> +foo ()
> +{
> +  abort ();
> +  return 0;
> +}
> +
> +int
> +main ()
> +{
> +  return foo ();
> +}
> diff --git a/gdb/testsuite/gdb.multi/multi-core-files.exp b/gdb/testsuite/gdb.multi/multi-core-files.exp
> new file mode 100644
> index 00000000000..9cf188b0caa
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/multi-core-files.exp
> @@ -0,0 +1,156 @@
> +# Copyright 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/>.
> +
> +# This script runs some basic tests that GDB can support multiple
> +# inferiors each debugging different core files.
> +#
> +# We also check the behaviour of GDB if the user attempts to clone or
> +# duplicate an inferior that is debugging a core file.
> +
> +standard_testfile -1.c -2.c
> +
> +set binfile1 "${binfile}-1"
> +set binfile2 "${binfile}-2"
> +
> +if {[build_executable "build first executable" $binfile1 $srcfile \
> +	 debug] == -1} {
> +    untested "failed to compile first executable"
> +    return -1
> +}
> +
> +if {[build_executable "build first executable" $binfile2 $srcfile2 \

s/first/second/ maybe?

> +	 debug] == -1} {
> +    untested "failed to compile second executable"
> +    return -1
> +}
> +
> +set corefile1 [core_find $binfile1]
> +set corefile2 [core_find $binfile2]

I think you should check corefile*.

On my system (where ulimit -c gives 0), I have:

    WARNING: can't generate a core file - core tests suppressed - check ulimit -c
    WARNING: can't generate a core file - core tests suppressed - check ulimit -c
    FAIL: gdb.multi/multi-core-files.exp: load core file
    FAIL: gdb.multi/multi-core-files.exp: check backtrace in inferior 1
    FAIL: gdb.multi/multi-core-files.exp: add-inferior
    FAIL: gdb.multi/multi-core-files.exp: clone-inferior
    FAIL: gdb.multi/multi-core-files.exp: interpreter-exec mi "-add-inferior"
    FAIL: gdb.multi/multi-core-files.exp: first info inferiors call
    FAIL: gdb.multi/multi-core-files.exp: second info inferiors call
    FAIL: gdb.multi/multi-core-files.exp: info connections
    FAIL: gdb.multi/multi-core-files.exp: Loaded second core file
    FAIL: gdb.multi/multi-core-files.exp: check backtrace in inferior 2
    FAIL: gdb.multi/multi-core-files.exp: Loaded first core file into inferior 3
    FAIL: gdb.multi/multi-core-files.exp: check backtrace in inferior 3
    FAIL: gdb.multi/multi-core-files.exp: detach from inferior 3 core file (the program is no longer running)
    FAIL: gdb.multi/multi-core-files.exp: detach from inferior 2 core file (the program is no longer running)
    FAIL: gdb.multi/multi-core-files.exp: check backtrace in inferior 1 again
    FAIL: gdb.multi/multi-core-files.exp: detach from inferior 1 core file (the program is no longer running)

I think something like this could do:

    if { $corefile1 eq "" || $corefile2 eq "" } {
      untested "Can't generate core files"
      return
    }

> +
> +# Start GDB, and load the first executable and corefile into the first
> +# inferior.
> +clean_restart ${binfile1}
> +gdb_test "core-file $corefile1" "Program terminated with .*" \
> +    "load core file"
> +gdb_test "bt" "bar \\(\\) at .*" \
> +    "check backtrace in inferior 1"
> +
> +# Try to use add-inferior and clone-inferior to create new
> +# inferiors. In both cases this will try to share the core_target
               ^
Should be 2 spaces after the "."?

> +# between inferior 1 and the new inferior.  As the core_target can't
> +# be shared we should get a warning, and the inferior should be
> +# created without a connection.
> +gdb_test "add-inferior" \
> +    [multi_line \
> +	 "\\\[New inferior 2\\\]" \
> +	 "warning: can't share connection 1 \\(core\\) between inferiors" \
> +	 "Added inferior 2"]
> +gdb_test "clone-inferior" \
> +    [multi_line \
> +	 "\\\[New inferior 3\\\]" \
> +	 "warning: can't share connection 1 \\(core\\) between inferiors" \
> +	 "Added inferior 3"]
> +
> +# Check the MI -add-inferior command.  Do this using interpreter-exec.
> +# We're not doing a full MI test here, just checking this one command.
> +gdb_test "interpreter-exec mi \"-add-inferior\"" \
> +    [multi_line \
> +	 "~\"\\\[New inferior 4\\\]..\"" \
> +	 "&\"warning: can't share connection 1 \\(core\\) between inferiors..\"" \
> +	 "~\"Added inferior 4..\"" \
> +	 "\\^done,inferior=\"\[^\"\]+\""]
> +
> +# Now check that non of the new inferiors have a connection.

s/non/none/

> +gdb_test "info inferiors" \
> +    [multi_line \
> +	 "\\*\\s+1\\s+\[^\r\n\]+\\s+1 \\(core\\)\\s+\[^\r\n\]+.*" \
> +	 "\\s+2\\s+<null>\\s+" \
> +	 "\\s+3\\s+<null>\\s+\[^\r\n\]+" \
> +	 "\\s+4\\s+<null>\\s+"] \
> +    "first info inferiors call"
> +
> +# Now use add-inferior and clone-inferior but this time with the
> +# -no-connection option, this should avoid issuing the warning.  We
> +# also use interpreter-exec to test the MI version of this command.
> +gdb_test "add-inferior -no-connection" \
> +    [multi_line \
> +	 "\\\[New inferior 5\\\]" \
> +	 "Added inferior 5"]
> +gdb_test "clone-inferior -no-connection" \
> +    [multi_line \
> +	 "\\\[New inferior 6\\\]" \
> +	 "Added inferior 6"]
> +gdb_test "interpreter-exec mi \"-add-inferior --no-connection\"" \
> +    "\\\[New inferior 7\\\].*Added inferior 7.*"
> +
> +# Now check that non of the new inferiors have a connection.

s/non/none/

Best,
Lancelot.

> +gdb_test "info inferiors" \
> +    [multi_line \
> +	 "\\*\\s+1\\s+\[^\r\n\]+\\s+1 \\(core\\)\\s+\[^\r\n\]+.*" \
> +	 "\\s+2\\s+<null>\\s+" \
> +	 "\\s+3\\s+<null>\\s+\[^\r\n\]+" \
> +	 "\\s+4\\s+<null>\\s+" \
> +	 "\\s+5\\s+<null>\\s+" \
> +	 "\\s+6\\s+<null>\\s+\[^\r\n\]+" \
> +	 "\\s+7\\s+<null>\\s+"] \
> +    "second info inferiors call"
> +
> +# Check after all the new inferiors have been created that we still
> +# only have a single connection.
> +gdb_test "info connections" \
> +    "\\*\\s+1\\s+\\s+core\\s+Local core dump file\\s*"
> +
> +# Now switch to inferior 2 and load the second executable and core
> +# file.  Check the backtrace for the presence of function 'foo', this
> +# indicates we are seeing the correct core file.
> +gdb_test "inferior 2" "Switching to inferior 2 .*"
> +gdb_test "file $binfile2" \
> +    "Reading symbols from .*" \
> +    "Loaded second test binary"
> +gdb_test "core-file $corefile2" \
> +    "Program terminated with signal SIGABRT, Aborted.*" \
> +    "Loaded second core file"
> +gdb_test "bt" "foo \\(\\) at .*" \
> +    "check backtrace in inferior 2"
> +
> +# Switch to inferior 3, this one was cloned from inferior 1, so is
> +# already debugging the first binary file.  Check its backtrace for
> +# 'bar', which indicates we are debugging the correct core file.
> +gdb_test "inferior 3" "Switching to inferior 3 .*"
> +gdb_test "core-file $corefile1" \
> +    "Program terminated with signal SIGABRT, Aborted.*" \
> +    "Loaded first core file into inferior 3"
> +gdb_test "bt" "bar \\(\\) at .*" \
> +    "check backtrace in inferior 3"
> +
> +# Detach from some of the core files and delete some of the inferiors.
> +gdb_test "detach" "No core file now\\." \
> +    "detach from inferior 3 core file"
> +gdb_test "inferior 2" "Switching to inferior 2 .*" \
> +    "switch back to inferior 2"
> +gdb_test_no_output "remove-inferiors 3 4"
> +
> +# Now detach in inferior 2, and delete the inferior.
> +gdb_test "detach" "No core file now\\." \
> +    "detach from inferior 2 core file"
> +gdb_test "inferior 1" "Switching to inferior 1 .*" \
> +    "switch back to inferior 1"
> +gdb_test_no_output "remove-inferiors 2"
> +
> +# Finally, check that inferior 1 backtrace is still working.
> +gdb_test "bt" "bar \\(\\) at .*" \
> +    "check backtrace in inferior 1 again"
> +gdb_test "detach" "No core file now\\." \
> +    "detach from inferior 1 core file"
> -- 
> 2.25.4
> 

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

* Re: [PATCHv2 4/7] gdb: remove the pop_all_targets (and friends) global functions
  2022-10-05 20:49     ` Lancelot SIX
@ 2022-10-06 11:14       ` Andrew Burgess
  0 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-06 11:14 UTC (permalink / raw)
  To: Lancelot SIX; +Cc: gdb-patches

Lancelot SIX <lsix@lancelotsix.com> writes:

> Hi,
>
> I have a minor comment inline below.
>
> On Sun, Oct 02, 2022 at 06:04:45PM +0100, Andrew Burgess via Gdb-patches wrote:
>> This commit removes the global functions pop_all_targets,
>> pop_all_targets_above, and pop_all_targets_at_and_above, and makes
>> them methods on the inferior class.
>> 
>> As the pop_all_targets functions will unpush each target, which
>> decrements the targets reference count, it is possible that the target
>> might be closed.
>> 
>> Right now, closing a target, in some cases, depends on the current
>> inferior being set correctly, that is, to the inferior from which the
>> target was popped.
>> 
>> To facilitate this I have used switch_to_inferior_no_thread within the
>> new methods.  Previously it was the responsibility of the caller to
>> ensure that the correct inferior was selected.
>> 
>> In a couple of places (event-top.c and top.c) I have been able to
>> remove a previous switch_to_inferior_no_thread call.
>> 
>> In remote_unpush_target (remote.c) I have left the
>> switch_to_inferior_no_thread call as it is required for the
>> generic_mourn_inferior call.
>> ---
>>  gdb/event-top.c           |  3 +--
>>  gdb/inferior.c            | 40 ++++++++++++++++++++++++++++++++++++++
>>  gdb/inferior.h            | 20 +++++++++++++++++++
>>  gdb/remote.c              |  2 +-
>>  gdb/scoped-mock-context.h |  2 +-
>>  gdb/target.c              | 41 +--------------------------------------
>>  gdb/target.h              | 11 -----------
>>  gdb/top.c                 |  3 +--
>>  8 files changed, 65 insertions(+), 57 deletions(-)
>> 
>> diff --git a/gdb/event-top.c b/gdb/event-top.c
>> index 88c53b720a9..836e6b935e3 100644
>> --- a/gdb/event-top.c
>> +++ b/gdb/event-top.c
>> @@ -1293,10 +1293,9 @@ async_disconnect (gdb_client_data arg)
>>  
>>    for (inferior *inf : all_inferiors ())
>>      {
>> -      switch_to_inferior_no_thread (inf);
>>        try
>>  	{
>> -	  pop_all_targets ();
>> +	  inf->pop_all_targets ();
>>  	}
>>        catch (const gdb_exception &exception)
>>  	{
>> diff --git a/gdb/inferior.c b/gdb/inferior.c
>> index 7eb2bd97907..2014bf926b7 100644
>> --- a/gdb/inferior.c
>> +++ b/gdb/inferior.c
>> @@ -103,6 +103,46 @@ inferior::unpush_target (struct target_ops *t)
>>    return m_target_stack.unpush (t);
>>  }
>>  
>> +/* See inferior.h.  */
>> +
>> +void inferior::unpush_target_and_assert (struct target_ops *target)
>> +{
>> +  gdb_assert (current_inferior () == this);
>> +
>> +  if (!unpush_target (target))
>> +    internal_error (__FILE__, __LINE__,
>> +		    "pop_all_targets couldn't find target %s\n",
>> +		    target->shortname ());
>> +}
>> +
>> +/* See inferior.h.  */
>> +
>> +void inferior::pop_all_targets_above (enum strata stratum)
>> +{
>> +  /* Unpushing a target might cause it to close.  Some targets currently
>> +     rely on the current_inferior being set for their ::close method, so we
>> +     temporarily switch inferior now.  */
>> +  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
>> +  switch_to_inferior_no_thread (this);
>> +
>> +  while ((int) (top_target ()->stratum ()) > (int) stratum)
>
> Seems to me that the casts to int are not necessary here.  Any reason to
> keep those?

No, I just copied over the existing code.

I've removed the cast in the updated patch below.

Thanks,
Andrew

---

commit 290023b39717030586fca501aa4c80af578b2bb0
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Sep 22 12:22:22 2022 +0100

    gdb: remove the pop_all_targets (and friends) global functions
    
    This commit removes the global functions pop_all_targets,
    pop_all_targets_above, and pop_all_targets_at_and_above, and makes
    them methods on the inferior class.
    
    As the pop_all_targets functions will unpush each target, which
    decrements the targets reference count, it is possible that the target
    might be closed.
    
    Right now, closing a target, in some cases, depends on the current
    inferior being set correctly, that is, to the inferior from which the
    target was popped.
    
    To facilitate this I have used switch_to_inferior_no_thread within the
    new methods.  Previously it was the responsibility of the caller to
    ensure that the correct inferior was selected.
    
    In a couple of places (event-top.c and top.c) I have been able to
    remove a previous switch_to_inferior_no_thread call.
    
    In remote_unpush_target (remote.c) I have left the
    switch_to_inferior_no_thread call as it is required for the
    generic_mourn_inferior call.

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 88c53b720a9..836e6b935e3 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1293,10 +1293,9 @@ async_disconnect (gdb_client_data arg)
 
   for (inferior *inf : all_inferiors ())
     {
-      switch_to_inferior_no_thread (inf);
       try
 	{
-	  pop_all_targets ();
+	  inf->pop_all_targets ();
 	}
       catch (const gdb_exception &exception)
 	{
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 7eb2bd97907..676b37ce6d5 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -103,6 +103,46 @@ inferior::unpush_target (struct target_ops *t)
   return m_target_stack.unpush (t);
 }
 
+/* See inferior.h.  */
+
+void inferior::unpush_target_and_assert (struct target_ops *target)
+{
+  gdb_assert (current_inferior () == this);
+
+  if (!unpush_target (target))
+    internal_error (__FILE__, __LINE__,
+		    "pop_all_targets couldn't find target %s\n",
+		    target->shortname ());
+}
+
+/* See inferior.h.  */
+
+void inferior::pop_all_targets_above (enum strata stratum)
+{
+  /* Unpushing a target might cause it to close.  Some targets currently
+     rely on the current_inferior being set for their ::close method, so we
+     temporarily switch inferior now.  */
+  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
+  switch_to_inferior_no_thread (this);
+
+  while (top_target ()->stratum () > stratum)
+    unpush_target_and_assert (top_target ());
+}
+
+/* See inferior.h.  */
+
+void inferior::pop_all_targets_at_and_above (enum strata stratum)
+{
+  /* Unpushing a target might cause it to close.  Some targets currently
+     rely on the current_inferior being set for their ::close method, so we
+     temporarily switch inferior now.  */
+  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
+  switch_to_inferior_no_thread (this);
+
+  while (top_target ()->stratum () >= stratum)
+    unpush_target_and_assert (top_target ());
+}
+
 void
 inferior::set_tty (std::string terminal_name)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 27765304ece..ab6a209a448 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -380,6 +380,22 @@ class inferior : public refcounted_object,
   target_ops *top_target ()
   { return m_target_stack.top (); }
 
+  /* Unpush all targets except the dummy target from m_target_stack.  As
+     targets are removed from m_target_stack their reference count is
+     decremented, which may cause a target to close.  */
+  void pop_all_targets ()
+  { pop_all_targets_above (dummy_stratum); }
+
+  /* Unpush all targets above STRATUM from m_target_stack.  As targets are
+     removed from m_target_stack their reference count is decremented,
+     which may cause a target to close.  */
+  void pop_all_targets_above (enum strata stratum);
+
+  /* Unpush all targets at and above STRATUM from m_target_stack.  As
+     targets are removed from m_target_stack their reference count is
+     decremented, which may cause a target to close.  */
+  void pop_all_targets_at_and_above (enum strata stratum);
+
   /* Return the target at process_stratum level in this inferior's
      target stack.  */
   struct process_stratum_target *process_target ()
@@ -598,6 +614,10 @@ class inferior : public refcounted_object,
   registry<inferior> registry_fields;
 
 private:
+
+  /* Unpush TARGET and assert that it worked.  */
+  void unpush_target_and_assert (struct target_ops *target);
+
   /* The inferior's target stack.  */
   target_stack m_target_stack;
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 5a71c41d61e..4864e9a55c3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5721,7 +5721,7 @@ remote_unpush_target (remote_target *target)
   for (inferior *inf : all_inferiors (target))
     {
       switch_to_inferior_no_thread (inf);
-      pop_all_targets_at_and_above (process_stratum);
+      inf->pop_all_targets_at_and_above (process_stratum);
       generic_mourn_inferior ();
     }
 
diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
index a9895303015..87c1df0d206 100644
--- a/gdb/scoped-mock-context.h
+++ b/gdb/scoped-mock-context.h
@@ -71,7 +71,7 @@ struct scoped_mock_context
   ~scoped_mock_context ()
   {
     inferior_list.erase (inferior_list.iterator_to (mock_inferior));
-    pop_all_targets_at_and_above (process_stratum);
+    mock_inferior.pop_all_targets_at_and_above (process_stratum);
   }
 };
 
diff --git a/gdb/target.c b/gdb/target.c
index 0f4d6d01057..1e447f604d9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1238,45 +1238,6 @@ target_stack::unpush (target_ops *t)
   return true;
 }
 
-/* Unpush TARGET and assert that it worked.  */
-
-static void
-unpush_target_and_assert (struct target_ops *target)
-{
-  if (!current_inferior ()->unpush_target (target))
-    {
-      gdb_printf (gdb_stderr,
-		  "pop_all_targets couldn't find target %s\n",
-		  target->shortname ());
-      internal_error (__FILE__, __LINE__,
-		      _("failed internal consistency check"));
-    }
-}
-
-void
-pop_all_targets_above (enum strata above_stratum)
-{
-  while ((int) (current_inferior ()->top_target ()->stratum ())
-	 > (int) above_stratum)
-    unpush_target_and_assert (current_inferior ()->top_target ());
-}
-
-/* See target.h.  */
-
-void
-pop_all_targets_at_and_above (enum strata stratum)
-{
-  while ((int) (current_inferior ()->top_target ()->stratum ())
-	 >= (int) stratum)
-    unpush_target_and_assert (current_inferior ()->top_target ());
-}
-
-void
-pop_all_targets (void)
-{
-  pop_all_targets_above (dummy_stratum);
-}
-
 void
 target_unpusher::operator() (struct target_ops *ops) const
 {
@@ -2533,7 +2494,7 @@ target_preopen (int from_tty)
      it doesn't (which seems like a win for UDI), remove it now.  */
   /* Leave the exec target, though.  The user may be switching from a
      live process to a core of the same program.  */
-  pop_all_targets_above (file_stratum);
+  current_inferior ()->pop_all_targets_above (file_stratum);
 
   target_pre_inferior (from_tty);
 }
diff --git a/gdb/target.h b/gdb/target.h
index 68446a39c1b..547ee8a3bbd 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2389,17 +2389,6 @@ extern void target_pre_inferior (int);
 
 extern void target_preopen (int);
 
-/* Does whatever cleanup is required to get rid of all pushed targets.  */
-extern void pop_all_targets (void);
-
-/* Like pop_all_targets, but pops only targets whose stratum is at or
-   above STRATUM.  */
-extern void pop_all_targets_at_and_above (enum strata stratum);
-
-/* Like pop_all_targets, but pops only targets whose stratum is
-   strictly above ABOVE_STRATUM.  */
-extern void pop_all_targets_above (enum strata above_stratum);
-
 extern CORE_ADDR target_translate_tls_address (struct objfile *objfile,
 					       CORE_ADDR offset);
 
diff --git a/gdb/top.c b/gdb/top.c
index 54c7c922142..5f64c6bddf0 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1845,10 +1845,9 @@ quit_force (int *exit_arg, int from_tty)
      them all out.  */
   for (inferior *inf : all_inferiors ())
     {
-      switch_to_inferior_no_thread (inf);
       try
 	{
-	  pop_all_targets ();
+	  inf->pop_all_targets ();
 	}
       catch (const gdb_exception &ex)
 	{


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

* Re: [PATCHv2 7/7] gdb: some process_stratum_target should not be shared
  2022-10-05 21:15     ` Lancelot SIX
@ 2022-10-06 11:44       ` Andrew Burgess
  0 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-10-06 11:44 UTC (permalink / raw)
  To: Lancelot SIX; +Cc: gdb-patches

Lancelot SIX <lsix@lancelotsix.com> writes:

> Hi,
>
> Thanks for doing this!
>
> I have a couple of comments below.
>
> On Sun, Oct 02, 2022 at 06:04:48PM +0100, Andrew Burgess via Gdb-patches wrote:
>> When multi-target support was added to GDB, an assumption was made
>> that all process_stratum_target sub-classes could be shared by
>> multiple inferiors.
>> 
>> For things like the Linux and FreeBSD native targets, as well as the
>> remote target, this is absolutely true (or was made true).  But some
>> targets were never updated to be shareable, for example, the
>> core_target, which is used when reading core-files, stores some of its
>> state in the program_space, but also, the core-file and the executable
>> being debugged are closely related.
>> 
>> As each add-inferior call creates an inferior with a new
>> program_space, and doesn't automatically copy the executable, or the
>> current core-file, I don't think it really makes sense to "share"
>> core_target objects between inferiors.
>> 
>> Consider this session:
>> 
>>   $ gdb -q
>>   (gdb) file test1
>>   Reading symbols from test1...
>>   (gdb) core-file core.test1.433190
>>   [New LWP 433190]
>>   Core was generated by `./test1'.
>>   Program terminated with signal SIGSEGV, Segmentation fault.
>>   #0  0x0000000000401111 in foo () at test1.c:6
>>   6	  return *global_ptr;
>>   (gdb) add-inferior
>>   [New inferior 2]
>>   Added inferior 2 on connection 1 (core)
>>   (gdb) info inferiors
>>     Num  Description       Connection           Executable
>>   * 1    process 433190    1 (core)             /tmp/multi-core/test1
>>     2    <null>            1 (core)
>>   (gdb) info connections
>>     Num  What  Description
>>   * 1    core  Local core dump file
>>   (gdb) inferior 2
>>   [Switching to inferior 2 [<null>] (<noexec>)]
>>   (gdb) file test2
>>   Reading symbols from test2...
>>   (gdb) core-file core.test2.433203
>>   [New LWP 433203]
>>   Core was generated by `./test2'.
>>   Program terminated with signal SIGSEGV, Segmentation fault.
>>   #0  0x0000000000401111 in main () at test2.c:6
>>   6	  return *global_ptr;
>>   (gdb) info inferiors
>>     Num  Description       Connection           Executable
>>     1    process 433190    1 (core)             /tmp/multi-core/test1
>>   * 2    process 433203    2 (core)             /tmp/multi-core/test2
>>   (gdb) info connections
>>     Num  What  Description
>>     1    core  Local core dump file
>>   * 2    core  Local core dump file
>>   (gdb)
>> 
>> After the 'add-inferior' the core_target connection is shared between
>> the inferiors.  However, as soon as the user sets up the core-file and
>> executable in the new inferior a new core connection has been created.
>> 
>> I think this behaviour might be confusing, so I'd like to have GDB not
>> initially share the core connection.  Instead, when the user tries to
>> add the new inferior a warning is given, and the new inferior is
>> created without a connection, like this:
>> 
>>   $ gdb -q
>>   (gdb) file test1
>>   Reading symbols from test1...
>>   (gdb) core-file core.test1.433190
>>   [New LWP 433190]
>>   Core was generated by `./test1'.
>>   Program terminated with signal SIGSEGV, Segmentation fault.
>>   #0  0x0000000000401111 in foo () at test1.c:6
>>   6	  return *global_ptr;
>>   (gdb) add-inferior
>>   [New inferior 2]
>>   warning: can't share connection 1 (core) between inferiors
>>   Added inferior 2
>>   (gdb) info inferiors
>>     Num  Description       Connection           Executable
>>   * 1    process 433190    1 (core)             /tmp/multi-core/test1
>>     2    <null>
>>   (gdb)
>> 
>> If the user explicitly asks for the new inferior to be created without
>> a connection, then no warning will be given.
>> 
>> At this point the user is free to setup inferior 2 with a different
>> executable and core file (or to do anything they like with the
>> inferior).
>> 
>> In an earlier version of this patch I had GDB error instead of giving
>> a warning.  However, the error message ended up being something like:
>> 
>>   can't share connection ..... between inferiors, use -no-connection
>>       option to create an inferior without sharing a connection.
>> 
>> but it seemed better to just create the inferior.
>> 
>> I've updated the docs, and added a NEWS entry for the new warning.  In
>> the docs for clone-inferior I've added reference to -no-connection,
>> which was previously missing.
>> ---
>>  gdb/NEWS                                     |   7 +
>>  gdb/corelow.c                                |   5 +
>>  gdb/doc/gdb.texinfo                          |  37 ++++-
>>  gdb/inferior.c                               |  16 ++
>>  gdb/inferior.h                               |   6 +-
>>  gdb/target.c                                 |  14 ++
>>  gdb/target.h                                 |   8 +
>>  gdb/testsuite/gdb.multi/multi-core-files-1.c |  37 +++++
>>  gdb/testsuite/gdb.multi/multi-core-files-2.c |  31 ++++
>>  gdb/testsuite/gdb.multi/multi-core-files.exp | 156 +++++++++++++++++++
>>  10 files changed, 313 insertions(+), 4 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-1.c
>>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-2.c
>>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files.exp
>> 
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index a6ea7c9f98f..cb576edd203 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -71,6 +71,13 @@
>>    For both /r and /b GDB is now better at using whitespace in order to
>>    align the disassembled instruction text.
>>  
>> +* The add-inferior, clone-inferior, and MI -add-inferior commands will
>> +  now give a warning, and create the new inferior without a
>> +  connection, when the current inferior, at the time the command is
>> +  given, is a core-file target.  The core-file target could never
>> +  really be shared between inferiors, GDB is now more vocal about what
>> +  is going on.
>> +
>>  * New commands
>>  
>>  maintenance set ignore-prologue-end-flag on|off
>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index 293bc8d4f59..4d8393a3587 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -128,6 +128,11 @@ class core_target final : public process_stratum_target
>>    /* See definition.  */
>>    void info_proc_mappings (struct gdbarch *gdbarch);
>>  
>> +  /* The core_target only works for the inferior in which it was initially
>> +     opened, and can't be copied to some other inferior's target_stack.  */
>> +  bool is_shareable () override
>> +  { return false; }
>> +
>>  private: /* per-core data */
>>  
>>    /* Get rid of the core inferior.  */
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 107df84d108..a558fc4de38 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -3358,8 +3358,31 @@
>>  remote} command to connect to some other @code{gdbserver} instance,
>>  use @code{run} to spawn a local program, etc.
>>  
>> +Not all connections can be shared between inferiors.  For example, the
>> +@code{target core} target is unique for each inferior.  That is,
>> +multiple inferiors can use @code{target core} at the same time, but
>> +each @code{target core} is different.  If you try to
>> +@code{add-inferior}, and the current inferior is @code{target core},
>> +then @value{GDBN} will give a warning and create the new inferior
>> +without a connection, like this:
>> +
>> +@smallexample
>> +(@value{GDBP}) file test1
>> +Reading symbols from test1...
>> +(@value{GDBP}) target core core.test1.433190
>> +[New LWP 433190]
>> +Core was generated by `./test1'.
>> +Program terminated with signal SIGSEGV, Segmentation fault.
>> +#0  0x0000000000401111 in foo () at test1.c:6
>> +6	  return *global_ptr;
>> +(@value{GDBP}) add-inferior
>> +[New inferior 2]
>> +warning: can't share connection 1 (core) between inferiors
>> +Added inferior 2
>> +@end smallexample
>> +
>>  @kindex clone-inferior
>> -@item clone-inferior [ -copies @var{n} ] [ @var{infno} ]
>> +@item clone-inferior [ -copies @var{n} ] [ -no-connection ] [ @var{infno} ]
>>  Adds @var{n} inferiors ready to execute the same program as inferior
>>  @var{infno}; @var{n} defaults to 1, and @var{infno} defaults to the
>>  number of the current inferior.  This command copies the values of the
>> @@ -3384,6 +3407,13 @@
>>  
>>  You can now simply switch focus to inferior 2 and run it.
>>  
>> +Like @code{add-inferior}, @code{clone-inferior} shares the connection
>> +with the inferior @var{infno}.  If the @var{-no-connection} option is
>> +given then the new inferior will be created without a connection.  If
>> +the connection of inferior @var{infno} can't be shared, then
>> +@value{GDBN} will give a warning, and the new inferior will be created
>> +without a connection.
>> +
>>  @kindex remove-inferiors
>>  @item remove-inferiors @var{infno}@dots{}
>>  Removes the inferior or inferiors @var{infno}@dots{}.  It is not
>> @@ -37786,6 +37816,11 @@
>>  @code{gdbserver} instance, use @code{-exec-run} to spawn a local
>>  program, etc.
>>  
>> +If the connection of the current inferior cannot be shared, e.g.@: the
>> +@code{-target-select core} target cannot be shared between inferiors,
>> +then @value{GDBN} will give a warning and create the new inferior
>> +without a connection.
>> +
>>  The command response always has a field, @var{inferior}, whose value
>>  is the identifier of the thread group corresponding to the new
>>  inferior.
>> diff --git a/gdb/inferior.c b/gdb/inferior.c
>> index a498b9d493c..0868a55593a 100644
>> --- a/gdb/inferior.c
>> +++ b/gdb/inferior.c
>> @@ -817,6 +817,22 @@ switch_to_inferior_and_push_target (inferior *new_inf,
>>       symbols.  */
>>    switch_to_inferior_no_thread (new_inf);
>>  
>> +  if (!no_connection && proc_target != nullptr
>> +      && !proc_target->is_shareable ())
>> +    {
>> +      if (proc_target->connection_string () != nullptr)
>> +	warning (_("can't share connection %d (%s %s) between inferiors"),
>> +		 proc_target->connection_number,
>> +		 proc_target->shortname (),
>> +		 proc_target->connection_string ());
>> +      else
>> +	warning (_("can't share connection %d (%s) between inferiors"),
>> +		 proc_target->connection_number,
>> +		 proc_target->shortname ());
>> +
>> +      proc_target = nullptr;
>> +    }
>> +
>>    /* Reuse the target for new inferior.  */
>>    if (!no_connection && proc_target != NULL)
>>      {
>> diff --git a/gdb/inferior.h b/gdb/inferior.h
>> index 344974c4811..639364e5de3 100644
>> --- a/gdb/inferior.h
>> +++ b/gdb/inferior.h
>> @@ -762,9 +762,9 @@ extern struct inferior *add_inferior_with_spaces (void);
>>  /* Print the current selected inferior.  */
>>  extern void print_selected_inferior (struct ui_out *uiout);
>>  
>> -/* Switch to inferior NEW_INF, a new inferior, and unless
>> -   NO_CONNECTION is true, push the process_stratum_target of ORG_INF
>> -   to NEW_INF.  */
>> +/* Switch to inferior NEW_INF, a new inferior, and unless NO_CONNECTION is
>> +   true, or the process_stratum_target of ORG_INF is not shareable, push
>> +   the process_stratum_target of ORG_INF to NEW_INF.  */
>>  
>>  extern void switch_to_inferior_and_push_target
>>    (inferior *new_inf, bool no_connection, inferior *org_inf);
>> diff --git a/gdb/target.c b/gdb/target.c
>> index 1e447f604d9..65bc0e7246e 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -1188,6 +1188,12 @@ target_stack::push (target_ops *t)
>>    if (m_stack[stratum].get () != nullptr)
>>      unpush (m_stack[stratum].get ());
>>  
>> +  /* If this target can't be shared, then check that the target doesn't
>> +     already appear on some other target stack.  */
>> +  if (!t->is_shareable ())
>> +    for (inferior *inf : all_inferiors ())
>> +      gdb_assert (!inf->target_is_pushed (t));
>> +
>>    /* Now add the new one.  */
>>    m_stack[stratum] = std::move (ref);
>>  
>> @@ -3226,6 +3232,14 @@ target_ops::fileio_readlink (struct inferior *inf, const char *filename,
>>  
>>  /* See target.h.  */
>>  
>> +bool
>> +target_ops::is_shareable ()
>> +{
>> +  return true;
>> +}
>> +
>> +/* See target.h.  */
>> +
>>  int
>>  target_fileio_open (struct inferior *inf, const char *filename,
>>  		    int flags, int mode, bool warn_if_slow, fileio_error *target_errno)
>> diff --git a/gdb/target.h b/gdb/target.h
>> index 547ee8a3bbd..30e5085a543 100644
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -1321,6 +1321,14 @@ struct target_ops
>>      virtual bool store_memtags (CORE_ADDR address, size_t len,
>>  				const gdb::byte_vector &tags, int type)
>>        TARGET_DEFAULT_NORETURN (tcomplain ());
>> +
>> +    /* Return true if this target can be shared on multiple target_stacks,
>> +       or false if this target should only appear on a single target_stack.
>> +       When this function returns false multiple separate instances of the
>> +       same target_ops sub-class can still appear on different
>> +       target_stacks, but the same concrete instance can only appear on a
>> +       single target_stack.  */
>> +    virtual bool is_shareable ();
>>    };
>>  
>>  /* Deleter for std::unique_ptr.  See comments in
>> diff --git a/gdb/testsuite/gdb.multi/multi-core-files-1.c b/gdb/testsuite/gdb.multi/multi-core-files-1.c
>> new file mode 100644
>> index 00000000000..f996973023e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.multi/multi-core-files-1.c
>> @@ -0,0 +1,37 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 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/>.  */
>> +
>> +#include <stdlib.h>
>> +
>> +int
>> +bar ()
>> +{
>> +  abort ();
>> +  return 0;
>> +}
>> +
>> +int
>> +baz ()
>> +{
>> +  return bar ();
>> +}
>> +
>> +int
>> +main ()
>> +{
>> +  return baz ();
>> +}
>> diff --git a/gdb/testsuite/gdb.multi/multi-core-files-2.c b/gdb/testsuite/gdb.multi/multi-core-files-2.c
>> new file mode 100644
>> index 00000000000..fb99e137c3f
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.multi/multi-core-files-2.c
>> @@ -0,0 +1,31 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 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/>.  */
>> +
>> +#include <stdlib.h>
>> +
>> +int
>> +foo ()
>> +{
>> +  abort ();
>> +  return 0;
>> +}
>> +
>> +int
>> +main ()
>> +{
>> +  return foo ();
>> +}
>> diff --git a/gdb/testsuite/gdb.multi/multi-core-files.exp b/gdb/testsuite/gdb.multi/multi-core-files.exp
>> new file mode 100644
>> index 00000000000..9cf188b0caa
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.multi/multi-core-files.exp
>> @@ -0,0 +1,156 @@
>> +# Copyright 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/>.
>> +
>> +# This script runs some basic tests that GDB can support multiple
>> +# inferiors each debugging different core files.
>> +#
>> +# We also check the behaviour of GDB if the user attempts to clone or
>> +# duplicate an inferior that is debugging a core file.
>> +
>> +standard_testfile -1.c -2.c
>> +
>> +set binfile1 "${binfile}-1"
>> +set binfile2 "${binfile}-2"
>> +
>> +if {[build_executable "build first executable" $binfile1 $srcfile \
>> +	 debug] == -1} {
>> +    untested "failed to compile first executable"
>> +    return -1
>> +}
>> +
>> +if {[build_executable "build first executable" $binfile2 $srcfile2 \
>
> s/first/second/ maybe?
>
>> +	 debug] == -1} {
>> +    untested "failed to compile second executable"
>> +    return -1
>> +}
>> +
>> +set corefile1 [core_find $binfile1]
>> +set corefile2 [core_find $binfile2]
>
> I think you should check corefile*.
>
> On my system (where ulimit -c gives 0), I have:
>
>     WARNING: can't generate a core file - core tests suppressed - check ulimit -c
>     WARNING: can't generate a core file - core tests suppressed - check ulimit -c
>     FAIL: gdb.multi/multi-core-files.exp: load core file
>     FAIL: gdb.multi/multi-core-files.exp: check backtrace in inferior 1
>     FAIL: gdb.multi/multi-core-files.exp: add-inferior
>     FAIL: gdb.multi/multi-core-files.exp: clone-inferior
>     FAIL: gdb.multi/multi-core-files.exp: interpreter-exec mi "-add-inferior"
>     FAIL: gdb.multi/multi-core-files.exp: first info inferiors call
>     FAIL: gdb.multi/multi-core-files.exp: second info inferiors call
>     FAIL: gdb.multi/multi-core-files.exp: info connections
>     FAIL: gdb.multi/multi-core-files.exp: Loaded second core file
>     FAIL: gdb.multi/multi-core-files.exp: check backtrace in inferior 2
>     FAIL: gdb.multi/multi-core-files.exp: Loaded first core file into inferior 3
>     FAIL: gdb.multi/multi-core-files.exp: check backtrace in inferior 3
>     FAIL: gdb.multi/multi-core-files.exp: detach from inferior 3 core file (the program is no longer running)
>     FAIL: gdb.multi/multi-core-files.exp: detach from inferior 2 core file (the program is no longer running)
>     FAIL: gdb.multi/multi-core-files.exp: check backtrace in inferior 1 again
>     FAIL: gdb.multi/multi-core-files.exp: detach from inferior 1 core file (the program is no longer running)
>
> I think something like this could do:
>
>     if { $corefile1 eq "" || $corefile2 eq "" } {
>       untested "Can't generate core files"
>       return
>     }
>

Thanks for spotting this.  I included the fix you suggested, and
confirmed that this resolves the FAILs when 'ulimit -c 0' is in use.

I've also fixed all the typos you pointed out.

An updated patch is below.

Thanks,
Andrew

---

commit df38705e87e25e24dbca7b04d0ac3a61affe714f
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Sep 29 12:45:26 2022 +0100

    gdb: some process_stratum_target should not be shared
    
    When multi-target support was added to GDB, an assumption was made
    that all process_stratum_target sub-classes could be shared by
    multiple inferiors.
    
    For things like the Linux and FreeBSD native targets, as well as the
    remote target, this is absolutely true (or was made true).  But some
    targets were never updated to be shareable, for example, the
    core_target, which is used when reading core-files, stores some of its
    state in the program_space, but also, the core-file and the executable
    being debugged are closely related.
    
    As each add-inferior call creates an inferior with a new
    program_space, and doesn't automatically copy the executable, or the
    current core-file, I don't think it really makes sense to "share"
    core_target objects between inferiors.
    
    Consider this session:
    
      $ gdb -q
      (gdb) file test1
      Reading symbols from test1...
      (gdb) core-file core.test1.433190
      [New LWP 433190]
      Core was generated by `./test1'.
      Program terminated with signal SIGSEGV, Segmentation fault.
      #0  0x0000000000401111 in foo () at test1.c:6
      6       return *global_ptr;
      (gdb) add-inferior
      [New inferior 2]
      Added inferior 2 on connection 1 (core)
      (gdb) info inferiors
        Num  Description       Connection           Executable
      * 1    process 433190    1 (core)             /tmp/multi-core/test1
        2    <null>            1 (core)
      (gdb) info connections
        Num  What  Description
      * 1    core  Local core dump file
      (gdb) inferior 2
      [Switching to inferior 2 [<null>] (<noexec>)]
      (gdb) file test2
      Reading symbols from test2...
      (gdb) core-file core.test2.433203
      [New LWP 433203]
      Core was generated by `./test2'.
      Program terminated with signal SIGSEGV, Segmentation fault.
      #0  0x0000000000401111 in main () at test2.c:6
      6       return *global_ptr;
      (gdb) info inferiors
        Num  Description       Connection           Executable
        1    process 433190    1 (core)             /tmp/multi-core/test1
      * 2    process 433203    2 (core)             /tmp/multi-core/test2
      (gdb) info connections
        Num  What  Description
        1    core  Local core dump file
      * 2    core  Local core dump file
      (gdb)
    
    After the 'add-inferior' the core_target connection is shared between
    the inferiors.  However, as soon as the user sets up the core-file and
    executable in the new inferior a new core connection has been created.
    
    I think this behaviour might be confusing, so I'd like to have GDB not
    initially share the core connection.  Instead, when the user tries to
    add the new inferior a warning is given, and the new inferior is
    created without a connection, like this:
    
      $ gdb -q
      (gdb) file test1
      Reading symbols from test1...
      (gdb) core-file core.test1.433190
      [New LWP 433190]
      Core was generated by `./test1'.
      Program terminated with signal SIGSEGV, Segmentation fault.
      #0  0x0000000000401111 in foo () at test1.c:6
      6       return *global_ptr;
      (gdb) add-inferior
      [New inferior 2]
      warning: can't share connection 1 (core) between inferiors
      Added inferior 2
      (gdb) info inferiors
        Num  Description       Connection           Executable
      * 1    process 433190    1 (core)             /tmp/multi-core/test1
        2    <null>
      (gdb)
    
    If the user explicitly asks for the new inferior to be created without
    a connection, then no warning will be given.
    
    At this point the user is free to setup inferior 2 with a different
    executable and core file (or to do anything they like with the
    inferior).
    
    In an earlier version of this patch I had GDB error instead of giving
    a warning.  However, the error message ended up being something like:
    
      can't share connection ..... between inferiors, use -no-connection
          option to create an inferior without sharing a connection.
    
    but it seemed better to just create the inferior.
    
    I've updated the docs, and added a NEWS entry for the new warning.  In
    the docs for clone-inferior I've added reference to -no-connection,
    which was previously missing.

diff --git a/gdb/NEWS b/gdb/NEWS
index a6ea7c9f98f..cb576edd203 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -71,6 +71,13 @@
   For both /r and /b GDB is now better at using whitespace in order to
   align the disassembled instruction text.
 
+* The add-inferior, clone-inferior, and MI -add-inferior commands will
+  now give a warning, and create the new inferior without a
+  connection, when the current inferior, at the time the command is
+  given, is a core-file target.  The core-file target could never
+  really be shared between inferiors, GDB is now more vocal about what
+  is going on.
+
 * New commands
 
 maintenance set ignore-prologue-end-flag on|off
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 293bc8d4f59..4d8393a3587 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -128,6 +128,11 @@ class core_target final : public process_stratum_target
   /* See definition.  */
   void info_proc_mappings (struct gdbarch *gdbarch);
 
+  /* The core_target only works for the inferior in which it was initially
+     opened, and can't be copied to some other inferior's target_stack.  */
+  bool is_shareable () override
+  { return false; }
+
 private: /* per-core data */
 
   /* Get rid of the core inferior.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 107df84d108..a5009c85e09 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3358,8 +3358,31 @@
 remote} command to connect to some other @code{gdbserver} instance,
 use @code{run} to spawn a local program, etc.
 
+Not all connections can be shared between inferiors.  For example, the
+@code{target core} target is unique for each inferior.  That is,
+multiple inferiors can use @code{target core} at the same time, but
+each @code{target core} is different.  If you try to
+@code{add-inferior}, and the current inferior is @code{target core},
+then @value{GDBN} will give a warning and create the new inferior
+without a connection, like this:
+
+@smallexample
+(@value{GDBP}) file test1
+Reading symbols from test1...
+(@value{GDBP}) target core core.test1.433190
+[New LWP 433190]
+Core was generated by `./test1'.
+Program terminated with signal SIGSEGV, Segmentation fault.
+#0  0x0000000000401111 in foo () at test1.c:6
+6	  return *global_ptr;
+(@value{GDBP}) add-inferior
+[New inferior 2]
+warning: can't share connection 1 (core) between inferiors
+Added inferior 2
+@end smallexample
+
 @kindex clone-inferior
-@item clone-inferior [ -copies @var{n} ] [ @var{infno} ]
+@item clone-inferior [ -copies @var{n} ] [ -no-connection ] [ @var{infno} ]
 Adds @var{n} inferiors ready to execute the same program as inferior
 @var{infno}; @var{n} defaults to 1, and @var{infno} defaults to the
 number of the current inferior.  This command copies the values of the
@@ -3384,6 +3407,13 @@
 
 You can now simply switch focus to inferior 2 and run it.
 
+Like @code{add-inferior}, @code{clone-inferior} shares the connection
+with the inferior @var{infno}.  If the @var{-no-connection} option is
+given, then the new inferior will be created without a connection.  If
+the connection of inferior @var{infno} can't be shared, then
+@value{GDBN} will give a warning, and the new inferior will be created
+without a connection.
+
 @kindex remove-inferiors
 @item remove-inferiors @var{infno}@dots{}
 Removes the inferior or inferiors @var{infno}@dots{}.  It is not
@@ -37786,6 +37816,11 @@
 @code{gdbserver} instance, use @code{-exec-run} to spawn a local
 program, etc.
 
+If the connection of the current inferior cannot be shared, e.g.@: the
+@code{-target-select core} target cannot be shared between inferiors,
+then @value{GDBN} will give a warning and create the new inferior
+without a connection.
+
 The command response always has a field, @var{inferior}, whose value
 is the identifier of the thread group corresponding to the new
 inferior.
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 26b242ed1bc..ccc4d451e2f 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -817,6 +817,22 @@ switch_to_inferior_and_push_target (inferior *new_inf,
      symbols.  */
   switch_to_inferior_no_thread (new_inf);
 
+  if (!no_connection && proc_target != nullptr
+      && !proc_target->is_shareable ())
+    {
+      if (proc_target->connection_string () != nullptr)
+	warning (_("can't share connection %d (%s %s) between inferiors"),
+		 proc_target->connection_number,
+		 proc_target->shortname (),
+		 proc_target->connection_string ());
+      else
+	warning (_("can't share connection %d (%s) between inferiors"),
+		 proc_target->connection_number,
+		 proc_target->shortname ());
+
+      proc_target = nullptr;
+    }
+
   /* Reuse the target for new inferior.  */
   if (!no_connection && proc_target != NULL)
     {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index ab6a209a448..7aa5f157034 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -762,9 +762,9 @@ extern struct inferior *add_inferior_with_spaces (void);
 /* Print the current selected inferior.  */
 extern void print_selected_inferior (struct ui_out *uiout);
 
-/* Switch to inferior NEW_INF, a new inferior, and unless
-   NO_CONNECTION is true, push the process_stratum_target of ORG_INF
-   to NEW_INF.  */
+/* Switch to inferior NEW_INF, a new inferior, and unless NO_CONNECTION is
+   true, or the process_stratum_target of ORG_INF is not shareable, push
+   the process_stratum_target of ORG_INF to NEW_INF.  */
 
 extern void switch_to_inferior_and_push_target
   (inferior *new_inf, bool no_connection, inferior *org_inf);
diff --git a/gdb/target.c b/gdb/target.c
index 1e447f604d9..65bc0e7246e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1188,6 +1188,12 @@ target_stack::push (target_ops *t)
   if (m_stack[stratum].get () != nullptr)
     unpush (m_stack[stratum].get ());
 
+  /* If this target can't be shared, then check that the target doesn't
+     already appear on some other target stack.  */
+  if (!t->is_shareable ())
+    for (inferior *inf : all_inferiors ())
+      gdb_assert (!inf->target_is_pushed (t));
+
   /* Now add the new one.  */
   m_stack[stratum] = std::move (ref);
 
@@ -3226,6 +3232,14 @@ target_ops::fileio_readlink (struct inferior *inf, const char *filename,
 
 /* See target.h.  */
 
+bool
+target_ops::is_shareable ()
+{
+  return true;
+}
+
+/* See target.h.  */
+
 int
 target_fileio_open (struct inferior *inf, const char *filename,
 		    int flags, int mode, bool warn_if_slow, fileio_error *target_errno)
diff --git a/gdb/target.h b/gdb/target.h
index 547ee8a3bbd..30e5085a543 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1321,6 +1321,14 @@ struct target_ops
     virtual bool store_memtags (CORE_ADDR address, size_t len,
 				const gdb::byte_vector &tags, int type)
       TARGET_DEFAULT_NORETURN (tcomplain ());
+
+    /* Return true if this target can be shared on multiple target_stacks,
+       or false if this target should only appear on a single target_stack.
+       When this function returns false multiple separate instances of the
+       same target_ops sub-class can still appear on different
+       target_stacks, but the same concrete instance can only appear on a
+       single target_stack.  */
+    virtual bool is_shareable ();
   };
 
 /* Deleter for std::unique_ptr.  See comments in
diff --git a/gdb/testsuite/gdb.multi/multi-core-files-1.c b/gdb/testsuite/gdb.multi/multi-core-files-1.c
new file mode 100644
index 00000000000..f996973023e
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files-1.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <stdlib.h>
+
+int
+bar ()
+{
+  abort ();
+  return 0;
+}
+
+int
+baz ()
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  return baz ();
+}
diff --git a/gdb/testsuite/gdb.multi/multi-core-files-2.c b/gdb/testsuite/gdb.multi/multi-core-files-2.c
new file mode 100644
index 00000000000..fb99e137c3f
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files-2.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <stdlib.h>
+
+int
+foo ()
+{
+  abort ();
+  return 0;
+}
+
+int
+main ()
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.multi/multi-core-files.exp b/gdb/testsuite/gdb.multi/multi-core-files.exp
new file mode 100644
index 00000000000..22a56c07ceb
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files.exp
@@ -0,0 +1,160 @@
+# Copyright 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/>.
+
+# This script runs some basic tests that GDB can support multiple
+# inferiors each debugging different core files.
+#
+# We also check the behaviour of GDB if the user attempts to clone or
+# duplicate an inferior that is debugging a core file.
+
+standard_testfile -1.c -2.c
+
+set binfile1 "${binfile}-1"
+set binfile2 "${binfile}-2"
+
+if {[build_executable "build first executable" $binfile1 $srcfile \
+	 debug] == -1} {
+    untested "failed to compile first executable"
+    return -1
+}
+
+if {[build_executable "build second executable" $binfile2 $srcfile2 \
+	 debug] == -1} {
+    untested "failed to compile second executable"
+    return -1
+}
+
+set corefile1 [core_find $binfile1]
+set corefile2 [core_find $binfile2]
+if { $corefile1 == "" || $corefile2 == "" } {
+    untested "Can't generate core files"
+    return
+}
+
+# Start GDB, and load the first executable and corefile into the first
+# inferior.
+clean_restart ${binfile1}
+gdb_test "core-file $corefile1" "Program terminated with .*" \
+    "load core file"
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 1"
+
+# Try to use add-inferior and clone-inferior to create new
+# inferiors.  In both cases this will try to share the core_target
+# between inferior 1 and the new inferior.  As the core_target can't
+# be shared we should get a warning, and the inferior should be
+# created without a connection.
+gdb_test "add-inferior" \
+    [multi_line \
+	 "\\\[New inferior 2\\\]" \
+	 "warning: can't share connection 1 \\(core\\) between inferiors" \
+	 "Added inferior 2"]
+gdb_test "clone-inferior" \
+    [multi_line \
+	 "\\\[New inferior 3\\\]" \
+	 "warning: can't share connection 1 \\(core\\) between inferiors" \
+	 "Added inferior 3"]
+
+# Check the MI -add-inferior command.  Do this using interpreter-exec.
+# We're not doing a full MI test here, just checking this one command.
+gdb_test "interpreter-exec mi \"-add-inferior\"" \
+    [multi_line \
+	 "~\"\\\[New inferior 4\\\]..\"" \
+	 "&\"warning: can't share connection 1 \\(core\\) between inferiors..\"" \
+	 "~\"Added inferior 4..\"" \
+	 "\\^done,inferior=\"\[^\"\]+\""]
+
+# Now check that none of the new inferiors have a connection.
+gdb_test "info inferiors" \
+    [multi_line \
+	 "\\*\\s+1\\s+\[^\r\n\]+\\s+1 \\(core\\)\\s+\[^\r\n\]+.*" \
+	 "\\s+2\\s+<null>\\s+" \
+	 "\\s+3\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+4\\s+<null>\\s+"] \
+    "first info inferiors call"
+
+# Now use add-inferior and clone-inferior but this time with the
+# -no-connection option, this should avoid issuing the warning.  We
+# also use interpreter-exec to test the MI version of this command.
+gdb_test "add-inferior -no-connection" \
+    [multi_line \
+	 "\\\[New inferior 5\\\]" \
+	 "Added inferior 5"]
+gdb_test "clone-inferior -no-connection" \
+    [multi_line \
+	 "\\\[New inferior 6\\\]" \
+	 "Added inferior 6"]
+gdb_test "interpreter-exec mi \"-add-inferior --no-connection\"" \
+    "\\\[New inferior 7\\\].*Added inferior 7.*"
+
+# Now check that none of the new inferiors have a connection.
+gdb_test "info inferiors" \
+    [multi_line \
+	 "\\*\\s+1\\s+\[^\r\n\]+\\s+1 \\(core\\)\\s+\[^\r\n\]+.*" \
+	 "\\s+2\\s+<null>\\s+" \
+	 "\\s+3\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+4\\s+<null>\\s+" \
+	 "\\s+5\\s+<null>\\s+" \
+	 "\\s+6\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+7\\s+<null>\\s+"] \
+    "second info inferiors call"
+
+# Check after all the new inferiors have been created that we still
+# only have a single connection.
+gdb_test "info connections" \
+    "\\*\\s+1\\s+\\s+core\\s+Local core dump file\\s*"
+
+# Now switch to inferior 2 and load the second executable and core
+# file.  Check the backtrace for the presence of function 'foo', this
+# indicates we are seeing the correct core file.
+gdb_test "inferior 2" "Switching to inferior 2 .*"
+gdb_test "file $binfile2" \
+    "Reading symbols from .*" \
+    "Loaded second test binary"
+gdb_test "core-file $corefile2" \
+    "Program terminated with signal SIGABRT, Aborted.*" \
+    "Loaded second core file"
+gdb_test "bt" "foo \\(\\) at .*" \
+    "check backtrace in inferior 2"
+
+# Switch to inferior 3, this one was cloned from inferior 1, so is
+# already debugging the first binary file.  Check its backtrace for
+# 'bar', which indicates we are debugging the correct core file.
+gdb_test "inferior 3" "Switching to inferior 3 .*"
+gdb_test "core-file $corefile1" \
+    "Program terminated with signal SIGABRT, Aborted.*" \
+    "Loaded first core file into inferior 3"
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 3"
+
+# Detach from some of the core files and delete some of the inferiors.
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 3 core file"
+gdb_test "inferior 2" "Switching to inferior 2 .*" \
+    "switch back to inferior 2"
+gdb_test_no_output "remove-inferiors 3 4"
+
+# Now detach in inferior 2, and delete the inferior.
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 2 core file"
+gdb_test "inferior 1" "Switching to inferior 1 .*" \
+    "switch back to inferior 1"
+gdb_test_no_output "remove-inferiors 2"
+
+# Finally, check that inferior 1 backtrace is still working.
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 1 again"
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 1 core file"


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

* [PATCHv3 0/7] gdb: fix target_ops reference count for some cases
  2022-10-02 17:04 ` [PATCHv2 0/7] " Andrew Burgess
                     ` (6 preceding siblings ...)
  2022-10-02 17:04   ` [PATCHv2 7/7] gdb: some process_stratum_target should not be shared Andrew Burgess
@ 2022-11-18 16:42   ` Andrew Burgess
  2022-11-18 16:42     ` [PATCHv3 1/7] gdb/remote: remove some manual reference count handling Andrew Burgess
                       ` (6 more replies)
  7 siblings, 7 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-11-18 16:42 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

Changes in v3:

  - Rebase onto current upstream/master, resolve merge conflicts.
  - Updated the test in patch 7 to fix a failure when using the
    native-extended-gdbserver board.

Changes in v2:

  A complete rewrite (and expansion) of this patch after Simon's
  feedback.
  
  The original idea (from v1) is really covered by patches #3 and #5.
  There's still (maybe) an open question about the use of
  scoped_restore_pspace_and_thread vs scoped_restore_thread, but I hope
  I've addressed this question in the v1 thread.  Do let me know if
  there's still any questions on this though.
  
  Patches #1 and #2 are target_ops ref-count related cleanups that I
  spotted while working on this updated series.
  
  Patch #4 is also a cleanup, moving some global functions to become
  member functions on the inferior class.
  
  Patch #6 is a completely random addition to add some extra maintenance
  output, I ended up using this while debugging patch #7, but this
  change isn't actually required at all.
  
  Patch #7 does relate to target_ops, and target_ops sharing, which is
  kind-of related to reference counting... maybe?  I can easily drop
  this patch from the series if it turns out the idea in here is not
  taking GDB in the right direction.

Thanks,
Andrew

---

Andrew Burgess (7):
  gdb/remote: remove some manual reference count handling
  gdb: remove decref_target
  gdb: have target_stack automate reference count handling
  gdb: remove the pop_all_targets (and friends) global functions
  gdb: ensure all targets are popped before an inferior is destructed
  gdb/maint: add core file name to 'maint info program-spaces' output
  gdb: some process_stratum_target should not be shared

 gdb/NEWS                                      |  11 ++
 gdb/corelow.c                                 |   5 +
 gdb/doc/gdb.texinfo                           |  45 ++++-
 gdb/event-top.c                               |   3 +-
 gdb/inferior.c                                |  70 +++++++
 gdb/inferior.h                                |  26 ++-
 gdb/progspace.c                               |  18 +-
 gdb/remote.c                                  |  40 ++--
 gdb/scoped-mock-context.h                     |   2 +-
 gdb/target.c                                  |  99 ++++------
 gdb/target.h                                  |  33 ++--
 gdb/testsuite/gdb.multi/multi-core-files-1.c  |  37 ++++
 gdb/testsuite/gdb.multi/multi-core-files-2.c  |  31 ++++
 gdb/testsuite/gdb.multi/multi-core-files.exp  | 172 ++++++++++++++++++
 .../gdb.python/py-connection-removed.exp      |  92 ++++++++++
 gdb/top.c                                     |   3 +-
 16 files changed, 578 insertions(+), 109 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-1.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-2.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files.exp
 create mode 100644 gdb/testsuite/gdb.python/py-connection-removed.exp


base-commit: f9f88aede3bb84efd088a59a5f6bccb3a6bb6516
-- 
2.25.4


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

* [PATCHv3 1/7] gdb/remote: remove some manual reference count handling
  2022-11-18 16:42   ` [PATCHv3 0/7] gdb: fix target_ops reference count for some cases Andrew Burgess
@ 2022-11-18 16:42     ` Andrew Burgess
  2022-11-18 16:42     ` [PATCHv3 2/7] gdb: remove decref_target Andrew Burgess
                       ` (5 subsequent siblings)
  6 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-11-18 16:42 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

While working on some other target_ops reference count related code, I
spotted that in remote.c we do some manual reference count handling,
i.e. we call target_ops::incref and decref_target (which wraps
target_ops::decref).

I think it would be better to make use of gdb::ref_ptr to automate the
reference count management.

So, this commit updates scoped_mark_target_starting in two ways,
first, I use gdb::ref_ptr to handle the reference counts.  Then,
instead of using the scoped_mark_target_starting constructor and
destructor to set and reset the starting_up flag, I now use a
scoped_restore_tmpl object to set and restore the flag.

The above changes mean that the scoped_mark_target_starting destructor
can be completely removed, and the constructor body is now empty.

I've also fixed a typo in the class comment.

The only change in behaviour after this commit is that previously we
didn't care what the value of starting_up was, we just set it to true
in the constructor and false in the destructor.

Now, I assert that the flag is initially false, then set the flag true
when the scoped_mark_target_starting is created.

As the starting_up flag is initialized to false then, for the assert
to fire, we would need to recursively enter
remote_target::start_remote_1, which I don't think is something we
should be doing, so I think the new assert is an improvement.
---
 gdb/remote.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 5118ecd0a31..24da3d544d5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4702,7 +4702,7 @@ remote_target::process_initial_stop_replies (int from_tty)
     }
 }
 
-/* Mark a remote_target as marking (by setting the starting_up flag within
+/* Mark a remote_target as starting (by setting the starting_up flag within
    its remote_state) for the lifetime of this object.  The reference count
    on the remote target is temporarily incremented, to prevent the target
    being deleted under our feet.  */
@@ -4712,26 +4712,32 @@ struct scoped_mark_target_starting
   /* Constructor, TARGET is the target to be marked as starting, its
      reference count will be incremented.  */
   scoped_mark_target_starting (remote_target *target)
-    : m_remote_target (target)
-  {
-    m_remote_target->incref ();
-    remote_state *rs = m_remote_target->get_remote_state ();
-    rs->starting_up = true;
-  }
+    : m_remote_target (remote_target_ref::new_reference (target)),
+      m_restore_starting_up (set_starting_up_flag (target))
+  { /* Nothing.  */ }
+
+private:
 
-  /* Destructor, mark the target being worked on as no longer starting, and
-     decrement the reference count.  */
-  ~scoped_mark_target_starting ()
+  /* Helper function, set the starting_up flag on TARGET and return an
+     object which, when it goes out of scope, will restore the previous
+     value of the starting_up flag.  */
+  static scoped_restore_tmpl<bool>
+  set_starting_up_flag (remote_target *target)
   {
-    remote_state *rs = m_remote_target->get_remote_state ();
-    rs->starting_up = false;
-    decref_target (m_remote_target);
+    remote_state *rs = target->get_remote_state ();
+    gdb_assert (!rs->starting_up);
+    return make_scoped_restore (&rs->starting_up, true);
   }
 
-private:
+  /* A gdb::ref_ptr pointer to a remote_target.  */
+  using remote_target_ref = gdb::ref_ptr<remote_target, target_ops_ref_policy>;
+
+  /* A reference to the target on which we are operating.  */
+  remote_target_ref m_remote_target;
 
-  /* The target on which we are operating.  */
-  remote_target *m_remote_target;
+  /* An object which restores the previous value of the starting_up flag
+     when it goes out of scope.  */
+  scoped_restore_tmpl<bool> m_restore_starting_up;
 };
 
 /* Helper for remote_target::start_remote, start the remote connection and
-- 
2.25.4


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

* [PATCHv3 2/7] gdb: remove decref_target
  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     ` 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
                       ` (4 subsequent siblings)
  6 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-11-18 16:42 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

The decref_target function is not really needed.  Calling
target_ops::decref will just redirect to decref_target anyway, so why
not just rename decref_target to target_ops::decref?

That's what this commit does.

It's not exactly renaming to target_ops::decref, because the decref
functionality is handled by a policy class, so the new name is now
target_ops_ref_policy::decref.

There should be no user visible change after this commit.
---
 gdb/target.c |  2 +-
 gdb/target.h | 10 +++-------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 74925e139dc..d7f742c83ec 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1157,7 +1157,7 @@ to_execution_direction must be implemented for reverse async");
 /* See target.h.  */
 
 void
-decref_target (target_ops *t)
+target_ops_ref_policy::decref (target_ops *t)
 {
   t->decref ();
   if (t->refcount () == 0)
diff --git a/gdb/target.h b/gdb/target.h
index 28aa9273893..0f5038fb9b2 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1337,9 +1337,6 @@ struct target_ops_deleter
 /* A unique pointer for target_ops.  */
 typedef std::unique_ptr<target_ops, target_ops_deleter> target_ops_up;
 
-/* Decref a target and close if, if there are no references left.  */
-extern void decref_target (target_ops *t);
-
 /* A policy class to interface gdb::ref_ptr with target_ops.  */
 
 struct target_ops_ref_policy
@@ -1349,10 +1346,9 @@ struct target_ops_ref_policy
     t->incref ();
   }
 
-  static void decref (target_ops *t)
-  {
-    decref_target (t);
-  }
+  /* Decrement the reference count on T, and, if the reference count
+     reaches zero, close the target.  */
+  static void decref (target_ops *t);
 };
 
 /* A gdb::ref_ptr pointer to a target_ops.  */
-- 
2.25.4


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

* [PATCHv3 3/7] gdb: have target_stack automate reference count handling
  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 16:42     ` 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
                       ` (3 subsequent siblings)
  6 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-11-18 16:42 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

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.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.

To ensure that we have one gdb.TargtetConnection object for each
connection, the all_connection_objects map exists, this maps the
process_stratum_target object (the connection) to the
gdb.TargtetConnection 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.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 still 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.

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 d7f742c83ec..75bc6c680d8 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1173,23 +1173,28 @@ target_ops_ref_policy::decref (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.  */
@@ -1214,19 +1219,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;
 }
@@ -3598,8 +3603,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 0f5038fb9b2..68446a39c1b 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1385,7 +1385,7 @@ class target_stack
   { 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); }
@@ -1400,7 +1400,7 @@ class target_stack
   /* 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
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 <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"
+}
-- 
2.25.4


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

* [PATCHv3 4/7] gdb: remove the pop_all_targets (and friends) global functions
  2022-11-18 16:42   ` [PATCHv3 0/7] gdb: fix target_ops reference count for some cases Andrew Burgess
                       ` (2 preceding siblings ...)
  2022-11-18 16:42     ` [PATCHv3 3/7] gdb: have target_stack automate reference count handling Andrew Burgess
@ 2022-11-18 16:42     ` 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
                       ` (2 subsequent siblings)
  6 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-11-18 16:42 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

This commit removes the global functions pop_all_targets,
pop_all_targets_above, and pop_all_targets_at_and_above, and makes
them methods on the inferior class.

As the pop_all_targets functions will unpush each target, which
decrements the targets reference count, it is possible that the target
might be closed.

Right now, closing a target, in some cases, depends on the current
inferior being set correctly, that is, to the inferior from which the
target was popped.

To facilitate this I have used switch_to_inferior_no_thread within the
new methods.  Previously it was the responsibility of the caller to
ensure that the correct inferior was selected.

In a couple of places (event-top.c and top.c) I have been able to
remove a previous switch_to_inferior_no_thread call.

In remote_unpush_target (remote.c) I have left the
switch_to_inferior_no_thread call as it is required for the
generic_mourn_inferior call.
---
 gdb/event-top.c           |  3 +--
 gdb/inferior.c            | 39 ++++++++++++++++++++++++++++++++++++++
 gdb/inferior.h            | 20 ++++++++++++++++++++
 gdb/remote.c              |  2 +-
 gdb/scoped-mock-context.h |  2 +-
 gdb/target.c              | 40 +--------------------------------------
 gdb/target.h              | 11 -----------
 gdb/top.c                 |  3 +--
 8 files changed, 64 insertions(+), 56 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 0e371194ee3..d858104c1e8 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1293,10 +1293,9 @@ async_disconnect (gdb_client_data arg)
 
   for (inferior *inf : all_inferiors ())
     {
-      switch_to_inferior_no_thread (inf);
       try
 	{
-	  pop_all_targets ();
+	  inf->pop_all_targets ();
 	}
       catch (const gdb_exception &exception)
 	{
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 7eb2bd97907..4bad2198af7 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -103,6 +103,45 @@ inferior::unpush_target (struct target_ops *t)
   return m_target_stack.unpush (t);
 }
 
+/* See inferior.h.  */
+
+void inferior::unpush_target_and_assert (struct target_ops *target)
+{
+  gdb_assert (current_inferior () == this);
+
+  if (!unpush_target (target))
+    internal_error ("pop_all_targets couldn't find target %s\n",
+		    target->shortname ());
+}
+
+/* See inferior.h.  */
+
+void inferior::pop_all_targets_above (enum strata stratum)
+{
+  /* Unpushing a target might cause it to close.  Some targets currently
+     rely on the current_inferior being set for their ::close method, so we
+     temporarily switch inferior now.  */
+  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
+  switch_to_inferior_no_thread (this);
+
+  while (top_target ()->stratum () > stratum)
+    unpush_target_and_assert (top_target ());
+}
+
+/* See inferior.h.  */
+
+void inferior::pop_all_targets_at_and_above (enum strata stratum)
+{
+  /* Unpushing a target might cause it to close.  Some targets currently
+     rely on the current_inferior being set for their ::close method, so we
+     temporarily switch inferior now.  */
+  scoped_restore_current_pspace_and_thread restore_pspace_and_thread;
+  switch_to_inferior_no_thread (this);
+
+  while (top_target ()->stratum () >= stratum)
+    unpush_target_and_assert (top_target ());
+}
+
 void
 inferior::set_tty (std::string terminal_name)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 69525a2e053..2917f42e624 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -377,6 +377,22 @@ class inferior : public refcounted_object,
   target_ops *top_target ()
   { return m_target_stack.top (); }
 
+  /* Unpush all targets except the dummy target from m_target_stack.  As
+     targets are removed from m_target_stack their reference count is
+     decremented, which may cause a target to close.  */
+  void pop_all_targets ()
+  { pop_all_targets_above (dummy_stratum); }
+
+  /* Unpush all targets above STRATUM from m_target_stack.  As targets are
+     removed from m_target_stack their reference count is decremented,
+     which may cause a target to close.  */
+  void pop_all_targets_above (enum strata stratum);
+
+  /* Unpush all targets at and above STRATUM from m_target_stack.  As
+     targets are removed from m_target_stack their reference count is
+     decremented, which may cause a target to close.  */
+  void pop_all_targets_at_and_above (enum strata stratum);
+
   /* Return the target at process_stratum level in this inferior's
      target stack.  */
   struct process_stratum_target *process_target ()
@@ -595,6 +611,10 @@ class inferior : public refcounted_object,
   registry<inferior> registry_fields;
 
 private:
+
+  /* Unpush TARGET and assert that it worked.  */
+  void unpush_target_and_assert (struct target_ops *target);
+
   /* The inferior's target stack.  */
   target_stack m_target_stack;
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 24da3d544d5..4db5fd18b14 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5717,7 +5717,7 @@ remote_unpush_target (remote_target *target)
   for (inferior *inf : all_inferiors (target))
     {
       switch_to_inferior_no_thread (inf);
-      pop_all_targets_at_and_above (process_stratum);
+      inf->pop_all_targets_at_and_above (process_stratum);
       generic_mourn_inferior ();
     }
 
diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h
index a9895303015..87c1df0d206 100644
--- a/gdb/scoped-mock-context.h
+++ b/gdb/scoped-mock-context.h
@@ -71,7 +71,7 @@ struct scoped_mock_context
   ~scoped_mock_context ()
   {
     inferior_list.erase (inferior_list.iterator_to (mock_inferior));
-    pop_all_targets_at_and_above (process_stratum);
+    mock_inferior.pop_all_targets_at_and_above (process_stratum);
   }
 };
 
diff --git a/gdb/target.c b/gdb/target.c
index 75bc6c680d8..b2af51551ba 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1236,44 +1236,6 @@ target_stack::unpush (target_ops *t)
   return true;
 }
 
-/* Unpush TARGET and assert that it worked.  */
-
-static void
-unpush_target_and_assert (struct target_ops *target)
-{
-  if (!current_inferior ()->unpush_target (target))
-    {
-      gdb_printf (gdb_stderr,
-		  "pop_all_targets couldn't find target %s\n",
-		  target->shortname ());
-      internal_error (_("failed internal consistency check"));
-    }
-}
-
-void
-pop_all_targets_above (enum strata above_stratum)
-{
-  while ((int) (current_inferior ()->top_target ()->stratum ())
-	 > (int) above_stratum)
-    unpush_target_and_assert (current_inferior ()->top_target ());
-}
-
-/* See target.h.  */
-
-void
-pop_all_targets_at_and_above (enum strata stratum)
-{
-  while ((int) (current_inferior ()->top_target ()->stratum ())
-	 >= (int) stratum)
-    unpush_target_and_assert (current_inferior ()->top_target ());
-}
-
-void
-pop_all_targets (void)
-{
-  pop_all_targets_above (dummy_stratum);
-}
-
 void
 target_unpusher::operator() (struct target_ops *ops) const
 {
@@ -2530,7 +2492,7 @@ target_preopen (int from_tty)
      it doesn't (which seems like a win for UDI), remove it now.  */
   /* Leave the exec target, though.  The user may be switching from a
      live process to a core of the same program.  */
-  pop_all_targets_above (file_stratum);
+  current_inferior ()->pop_all_targets_above (file_stratum);
 
   target_pre_inferior (from_tty);
 }
diff --git a/gdb/target.h b/gdb/target.h
index 68446a39c1b..547ee8a3bbd 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2389,17 +2389,6 @@ extern void target_pre_inferior (int);
 
 extern void target_preopen (int);
 
-/* Does whatever cleanup is required to get rid of all pushed targets.  */
-extern void pop_all_targets (void);
-
-/* Like pop_all_targets, but pops only targets whose stratum is at or
-   above STRATUM.  */
-extern void pop_all_targets_at_and_above (enum strata stratum);
-
-/* Like pop_all_targets, but pops only targets whose stratum is
-   strictly above ABOVE_STRATUM.  */
-extern void pop_all_targets_above (enum strata above_stratum);
-
 extern CORE_ADDR target_translate_tls_address (struct objfile *objfile,
 					       CORE_ADDR offset);
 
diff --git a/gdb/top.c b/gdb/top.c
index e9794184f07..f5fc7276ac7 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1845,10 +1845,9 @@ quit_force (int *exit_arg, int from_tty)
      them all out.  */
   for (inferior *inf : all_inferiors ())
     {
-      switch_to_inferior_no_thread (inf);
       try
 	{
-	  pop_all_targets ();
+	  inf->pop_all_targets ();
 	}
       catch (const gdb_exception &ex)
 	{
-- 
2.25.4


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

* [PATCHv3 5/7] gdb: ensure all targets are popped before an inferior is destructed
  2022-11-18 16:42   ` [PATCHv3 0/7] gdb: fix target_ops reference count for some cases Andrew Burgess
                       ` (3 preceding siblings ...)
  2022-11-18 16:42     ` [PATCHv3 4/7] gdb: remove the pop_all_targets (and friends) global functions Andrew Burgess
@ 2022-11-18 16:42     ` 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 16:42     ` [PATCHv3 7/7] gdb: some process_stratum_target should not be shared Andrew Burgess
  6 siblings, 0 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-11-18 16:42 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

Now that the inferiors target_stack automatically manages target
reference counts, we might think that we don't need to unpush targets
when an inferior is deleted...

...unfortunately that is not the case.  The inferior::unpush function
can do some work depending on the type of target, so it is important
that we still pass through this function.

To ensure that this is the case, in this commit I've added an assert
to inferior::~inferior that ensures the inferior's target_stack is
empty (except for the ever present dummy_target).

I've then added a pop_all_targets call to delete_inferior, otherwise
the new assert will fire in, e.g. the gdb.python/py-inferior.exp test.
---
 gdb/inferior.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 4bad2198af7..f4b9828a425 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -70,6 +70,15 @@ 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.  */
+  gdb_assert (m_target_stack.top ()->stratum () == dummy_stratum);
+
   m_continuations.clear ();
   target_desc_info_free (inf->tdesc_info);
 }
@@ -230,6 +239,12 @@ delete_inferior (struct inferior *inf)
 
   gdb::observers::inferior_removed.notify (inf);
 
+  /* Pop all targets now, this ensures that inferior::unpush is called
+     correctly.  As pop_all_targets ends up making a temporary switch to
+     inferior INF then we need to make this call before we delete the
+     program space, which we do below.  */
+  inf->pop_all_targets ();
+
   /* If this program space is rendered useless, remove it. */
   if (inf->pspace->empty ())
     delete inf->pspace;
-- 
2.25.4


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

* [PATCHv3 6/7] gdb/maint: add core file name to 'maint info program-spaces' output
  2022-11-18 16:42   ` [PATCHv3 0/7] gdb: fix target_ops reference count for some cases Andrew Burgess
                       ` (4 preceding siblings ...)
  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     ` 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
  6 siblings, 1 reply; 36+ messages in thread
From: Andrew Burgess @ 2022-11-18 16:42 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

Each program space can have an associated core file.  Include this
information in the output of 'maint info program-spaces'.
---
 gdb/NEWS            |  4 ++++
 gdb/doc/gdb.texinfo |  8 ++++++--
 gdb/progspace.c     | 18 ++++++++++++++++--
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 424e5ffd829..8176940ae1a 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -146,6 +146,10 @@ maintenance info line-table
   entry corresponds to an address where a breakpoint should be placed
   to be at the first instruction past a function's prologue.
 
+maintenance info program-spaces
+  This command now includes a 'Core File' column which indicates the
+  name of the core file associated with each program space.
+
 * New targets
 
 GNU/Linux/LoongArch (gdbserver)	loongarch*-*-linux*
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index bb7b2764408..566da9e7d92 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3465,6 +3465,10 @@
 the name of the executable loaded into the program space, with e.g.,
 the @code{file} command.
 
+@item
+the name of the core file loaded into the program space, with e.g.,
+the @code{core-file} command.
+
 @end enumerate
 
 @noindent
@@ -3477,7 +3481,7 @@
 
 @smallexample
 (@value{GDBP}) maint info program-spaces
-  Id   Executable
+  Id   Executable        Core File
 * 1    hello
   2    goodbye
         Bound inferiors: ID 1 (process 21561)
@@ -3491,7 +3495,7 @@
 
 @smallexample
 (@value{GDBP}) maint info program-spaces
-  Id   Executable
+  Id   Executable        Core File
 * 1    vfork-test
         Bound inferiors: ID 2 (process 18050), ID 1 (process 18045)
 @end smallexample
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 90003d964fe..4f58d44a0e6 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -251,22 +251,30 @@ print_program_space (struct ui_out *uiout, int requested)
 {
   int count = 0;
 
+  /* Start with a minimum width of 17 for the executable name column.  */
+  size_t longest_exec_name = 17;
+
   /* Compute number of pspaces we will print.  */
   for (struct program_space *pspace : program_spaces)
     {
       if (requested != -1 && pspace->num != requested)
 	continue;
 
+      if (pspace->exec_filename != nullptr)
+	longest_exec_name = std::max (strlen (pspace->exec_filename.get ()),
+				      longest_exec_name);
+
       ++count;
     }
 
   /* There should always be at least one.  */
   gdb_assert (count > 0);
 
-  ui_out_emit_table table_emitter (uiout, 3, count, "pspaces");
+  ui_out_emit_table table_emitter (uiout, 4, count, "pspaces");
   uiout->table_header (1, ui_left, "current", "");
   uiout->table_header (4, ui_left, "id", "Id");
-  uiout->table_header (17, ui_left, "exec", "Executable");
+  uiout->table_header (longest_exec_name, ui_left, "exec", "Executable");
+  uiout->table_header (17, ui_left, "core", "Core File");
   uiout->table_body ();
 
   for (struct program_space *pspace : program_spaces)
@@ -291,6 +299,12 @@ print_program_space (struct ui_out *uiout, int requested)
       else
 	uiout->field_skip ("exec");
 
+      if (pspace->cbfd != nullptr)
+	uiout->field_string ("core", bfd_get_filename (pspace->cbfd.get ()),
+			     file_name_style.style ());
+      else
+	uiout->field_skip ("core");
+
       /* Print extra info that doesn't really fit in tabular form.
 	 Currently, we print the list of inferiors bound to a pspace.
 	 There can be more than one inferior bound to the same pspace,
-- 
2.25.4


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

* [PATCHv3 7/7] gdb: some process_stratum_target should not be shared
  2022-11-18 16:42   ` [PATCHv3 0/7] gdb: fix target_ops reference count for some cases Andrew Burgess
                       ` (5 preceding siblings ...)
  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 16:42     ` Andrew Burgess
  2022-11-18 17:02       ` Eli Zaretskii
  2022-11-18 18:04       ` Tom Tromey
  6 siblings, 2 replies; 36+ messages in thread
From: Andrew Burgess @ 2022-11-18 16:42 UTC (permalink / raw)
  To: gdb-patches; +Cc: Andrew Burgess

When multi-target support was added to GDB, an assumption was made
that all process_stratum_target sub-classes could be shared by
multiple inferiors.

For things like the Linux and FreeBSD native targets, as well as the
remote target, this is absolutely true (or was made true).  But some
targets were never updated to be shareable, for example, the
core_target, which is used when reading core-files, stores some of its
state in the program_space, but also, the core-file and the executable
being debugged are closely related.

As each add-inferior call creates an inferior with a new
program_space, and doesn't automatically copy the executable, or the
current core-file, I don't think it really makes sense to "share"
core_target objects between inferiors.

Consider this session:

  $ gdb -q
  (gdb) file test1
  Reading symbols from test1...
  (gdb) core-file core.test1.433190
  [New LWP 433190]
  Core was generated by `./test1'.
  Program terminated with signal SIGSEGV, Segmentation fault.
  #0  0x0000000000401111 in foo () at test1.c:6
  6	  return *global_ptr;
  (gdb) add-inferior
  [New inferior 2]
  Added inferior 2 on connection 1 (core)
  (gdb) info inferiors
    Num  Description       Connection           Executable
  * 1    process 433190    1 (core)             /tmp/multi-core/test1
    2    <null>            1 (core)
  (gdb) info connections
    Num  What  Description
  * 1    core  Local core dump file
  (gdb) inferior 2
  [Switching to inferior 2 [<null>] (<noexec>)]
  (gdb) file test2
  Reading symbols from test2...
  (gdb) core-file core.test2.433203
  [New LWP 433203]
  Core was generated by `./test2'.
  Program terminated with signal SIGSEGV, Segmentation fault.
  #0  0x0000000000401111 in main () at test2.c:6
  6	  return *global_ptr;
  (gdb) info inferiors
    Num  Description       Connection           Executable
    1    process 433190    1 (core)             /tmp/multi-core/test1
  * 2    process 433203    2 (core)             /tmp/multi-core/test2
  (gdb) info connections
    Num  What  Description
    1    core  Local core dump file
  * 2    core  Local core dump file
  (gdb)

After the 'add-inferior' the core_target connection is shared between
the inferiors.  However, as soon as the user sets up the core-file and
executable in the new inferior a new core connection has been created.

I think this behaviour might be confusing, so I'd like to have GDB not
initially share the core connection.  Instead, when the user tries to
add the new inferior a warning is given, and the new inferior is
created without a connection, like this:

  $ gdb -q
  (gdb) file test1
  Reading symbols from test1...
  (gdb) core-file core.test1.433190
  [New LWP 433190]
  Core was generated by `./test1'.
  Program terminated with signal SIGSEGV, Segmentation fault.
  #0  0x0000000000401111 in foo () at test1.c:6
  6	  return *global_ptr;
  (gdb) add-inferior
  [New inferior 2]
  warning: can't share connection 1 (core) between inferiors
  Added inferior 2
  (gdb) info inferiors
    Num  Description       Connection           Executable
  * 1    process 433190    1 (core)             /tmp/multi-core/test1
    2    <null>
  (gdb)

If the user explicitly asks for the new inferior to be created without
a connection, then no warning will be given.

At this point the user is free to setup inferior 2 with a different
executable and core file (or to do anything they like with the
inferior).

In an earlier version of this patch I had GDB error instead of giving
a warning.  However, the error message ended up being something like:

  can't share connection ..... between inferiors, use -no-connection
      option to create an inferior without sharing a connection.

but it seemed better to just create the inferior.

I've updated the docs, and added a NEWS entry for the new warning.  In
the docs for clone-inferior I've added reference to -no-connection,
which was previously missing.
---
 gdb/NEWS                                     |   7 +
 gdb/corelow.c                                |   5 +
 gdb/doc/gdb.texinfo                          |  37 +++-
 gdb/inferior.c                               |  16 ++
 gdb/inferior.h                               |   6 +-
 gdb/target.c                                 |  14 ++
 gdb/target.h                                 |   8 +
 gdb/testsuite/gdb.multi/multi-core-files-1.c |  37 ++++
 gdb/testsuite/gdb.multi/multi-core-files-2.c |  31 ++++
 gdb/testsuite/gdb.multi/multi-core-files.exp | 172 +++++++++++++++++++
 10 files changed, 329 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-1.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-2.c
 create mode 100644 gdb/testsuite/gdb.multi/multi-core-files.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 8176940ae1a..5c8dac12de7 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -81,6 +81,13 @@
 * New convenience variable $_inferior_thread_count contains the number
   of live threads in the current inferior.
 
+* The add-inferior, clone-inferior, and MI -add-inferior commands will
+  now give a warning, and create the new inferior without a
+  connection, when the current inferior, at the time the command is
+  given, is a core-file target.  The core-file target could never
+  really be shared between inferiors, GDB is now more vocal about what
+  is going on.
+
 * New commands
 
 maintenance set ignore-prologue-end-flag on|off
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 293bc8d4f59..4d8393a3587 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -128,6 +128,11 @@ class core_target final : public process_stratum_target
   /* See definition.  */
   void info_proc_mappings (struct gdbarch *gdbarch);
 
+  /* The core_target only works for the inferior in which it was initially
+     opened, and can't be copied to some other inferior's target_stack.  */
+  bool is_shareable () override
+  { return false; }
+
 private: /* per-core data */
 
   /* Get rid of the core inferior.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 566da9e7d92..bb33e60a22e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3358,8 +3358,31 @@
 remote} command to connect to some other @code{gdbserver} instance,
 use @code{run} to spawn a local program, etc.
 
+Not all connections can be shared between inferiors.  For example, the
+@code{target core} target is unique for each inferior.  That is,
+multiple inferiors can use @code{target core} at the same time, but
+each @code{target core} is different.  If you try to
+@code{add-inferior}, and the current inferior is @code{target core},
+then @value{GDBN} will give a warning and create the new inferior
+without a connection, like this:
+
+@smallexample
+(@value{GDBP}) file test1
+Reading symbols from test1...
+(@value{GDBP}) target core core.test1.433190
+[New LWP 433190]
+Core was generated by `./test1'.
+Program terminated with signal SIGSEGV, Segmentation fault.
+#0  0x0000000000401111 in foo () at test1.c:6
+6	  return *global_ptr;
+(@value{GDBP}) add-inferior
+[New inferior 2]
+warning: can't share connection 1 (core) between inferiors
+Added inferior 2
+@end smallexample
+
 @kindex clone-inferior
-@item clone-inferior [ -copies @var{n} ] [ @var{infno} ]
+@item clone-inferior [ -copies @var{n} ] [ -no-connection ] [ @var{infno} ]
 Adds @var{n} inferiors ready to execute the same program as inferior
 @var{infno}; @var{n} defaults to 1, and @var{infno} defaults to the
 number of the current inferior.  This command copies the values of the
@@ -3384,6 +3407,13 @@
 
 You can now simply switch focus to inferior 2 and run it.
 
+Like @code{add-inferior}, @code{clone-inferior} shares the connection
+with the inferior @var{infno}.  If the @var{-no-connection} option is
+given, then the new inferior will be created without a connection.  If
+the connection of inferior @var{infno} can't be shared, then
+@value{GDBN} will give a warning, and the new inferior will be created
+without a connection.
+
 @kindex remove-inferiors
 @item remove-inferiors @var{infno}@dots{}
 Removes the inferior or inferiors @var{infno}@dots{}.  It is not
@@ -37828,6 +37858,11 @@
 @code{gdbserver} instance, use @code{-exec-run} to spawn a local
 program, etc.
 
+If the connection of the current inferior cannot be shared, e.g.@: the
+@code{-target-select core} target cannot be shared between inferiors,
+then @value{GDBN} will give a warning and create the new inferior
+without a connection.
+
 The command response always has a field, @var{inferior}, whose value
 is the identifier of the thread group corresponding to the new
 inferior.
diff --git a/gdb/inferior.c b/gdb/inferior.c
index f4b9828a425..ec4c348f7e3 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -816,6 +816,22 @@ switch_to_inferior_and_push_target (inferior *new_inf,
      symbols.  */
   switch_to_inferior_no_thread (new_inf);
 
+  if (!no_connection && proc_target != nullptr
+      && !proc_target->is_shareable ())
+    {
+      if (proc_target->connection_string () != nullptr)
+	warning (_("can't share connection %d (%s %s) between inferiors"),
+		 proc_target->connection_number,
+		 proc_target->shortname (),
+		 proc_target->connection_string ());
+      else
+	warning (_("can't share connection %d (%s) between inferiors"),
+		 proc_target->connection_number,
+		 proc_target->shortname ());
+
+      proc_target = nullptr;
+    }
+
   /* Reuse the target for new inferior.  */
   if (!no_connection && proc_target != NULL)
     {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 2917f42e624..a312cd2b9bb 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -759,9 +759,9 @@ extern struct inferior *add_inferior_with_spaces (void);
 /* Print the current selected inferior.  */
 extern void print_selected_inferior (struct ui_out *uiout);
 
-/* Switch to inferior NEW_INF, a new inferior, and unless
-   NO_CONNECTION is true, push the process_stratum_target of ORG_INF
-   to NEW_INF.  */
+/* Switch to inferior NEW_INF, a new inferior, and unless NO_CONNECTION is
+   true, or the process_stratum_target of ORG_INF is not shareable, push
+   the process_stratum_target of ORG_INF to NEW_INF.  */
 
 extern void switch_to_inferior_and_push_target
   (inferior *new_inf, bool no_connection, inferior *org_inf);
diff --git a/gdb/target.c b/gdb/target.c
index b2af51551ba..ac0608645b4 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1187,6 +1187,12 @@ target_stack::push (target_ops *t)
   if (m_stack[stratum].get () != nullptr)
     unpush (m_stack[stratum].get ());
 
+  /* If this target can't be shared, then check that the target doesn't
+     already appear on some other target stack.  */
+  if (!t->is_shareable ())
+    for (inferior *inf : all_inferiors ())
+      gdb_assert (!inf->target_is_pushed (t));
+
   /* Now add the new one.  */
   m_stack[stratum] = std::move (ref);
 
@@ -3221,6 +3227,14 @@ target_ops::fileio_readlink (struct inferior *inf, const char *filename,
 
 /* See target.h.  */
 
+bool
+target_ops::is_shareable ()
+{
+  return true;
+}
+
+/* See target.h.  */
+
 int
 target_fileio_open (struct inferior *inf, const char *filename,
 		    int flags, int mode, bool warn_if_slow, fileio_error *target_errno)
diff --git a/gdb/target.h b/gdb/target.h
index 547ee8a3bbd..30e5085a543 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1321,6 +1321,14 @@ struct target_ops
     virtual bool store_memtags (CORE_ADDR address, size_t len,
 				const gdb::byte_vector &tags, int type)
       TARGET_DEFAULT_NORETURN (tcomplain ());
+
+    /* Return true if this target can be shared on multiple target_stacks,
+       or false if this target should only appear on a single target_stack.
+       When this function returns false multiple separate instances of the
+       same target_ops sub-class can still appear on different
+       target_stacks, but the same concrete instance can only appear on a
+       single target_stack.  */
+    virtual bool is_shareable ();
   };
 
 /* Deleter for std::unique_ptr.  See comments in
diff --git a/gdb/testsuite/gdb.multi/multi-core-files-1.c b/gdb/testsuite/gdb.multi/multi-core-files-1.c
new file mode 100644
index 00000000000..f996973023e
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files-1.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <stdlib.h>
+
+int
+bar ()
+{
+  abort ();
+  return 0;
+}
+
+int
+baz ()
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  return baz ();
+}
diff --git a/gdb/testsuite/gdb.multi/multi-core-files-2.c b/gdb/testsuite/gdb.multi/multi-core-files-2.c
new file mode 100644
index 00000000000..fb99e137c3f
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files-2.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <stdlib.h>
+
+int
+foo ()
+{
+  abort ();
+  return 0;
+}
+
+int
+main ()
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.multi/multi-core-files.exp b/gdb/testsuite/gdb.multi/multi-core-files.exp
new file mode 100644
index 00000000000..8c2610000a0
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/multi-core-files.exp
@@ -0,0 +1,172 @@
+# Copyright 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/>.
+
+# This script runs some basic tests that GDB can support multiple
+# inferiors each debugging different core files.
+#
+# We also check the behaviour of GDB if the user attempts to clone or
+# duplicate an inferior that is debugging a core file.
+
+standard_testfile -1.c -2.c
+
+set binfile1 "${binfile}-1"
+set binfile2 "${binfile}-2"
+
+if {[build_executable "build first executable" $binfile1 $srcfile \
+	 debug] == -1} {
+    untested "failed to compile first executable"
+    return -1
+}
+
+if {[build_executable "build second executable" $binfile2 $srcfile2 \
+	 debug] == -1} {
+    untested "failed to compile second executable"
+    return -1
+}
+
+set corefile1 [core_find $binfile1]
+set corefile2 [core_find $binfile2]
+if { $corefile1 == "" || $corefile2 == "" } {
+    untested "Can't generate core files"
+    return
+}
+
+# Start GDB, and load the first executable and corefile into the first
+# inferior.
+clean_restart ${binfile1}
+gdb_test "core-file $corefile1" "Program terminated with .*" \
+    "load core file"
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 1"
+
+# The native-extended-remote board connects to the remote target as
+# soon as GDB is started, this means that connection 1 is to the
+# remote target, and the core target we create below will be
+# connection 2.
+#
+# In all other cases, the core target gets to be connection 1.
+if { [target_info gdb_protocol] == "extended-remote"} {
+    set conn_num 2
+} else {
+    set conn_num 1
+}
+
+# Try to use add-inferior and clone-inferior to create new
+# inferiors.  In both cases this will try to share the core_target
+# between inferior 1 and the new inferior.  As the core_target can't
+# be shared we should get a warning, and the inferior should be
+# created without a connection.
+gdb_test "add-inferior" \
+    [multi_line \
+	 "\\\[New inferior 2\\\]" \
+	 "warning: can't share connection ${conn_num} \\(core\\) between inferiors" \
+	 "Added inferior 2"]
+gdb_test "clone-inferior" \
+    [multi_line \
+	 "\\\[New inferior 3\\\]" \
+	 "warning: can't share connection ${conn_num} \\(core\\) between inferiors" \
+	 "Added inferior 3"]
+
+# Check the MI -add-inferior command.  Do this using interpreter-exec.
+# We're not doing a full MI test here, just checking this one command.
+gdb_test "interpreter-exec mi \"-add-inferior\"" \
+    [multi_line \
+	 "~\"\\\[New inferior 4\\\]..\"" \
+	 "&\"warning: can't share connection ${conn_num} \\(core\\) between inferiors..\"" \
+	 "~\"Added inferior 4..\"" \
+	 "\\^done,inferior=\"\[^\"\]+\""]
+
+# Now check that none of the new inferiors have a connection.
+gdb_test "info inferiors" \
+    [multi_line \
+	 "\\*\\s+1\\s+\[^\r\n\]+\\s+${conn_num} \\(core\\)\\s+\[^\r\n\]+.*" \
+	 "\\s+2\\s+<null>\\s+" \
+	 "\\s+3\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+4\\s+<null>\\s+"] \
+    "first info inferiors call"
+
+# Now use add-inferior and clone-inferior but this time with the
+# -no-connection option, this should avoid issuing the warning.  We
+# also use interpreter-exec to test the MI version of this command.
+gdb_test "add-inferior -no-connection" \
+    [multi_line \
+	 "\\\[New inferior 5\\\]" \
+	 "Added inferior 5"]
+gdb_test "clone-inferior -no-connection" \
+    [multi_line \
+	 "\\\[New inferior 6\\\]" \
+	 "Added inferior 6"]
+gdb_test "interpreter-exec mi \"-add-inferior --no-connection\"" \
+    "\\\[New inferior 7\\\].*Added inferior 7.*"
+
+# Now check that none of the new inferiors have a connection.
+gdb_test "info inferiors" \
+    [multi_line \
+	 "\\*\\s+1\\s+\[^\r\n\]+\\s+${conn_num} \\(core\\)\\s+\[^\r\n\]+.*" \
+	 "\\s+2\\s+<null>\\s+" \
+	 "\\s+3\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+4\\s+<null>\\s+" \
+	 "\\s+5\\s+<null>\\s+" \
+	 "\\s+6\\s+<null>\\s+\[^\r\n\]+" \
+	 "\\s+7\\s+<null>\\s+"] \
+    "second info inferiors call"
+
+# Check after all the new inferiors have been created that we still
+# only have a single connection.
+gdb_test "info connections" \
+    "\\*\\s+${conn_num}\\s+\\s+core\\s+Local core dump file\\s*"
+
+# Now switch to inferior 2 and load the second executable and core
+# file.  Check the backtrace for the presence of function 'foo', this
+# indicates we are seeing the correct core file.
+gdb_test "inferior 2" "Switching to inferior 2 .*"
+gdb_test "file $binfile2" \
+    "Reading symbols from .*" \
+    "Loaded second test binary"
+gdb_test "core-file $corefile2" \
+    "Program terminated with signal SIGABRT, Aborted.*" \
+    "Loaded second core file"
+gdb_test "bt" "foo \\(\\) at .*" \
+    "check backtrace in inferior 2"
+
+# Switch to inferior 3, this one was cloned from inferior 1, so is
+# already debugging the first binary file.  Check its backtrace for
+# 'bar', which indicates we are debugging the correct core file.
+gdb_test "inferior 3" "Switching to inferior 3 .*"
+gdb_test "core-file $corefile1" \
+    "Program terminated with signal SIGABRT, Aborted.*" \
+    "Loaded first core file into inferior 3"
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 3"
+
+# Detach from some of the core files and delete some of the inferiors.
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 3 core file"
+gdb_test "inferior 2" "Switching to inferior 2 .*" \
+    "switch back to inferior 2"
+gdb_test_no_output "remove-inferiors 3 4"
+
+# Now detach in inferior 2, and delete the inferior.
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 2 core file"
+gdb_test "inferior 1" "Switching to inferior 1 .*" \
+    "switch back to inferior 1"
+gdb_test_no_output "remove-inferiors 2"
+
+# Finally, check that inferior 1 backtrace is still working.
+gdb_test "bt" "bar \\(\\) at .*" \
+    "check backtrace in inferior 1 again"
+gdb_test "detach" "No core file now\\." \
+    "detach from inferior 1 core file"
-- 
2.25.4


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

* Re: [PATCHv3 7/7] gdb: some process_stratum_target should not be shared
  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
  1 sibling, 0 replies; 36+ messages in thread
From: Eli Zaretskii @ 2022-11-18 17:02 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches, aburgess

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 18 Nov 2022 16:42:58 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> I've updated the docs, and added a NEWS entry for the new warning.  In
> the docs for clone-inferior I've added reference to -no-connection,
> which was previously missing.
> ---
>  gdb/NEWS                                     |   7 +
>  gdb/corelow.c                                |   5 +
>  gdb/doc/gdb.texinfo                          |  37 +++-
>  gdb/inferior.c                               |  16 ++
>  gdb/inferior.h                               |   6 +-
>  gdb/target.c                                 |  14 ++
>  gdb/target.h                                 |   8 +
>  gdb/testsuite/gdb.multi/multi-core-files-1.c |  37 ++++
>  gdb/testsuite/gdb.multi/multi-core-files-2.c |  31 ++++
>  gdb/testsuite/gdb.multi/multi-core-files.exp | 172 +++++++++++++++++++
>  10 files changed, 329 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-1.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files-2.c
>  create mode 100644 gdb/testsuite/gdb.multi/multi-core-files.exp

OK for the documentation parts.

Thanks.

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

* Re: [PATCHv3 6/7] gdb/maint: add core file name to 'maint info program-spaces' output
  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
  0 siblings, 0 replies; 36+ messages in thread
From: Eli Zaretskii @ 2022-11-18 17:03 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches, aburgess

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 18 Nov 2022 16:42:57 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> Each program space can have an associated core file.  Include this
> information in the output of 'maint info program-spaces'.
> ---
>  gdb/NEWS            |  4 ++++
>  gdb/doc/gdb.texinfo |  8 ++++++--
>  gdb/progspace.c     | 18 ++++++++++++++++--
>  3 files changed, 26 insertions(+), 4 deletions(-)

Thanks, the documentation parts are okay.

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

* Re: [PATCHv3 2/7] gdb: remove decref_target
  2022-11-18 16:42     ` [PATCHv3 2/7] gdb: remove decref_target Andrew Burgess
@ 2022-11-18 17:22       ` Tom Tromey
  0 siblings, 0 replies; 36+ messages in thread
From: Tom Tromey @ 2022-11-18 17:22 UTC (permalink / raw)
  To: Andrew Burgess via Gdb-patches; +Cc: Andrew Burgess

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> The decref_target function is not really needed.  Calling
Andrew> target_ops::decref will just redirect to decref_target anyway, so why
Andrew> not just rename decref_target to target_ops::decref?

This looks good to me, but it seems like it should come after patch #3,
because that patch removes a use of decref_target -- meaning it's still
needed to compile gdb at this point in the series.

Tom

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

* Re: [PATCHv3 3/7] gdb: have target_stack automate reference count handling
  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
  0 siblings, 0 replies; 36+ messages in thread
From: Tom Tromey @ 2022-11-18 17:25 UTC (permalink / raw)
  To: Andrew Burgess via Gdb-patches; +Cc: Andrew Burgess

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

This patch looks good.

Andrew> Connections are represented in Python with gdb.TargtetConnection

I noticed the "TargtetConnection" typo throughout the commit message.
There's an extra "t" in there.

Tom

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

* Re: [PATCHv3 4/7] gdb: remove the pop_all_targets (and friends) global functions
  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
  0 siblings, 0 replies; 36+ messages in thread
From: Tom Tromey @ 2022-11-18 17:29 UTC (permalink / raw)
  To: Andrew Burgess via Gdb-patches; +Cc: Andrew Burgess

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> This commit removes the global functions pop_all_targets,
Andrew> pop_all_targets_above, and pop_all_targets_at_and_above, and makes
Andrew> them methods on the inferior class.

This seems like an improvement to me.

Andrew> +void inferior::unpush_target_and_assert (struct target_ops *target)

There should be a newline after 'void'; there's a few instances of this.

Tom

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

* Re: [PATCHv3 7/7] gdb: some process_stratum_target should not be shared
  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
  1 sibling, 0 replies; 36+ messages in thread
From: Tom Tromey @ 2022-11-18 18:04 UTC (permalink / raw)
  To: Andrew Burgess via Gdb-patches; +Cc: Andrew Burgess

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> For things like the Linux and FreeBSD native targets, as well as the
Andrew> remote target, this is absolutely true (or was made true).  But some
Andrew> targets were never updated to be shareable, for example, the
Andrew> core_target, which is used when reading core-files, stores some of its
Andrew> state in the program_space, but also, the core-file and the executable
Andrew> being debugged are closely related.

Andrew> As each add-inferior call creates an inferior with a new
Andrew> program_space, and doesn't automatically copy the executable, or the
Andrew> current core-file, I don't think it really makes sense to "share"
Andrew> core_target objects between inferiors.

Yeah, probably the core target just doesn't even make sense to share.

I guess if we did want to share it, we could move the state into the
core target.  Maybe this is worthwhile to do anyway?  I see a bunch of
uses of core_bfd (which is a #define reaching into the program space),
but some, e.g. in linux_read_core_file_mappings, seem like they could be
replaced with a parameter.

Andrew> I think this behaviour might be confusing, so I'd like to have GDB not
Andrew> initially share the core connection.  Instead, when the user tries to
Andrew> add the new inferior a warning is given, and the new inferior is
Andrew> created without a connection, like this:

This makes sense to me.

Andrew> +  /* The core_target only works for the inferior in which it was initially
Andrew> +     opened, and can't be copied to some other inferior's target_stack.  */
Andrew> +  bool is_shareable () override
Andrew> +  { return false; }

However, why only mark the core target this way?

I think there are a lot of other targets that can't be
shared... remote-sim, all the trace targets, even I think windows-nat,
since it isn't multi-inferior-capable yet.

So maybe the default implementation should be 'return false' and then
specific known-good targets should override it?

Tom

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

end of thread, other threads:[~2022-11-18 19:05 UTC | newest]

Thread overview: 36+ 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

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).