[PATCH 8/8] Fix removing inferiors from within "thread apply" commands

[Pedro Alves <palves at redhat dot com>]

This patch fixes an internal error exposed by a test that does
something like:

  define kill-and-remove
    kill inferiors 2
    remove-inferiors 2
  end

  # Start one inferior.
  start

  # Start another inferior.
  add-inferior 2
  inferior 2
  start

  # Kill and remove inferior 1 while inferior 2 is selected.
  thread apply 1.1 kill-and-remove

The internal error looks like this:

 Thread 1.1 (Thread 0x7ffff7fc2700 (LWP 20677)):
 [Switching to inferior 1 [process 20677] (gdb/testsuite/outputs/gdb.threads/threadapply/threadapply)]
 [Switching to thread 1.1 (Thread 0x7ffff7fc2700 (LWP 20677))]
 #0  main () at src/gdb/testsuite/gdb.threads/threadapply.c:38
 38          for (i = 0; i < NUM; i++)
 src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed.
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.
 Quit this debugging session? (y or n) FAIL: gdb.threads/threadapply.exp: kill_and_remove_inferior: try kill-and-remove: thread apply 1.1 kill-and-remove (GDB internal error)

There are several problems around this area of the code.  One is that
in do_restore_current_thread_cleanup, we do a look up of inferior by
ptid, which can find the wrong inferior if the previously selected
inferior exited and some other inferior was started with a reused pid
(rare, but still...).

The other problem is that the "remove-inferiors" command rejects
attempts to remove the current inferior, but when we get to
"remove-inferiors" in a "thread apply THR remove-inferiors 2" command,
the current inferior is the inferior of thread THR, not the previously
selected inferior, so if the previously selected inferior was inferior
2, that command still manages to wipe it, and then gdb restores the
old selected inferior, which is now a dangling pointer...

So the fix here is:

- Make make_cleanup_restore_current_thread store a pointer to the
  previously selected inferior directly, and use it directly instead
  of doing ptid look ups.

- Add a refcount to inferiors, very similar to thread_info's refcount,
  that is incremented/decremented by
  make_cleanup_restore_current_thread, and checked before deleting an
  inferior.

gdb/ChangeLog:
2017-04-11  Pedro Alves  <palves@redhat.com>

	* inferior.c (prune_inferiors, remove_inferior_command): Skip
	inferiors marked not-deletable instead of comparing to the
	currente inferior.
	* inferior.h (struct inferior): Forward declare.
	(current_inferior, set_current_inferior): Move declaration to
	before struct inferior's definition, and fix comment.
	(inferior::deletable, inferior::incref, inferior::decref): New
	methods.
	(m_refcount): New private field.
	* thread.c (switch_to_to_thread): New function.
	(switch_to_thread (thread_info *)): New function, factored out
	from ...
	(switch_to_thread (ptid_t)): ... this.
	(restore_current_thread): Delete.
	(current_thread_cleanup): Remove 'inf_id' and 'was_removable'
	fields, and add 'inf' field.
	(do_restore_current_thread_cleanup): Check whether old->inf is
	alive instead of looking up an inferior by ptid.  Use
	switch_to_thread and switch_to_no_thread.  Assert that the current
	inferior matches inferior_ptid.
	(restore_current_thread_cleanup_dtor): Use old->inf directly
	instead of lookup up an inferior by id.  Decref the inferior.
	Don't restore 'removable'.
	(make_cleanup_restore_current_thread): Same the inferior pointer
	in old, instead of the inferior number.  Incref the inferior.
	Don't save/clear 'removable'.

gdb/testsuite/ChangeLog:
2017-04-11  Pedro Alves  <palves@redhat.com>

	* gdb.threads/threadapply.exp (kill_and_remove_inferior): New
	procedure.
	(top level): Call it.
	* lib/gdb.exp (gdb_define_cmd): New procedure.
