[PATCHv2,1/2] gdb: Fix assert for extended-remote target

Message ID 20180808122317.GI3155@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess Aug. 8, 2018, 12:23 p.m. UTC
  * Tom Tromey <tom@tromey.com> [2018-08-02 13:21:36 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> gdb/ChangeLog:
> 
> Andrew> 	* target.c (dispose_inferior): Don't dispose of inferiors that are
> Andrew> 	already killed.
> 
> I think this is PR gdb/18050, so please mention that in the ChangeLog.
> 
> Thanks for the patch.  This is ok with that little change.

Thanks for the review.

The version I pushed is below, I made the following changes:

  * Updated description to mention PR gdb/18050
  * Moved the test into gdb.server/ as this seemed a better location.

Thanks,

Andrew

---

gdb: Fix assert for extended-remote target (PR gdb/18050)

Consider the following GDB session:

   (gdb) target extended-remote :2347
   (gdb) file /path/to/exe
   (gdb) set remote exec-file /path/to/exe
   (gdb) set detach-on-fork off
   (gdb) break breakpt
   (gdb) run
   # ... hits breakpoint
   (gdb) info inferiors
     Num  Description       Executable
   * 1    process 17001     /path/to/exe
     2    process 17002     /path/to/exe
   (gdb) kill
   (gdb) info inferiors
     Num  Description       Executable
   * 1    <null>            /path/to/exe
     2    process 17002     /path/to/exe
   (gdb) target extended-remote :2348
   ../../src/gdb/thread.c:660: internal-error: thread_info* any_thread_of_process(int): Assertion `pid != 0' failed.
   A problem internal to GDB has been detected,
   further debugging may prove unreliable.

Or, from bug PR gdb/18050:

   (gdb) start
   (gdb) add-inferior -exec /path/to/exe
   (gdb) target extended-remote :2347
   ../../src/gdb/thread.c:660: internal-error: thread_info* any_thread_of_process(int): Assertion `pid != 0' failed.
   A problem internal to GDB has been detected,
   further debugging may prove unreliable.

The issue is calling target.c:dispose_inferior with a killed inferior in
the inferior list.  This assertion is fixed in this commit.

The new test for this issue only runs on platforms that support
'detach-on-fork', and when using
'--target_board=native-extended-gdbserver'.

gdb/ChangeLog:

	PR gdb/18050:
	* target.c (dispose_inferior): Don't dispose of inferiors that are
	already killed.

gdb/testsuite/ChangeLog:

	PR gdb/18050:
	* gdb.server/extended-remote-restart.c: New file.
	* gdb.server/extended-remote-restart.exp: New file.
---
 gdb/ChangeLog                                      |   6 +
 gdb/target.c                                       |   6 +
 gdb/testsuite/ChangeLog                            |   6 +
 gdb/testsuite/gdb.server/extended-remote-restart.c |  60 ++++++++++
 .../gdb.server/extended-remote-restart.exp         | 132 +++++++++++++++++++++
 5 files changed, 210 insertions(+)
 create mode 100644 gdb/testsuite/gdb.server/extended-remote-restart.c
 create mode 100644 gdb/testsuite/gdb.server/extended-remote-restart.exp
  

Patch

diff --git a/gdb/target.c b/gdb/target.c
index a5245abb281..115e9ae4948 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2022,6 +2022,12 @@  target_pre_inferior (int from_tty)
 static int
 dispose_inferior (struct inferior *inf, void *args)
 {
+  /* Not all killed inferiors can, or will ever be, removed from the
+     inferior list.  Killed inferiors clearly don't need to be killed
+     again, so, we're done.  */
+  if (inf->pid == 0)
+    return 0;
+
   thread_info *thread = any_thread_of_inferior (inf);
   if (thread != NULL)
     {
diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.c b/gdb/testsuite/gdb.server/extended-remote-restart.c
new file mode 100644
index 00000000000..e195f48e080
--- /dev/null
+++ b/gdb/testsuite/gdb.server/extended-remote-restart.c
@@ -0,0 +1,60 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 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 <unistd.h>
+#include <stdlib.h>
+
+static void
+breakpt ()
+{
+  asm ("" ::: "memory");
+}
+
+static void
+go_child ()
+{
+  breakpt ();
+
+  while (1)
+    sleep (1);
+}
+
+static void
+go_parent ()
+{
+  breakpt ();
+
+  while (1)
+    sleep (1);
+}
+
+int
+main ()
+{
+  pid_t pid;
+
+  pid = fork ();
+  if (pid == -1)
+    abort ();
+
+  if (pid == 0)
+    go_child ();
+  else
+    go_parent ();
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.exp b/gdb/testsuite/gdb.server/extended-remote-restart.exp
new file mode 100644
index 00000000000..39fcb9e2e58
--- /dev/null
+++ b/gdb/testsuite/gdb.server/extended-remote-restart.exp
@@ -0,0 +1,132 @@ 
+# Copyright 2018 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 test is about restarting execution of a forked application when
+# using gdb extended remote target.
+#
+# There are two issues that the test tries to expose in GDB:
+#
+# 1. GDB would throw an assertion upon reconnecting to a remote target
+# if there was more than one inferior already active in GDB, and
+#
+# 2. GDB would not prune transient inferiors from the inferior list
+# when reconnecting to a remote target.  So, for example, an inferior
+# created by GDB to track the child of a fork would usually be removed
+# from the inferior list once the child exited.  However, reconnecting
+# to a remote target would result in the child inferior remaining in
+# the inferior list.
+
+# This test is only for extended remote targets.
+if {[target_info gdb_protocol] != "extended-remote"} {
+    continue
+}
+
+# This test also makes use of 'detach-on-fork' which is not supported
+# on all platforms.
+if { ![istarget "*-*-linux*"] && ![istarget "*-*-openbsd*"] } then {
+    continue
+}
+
+# And we need to be able to reconnect to gdbserver.
+set gdbserver_reconnect_p 1
+if { [info proc gdb_reconnect] == "" } {
+    return 0
+}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Core of the test.  DO_KILL_P controls whether we kill one of the
+# inferiors before reconnecting.  And FOLLOW_CHILD_P controls whether
+# we follow the child or the parent at the fork.
+proc test_reload { do_kill_p follow_child_p } {
+    global decimal
+    global binfile
+
+    clean_restart ${binfile}
+
+    if ![runto_main] then {
+	fail "can't run to main"
+	return 0
+    }
+
+    # Set detach-on-fork off
+    gdb_test_no_output "set detach-on-fork off"
+
+    set live_inf_ptn "process $decimal"
+    set dead_inf_ptn "<null>"
+
+    if ${follow_child_p} {
+	gdb_test_no_output "set follow-fork child"
+	set parent_prefix " "
+	set child_prefix "\\*"
+	set parent_inf_after_kill_ptn ${live_inf_ptn}
+	set child_inf_after_kill_ptn ${dead_inf_ptn}
+    } else {
+	gdb_test_no_output "set follow-fork parent"
+	set parent_prefix "\\*"
+	set child_prefix " "
+	set parent_inf_after_kill_ptn ${dead_inf_ptn}
+	set child_inf_after_kill_ptn ${live_inf_ptn}
+    }
+
+    gdb_breakpoint "breakpt"
+    gdb_continue_to_breakpoint "breakpt"
+
+    # Check we have the expected inferiors.
+    gdb_test "info inferiors" \
+	[multi_line \
+	     "  Num  Description       Executable.*" \
+	     "${parent_prefix} 1 +${live_inf_ptn} \[^\r\n\]+" \
+	     "${child_prefix} 2 +${live_inf_ptn} \[^\r\n\]+" ] \
+	"Check inferiors at breakpoint"
+
+    if { $do_kill_p } {
+	# (Optional) Kill one of the inferiors.
+	gdb_test "kill" \
+	    "" \
+	    "Kill inferior" \
+	    "Kill the program being debugged.*y or n. $" \
+	    "y"
+
+	# Check the first inferior really did die.
+	gdb_test "info inferiors" \
+	    [multi_line \
+		 "  Num  Description       Executable.*" \
+		 "${parent_prefix} 1 +${parent_inf_after_kill_ptn} \[^\r\n\]+" \
+		 "${child_prefix} 2 +${child_inf_after_kill_ptn} \[^\r\n\]+" ] \
+	    "Check inferior was killed"
+    }
+
+    # Reconnect to the target.
+    if { [gdb_reconnect] == 0 } {
+	pass "reconnect after fork"
+    } else {
+	fail "reconnect after fork"
+    }
+}
+
+# Run all combinations of the test.
+foreach do_kill_p [list 1 0] {
+    foreach follow_child_p [list 1 0] {
+	with_test_prefix \
+	    "kill: ${do_kill_p}, follow-child ${follow_child_p}" {
+		test_reload ${do_kill_p} ${follow_child_p}
+	    }
+    }
+}