---
 gdb/inferior.c                            |   5 +-
 gdb/inferior.h                            |  40 +++++++--
 gdb/testsuite/gdb.threads/threadapply.exp | 135 ++++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                 |  19 +++++
 gdb/thread.c                              |  85 ++++++++++---------
 5 files changed, 237 insertions(+), 47 deletions(-)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 327590a..c918a5b 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -483,13 +483,12 @@ void
 prune_inferiors (void)
 {
   struct inferior *ss, **ss_link;
-  struct inferior *current = current_inferior ();
 
   ss = inferior_list;
   ss_link = &inferior_list;
   while (ss)
     {
-      if (ss == current
+      if (!ss->deletable ()
 	  || !ss->removable
 	  || ss->pid != 0)
 	{
@@ -774,7 +773,7 @@ remove_inferior_command (char *args, int from_tty)
 	  continue;
 	}
 
-      if (inf == current_inferior ())
+      if (!inf->deletable ())
 	{
 	  warning (_("Can not remove current inferior %d."), num);
 	  continue;
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 44f8157..89acb83 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -32,6 +32,7 @@ struct terminal_info;
 struct target_desc_info;
 struct gdb_environ;
 struct continuation;
+struct inferior;
 
 /* For bpstat.  */
 #include "breakpoint.h"
@@ -298,6 +299,11 @@ struct inferior_control_state
   enum stop_kind stop_soon;
 };
 
+/* Return a pointer to the current inferior.  */
+extern inferior *current_inferior ();
+
+extern void set_current_inferior (inferior *);
+
 /* GDB represents the state of each program execution with an object
    called an inferior.  An inferior typically corresponds to a process
    but is more general and applies also to targets that do not have a
@@ -313,9 +319,31 @@ public:
   explicit inferior (int pid);
   ~inferior ();
 
+  /* Returns true if we can delete this inferior.  We can't delete it
+     if it is the current inferior, or if there's code out there that
+     relies on it existing (m_refcount > 0).  */
+  bool deletable () const
+  {
+    return m_refcount == 0 && this != current_inferior ();
+  }
+
   /* Pointer to next inferior in singly-linked list of inferiors.  */
   struct inferior *next = NULL;
 
+  /* Increase the refcount.  */
+  void incref ()
+  {
+    gdb_assert (m_refcount >= 0);
+    m_refcount++;
+  }
+
+  /* Decrease the refcount.  */
+  void decref ()
+  {
+    m_refcount--;
+    gdb_assert (m_refcount >= 0);
+  }
+
   /* Convenient handle (GDB inferior id).  Unique across all
      inferiors.  */
   int num = 0;
@@ -431,6 +459,12 @@ public:
 
   /* Per inferior data-pointers required by other GDB modules.  */
   REGISTRY_FIELDS;
+
+private:
+  /* If this is > 0, then it means there's code out there that relies
+     on this inferior existing.  Don't allow deleting it in that
+     case.  */
+  int m_refcount = 0;
 };
 
 /* Keep a registry of per-inferior data-pointers required by other GDB
@@ -517,12 +551,6 @@ extern int number_of_live_inferiors (void);
    (not cores, not executables, real live processes).  */
 extern int have_live_inferiors (void);
 
-/* Return a pointer to the current inferior.  It is an error to call
-   this if there is no current inferior.  */
-extern struct inferior *current_inferior (void);
-
-extern void set_current_inferior (struct inferior *);
-
 extern struct cleanup *save_current_inferior (void);
 
 /* Traverse all inferiors.  */
diff --git a/gdb/testsuite/gdb.threads/threadapply.exp b/gdb/testsuite/gdb.threads/threadapply.exp
index 959e8b9..39687da 100644
--- a/gdb/testsuite/gdb.threads/threadapply.exp
+++ b/gdb/testsuite/gdb.threads/threadapply.exp
@@ -93,3 +93,138 @@ proc thr_apply_detach {thread_set} {
 foreach thread_set {"all" "1.1 1.2 1.3"} {
     thr_apply_detach $thread_set
 }
+
+# Test killing and removing inferiors from a command run via "thread
+# apply THREAD_SET".  THREAD_SET can be either "1.1", or "all".  GDB
+# used to mistakenly allow deleting the previously-selected inferior,
+# in some cases, leading to crashes.
+
+proc kill_and_remove_inferior {thread_set} {
+    global binfile
+    global gdb_prompt
+
+    # The test starts multiple inferiors, therefore non-extended
+    # remote is not supported.
+    if [target_info exists use_gdb_stub] {
+	unsupported "using gdb stub"
+	return
+    }
+
+    set any "\[^\r\n\]*"
+    set ws "\[ \t\]\+"
+
+    clean_restart ${binfile}
+
+    with_test_prefix "start inferior 1" {
+	runto_main
+    }
+
+    # Add and start inferior number NUM.
+    proc add_and_start_inferior {num} {
+	global binfile
+
+	# Start another inferior.
+	gdb_test "add-inferior" "Added inferior $num.*" \
+	    "add empty inferior $num"
+	gdb_test "inferior $num" "Switching to inferior $num.*" \
+	    "switch to inferior $num"
+	gdb_test "file ${binfile}" ".*" "load file in inferior $num"
+
+	with_test_prefix "start inferior $num" {
+	    runto_main
+	}
+    }
+
+    # Start another inferior.
+    add_and_start_inferior 2
+
+    # And yet another.
+    add_and_start_inferior 3
+
+    gdb_test "thread 2.1" "Switching to thread 2.1 .*"
+
+    # Try removing an active inferior via a "thread apply" command.
+    # Should fail/warn.
+    with_test_prefix "try remove" {
+
+	gdb_define_cmd "remove" {
+	    "remove-inferiors 3"
+	}
+
+	# Inferior 3 is still alive, so can't remove it.
+	gdb_test "thread apply $thread_set remove" \
+	    "warning: Can not remove active inferior 3.*"
+	# Check that GDB restored the selected thread.
+	gdb_test "thread" "Current thread is 2.1 .*"
+
+	gdb_test "info inferiors" \
+	    [multi_line \
+		 "${ws}1${ws}process ${any}" \
+		 "\\\* 2${ws}process ${any}" \
+		 "${ws}3${ws}process ${any}" \
+		]
+    }
+
+    # Kill and try to remove inferior 2 while inferior 2 is selected.
+    # Removing the inferior should fail/warn.
+    with_test_prefix "try kill-and-remove" {
+
+	# The "inferior 1" command works around PR gdb/19318 ("kill
+	# inferior N" shouldn't switch to inferior N).
+	gdb_define_cmd "kill-and-remove" {
+	    "kill inferiors 2"
+	    "inferior 1"
+	    "remove-inferiors 2"
+	}
+
+	# Note that when threads=1.1, this makes sure we're really
+	# testing failure to remove the inferior the user had selected
+	# before the "thread apply" command, instead of testing
+	# refusal to remove the currently-iterated inferior.
+	gdb_test "thread apply $thread_set kill-and-remove" \
+	    "warning: Can not remove current inferior 2.*"
+	gdb_test "thread" "No thread selected" \
+	    "switched to no thread selected"
+
+	gdb_test "info inferiors" \
+	    [multi_line \
+		 "${ws}1${ws}process ${any}" \
+		 "\\\* 2${ws}<null>${any}" \
+		 "${ws}3${ws}process ${any}" \
+		]
+    }
+
+    # Try removing (the now dead) inferior 2 while inferior 1 is
+    # selected.  This should succeed.
+    with_test_prefix "try remove 2" {
+
+	gdb_test "thread 1.1" "Switching to thread 1.1 .*"
+
+	gdb_define_cmd "remove-again" {
+	    "remove-inferiors 2"
+	}
+
+	set test "thread apply $thread_set remove-again"
+	gdb_test_multiple $test $test {
+	    -re "warning: Can not remove.*$gdb_prompt $" {
+		fail $test
+	    }
+	    -re "$gdb_prompt $" {
+		pass $test
+	    }
+	}
+	gdb_test "thread" "Current thread is 1.1 .*"
+	# Check that only inferiors 1 and 3 are around.
+	gdb_test "info inferiors" \
+	    [multi_line \
+		 "\\\* 1${ws}process ${any}" \
+		 "${ws}3${ws}process ${any}" \
+		]
+    }
+}
+
+# Test both "all" and a thread list, because those are implemented as
+# different commands in GDB.
+foreach_with_prefix thread_set {"all" "1.1"} {
+    kill_and_remove_inferior $thread_set
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index ea77361..6633d24 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -6070,5 +6070,24 @@ proc dejagnu_version { } {
     return $dg_ver
 }
 
+# Define user-defined command COMMAND using the COMMAND_LIST as the
+# command's definition.  The terminating "end" is added automatically.
+
+proc gdb_define_cmd {command command_list} {
+    global gdb_prompt
+
+    set input [multi_line_input {*}$command_list "end"]
+    set test "define $command"
+
+    gdb_test_multiple "define $command" $test {
+	-re "End with"  {
+	    gdb_test_multiple $input $test {
+		-re "\r\n$gdb_prompt " {
+		}
+	    }
+	}
+    }
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
diff --git a/gdb/thread.c b/gdb/thread.c
index 88fd521..f48d871 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -68,7 +68,6 @@ static void thread_apply_all_command (char *, int);
 static int thread_alive (struct thread_info *);
 static void info_threads_command (char *, int);
 static void thread_apply_command (char *, int);
-static void restore_current_thread (ptid_t);
 
 /* RAII type used to increase / decrease the refcount of each thread
    in a given list of threads.  */
@@ -1469,45 +1468,56 @@ switch_to_thread_no_regs (struct thread_info *thread)
   stop_pc = ~(CORE_ADDR) 0;
 }
 
-/* Switch from one thread to another.  */
+/* Switch to no thread selected.  */
 
-void
-switch_to_thread (ptid_t ptid)
+static void
+switch_to_no_thread ()
 {
-  /* Switch the program space as well, if we can infer it from the now
-     current thread.  Otherwise, it's up to the caller to select the
-     space it wants.  */
-  if (ptid != null_ptid)
-    {
-      struct inferior *inf;
+  if (inferior_ptid == null_ptid)
+    return;
 
-      inf = find_inferior_ptid (ptid);
-      gdb_assert (inf != NULL);
-      set_current_program_space (inf->pspace);
-      set_current_inferior (inf);
-    }
+  inferior_ptid = null_ptid;
+  reinit_frame_cache ();
+  stop_pc = ~(CORE_ADDR) 0;
+}
 
-  if (ptid == inferior_ptid)
+/* Switch from one thread to another.  */
+
+static void
+switch_to_thread (thread_info *thr)
+{
+  gdb_assert (thr != NULL);
+
+  if (inferior_ptid == thr->ptid)
     return;
 
-  inferior_ptid = ptid;
+  /* Switch the current program space and current inferior as
+     well.  */
+  set_current_program_space (thr->inf->pspace);
+  set_current_inferior (thr->inf);
+
+  inferior_ptid = thr->ptid;
   reinit_frame_cache ();
 
   /* We don't check for is_stopped, because we're called at times
      while in the TARGET_RUNNING state, e.g., while handling an
      internal event.  */
-  if (inferior_ptid != null_ptid
-      && !is_exited (ptid)
-      && !is_executing (ptid))
-    stop_pc = regcache_read_pc (get_thread_regcache (ptid));
+  if (thr->state != THREAD_EXITED
+      && !thr->executing)
+    stop_pc = regcache_read_pc (get_thread_regcache (thr->ptid));
   else
     stop_pc = ~(CORE_ADDR) 0;
 }
 
-static void
-restore_current_thread (ptid_t ptid)
+/* See gdbthread.h.  */
+
+void
+switch_to_thread (ptid_t ptid)
 {
-  switch_to_thread (ptid);
+  if (ptid == null_ptid)
+    switch_to_no_thread ();
+  else
+    switch_to_thread (find_thread_ptid (ptid));
 }
 
 static void
@@ -1578,8 +1588,7 @@ struct current_thread_cleanup
   struct frame_id selected_frame_id;
   int selected_frame_level;
   int was_stopped;
-  int inf_id;
-  int was_removable;
+  inferior *inf;
 };
 
 static void
@@ -1595,12 +1604,12 @@ do_restore_current_thread_cleanup (void *arg)
       /* If the previously selected thread belonged to a process that has
 	 in the mean time exited (or killed, detached, etc.), then don't revert
 	 back to it, but instead simply drop back to no thread selected.  */
-      && find_inferior_ptid (old->thread->ptid) != NULL)
-    restore_current_thread (old->thread->ptid);
+      && old->inf->pid != 0)
+    switch_to_thread (old->thread);
   else
     {
-      restore_current_thread (null_ptid);
-      set_current_inferior (find_inferior_id (old->inf_id));
+      switch_to_no_thread ();
+      set_current_inferior (old->inf);
     }
 
   /* The running state of the originally selected thread may have
@@ -1613,21 +1622,22 @@ do_restore_current_thread_cleanup (void *arg)
       && target_has_memory)
     restore_selected_frame (old->selected_frame_id,
 			    old->selected_frame_level);
+
+  if (inferior_ptid == null_ptid)
+    gdb_assert (current_inferior ()->pid == 0);
+  else
+    gdb_assert (current_inferior ()->pid == inferior_ptid.pid ());
 }
 
 static void
 restore_current_thread_cleanup_dtor (void *arg)
 {
   struct current_thread_cleanup *old = (struct current_thread_cleanup *) arg;
-  struct thread_info *tp;
-  struct inferior *inf;
 
   if (old->thread != NULL)
     old->thread->decref ();
 
-  inf = find_inferior_id (old->inf_id);
-  if (inf != NULL)
-    inf->removable = old->was_removable;
+  old->inf->decref ();
   xfree (old);
 }
 
@@ -1637,8 +1647,7 @@ make_cleanup_restore_current_thread (void)
   struct current_thread_cleanup *old = XNEW (struct current_thread_cleanup);
 
   old->thread = NULL;
-  old->inf_id = current_inferior ()->num;
-  old->was_removable = current_inferior ()->removable;
+  old->inf = current_inferior ();
 
   if (inferior_ptid != null_ptid)
     {
@@ -1670,7 +1679,7 @@ make_cleanup_restore_current_thread (void)
       old->thread = tp;
     }
 
-  current_inferior ()->removable = 0;
+  old->inf->incref ();
 
   return make_cleanup_dtor (do_restore_current_thread_cleanup, old,
 			    restore_current_thread_cleanup_dtor);
-- 
2.5.5