remote, defer deleting the inferior until it is mourned

remote, defer deleting the inferior until it is mourned

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

If we delete an inferior with delete_inferior (which tries to deletes all of
its child threads) while one of its threads is selected, delete_thread will
refuse to unlist it, but will instead tag it as exited.  When we get to do a
generic_mourn_inferior, we will no longer find the inferior listed, so this
exited thread was left behind, but we do set inferior_ptid to null_ptid.  This
happens to then trigger an internal error in mi-main.c:mi_execute_command: ...

      if (/* The notifications are only output when the top-level
	     interpreter (specified on the command line) is MI.  */      
	  ui_out_is_mi_like_p (interp_ui_out (top_level_interpreter ()))
	  /* Don't try report anything if there are no threads -- 
	     the program is dead.  */
	  && thread_count () != 0
	  /* -thread-select explicitly changes thread. If frontend uses that
	     internally, we don't want to emit =thread-selected, since
	     =thread-selected is supposed to indicate user's intentions.  */
	  && strcmp (command->command, "thread-select") != 0)
	{
	  struct mi_interp *mi = top_level_interpreter_data ();
	  struct thread_info *ti = inferior_thread ();
                                   ^^^^^^^^^^^^^^^^^^

... here, since inferior_ptid was pointing at null_ptid, and, thread_count()
returned 1.  null_ptid is never a listed thread, so inferior_thread rightfully
asserts.

To fix this, we either set inferior_ptid to null_ptid before deleting the
inferior when we want to prevent this, or, we defer deleting the inferior to
generic_mourn_inferior, which does just that.  I've done the latter.  This
also goes in the direction of what we were talking about in the infrun
cleanup thread, about the inferior being already gone when we handle an
inferior exit event.

Tested against an x86_64-linux-gnu gdbserver.  Checked in.

-- 
Pedro Alves

[-- Attachment #2: remote_mourn.diff --]
[-- Type: text/x-diff, Size: 2368 bytes --]

2008-12-12  Pedro Alves  <pedro@codesourcery.com>

	* remote.c (remote_detach_1): Don't delete the inferior here.
	(process_stop_reply): Ditto.
	(extended_remote_kill): Ditto.

---
 gdb/remote.c |   41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-12-12 00:48:29.000000000 +0000
+++ src/gdb/remote.c	2008-12-12 00:48:37.000000000 +0000
@@ -3316,7 +3316,6 @@ remote_detach_1 (char *args, int from_tt
     }
 
   discard_pending_stop_replies (pid);
-  detach_inferior (pid);
   target_mourn_inferior ();
 }
 
@@ -4510,31 +4509,28 @@ process_stop_reply (struct stop_reply *s
   if (ptid_equal (ptid, null_ptid))
     ptid = inferior_ptid;
 
-  if (status->kind == TARGET_WAITKIND_EXITED
-      || status->kind == TARGET_WAITKIND_SIGNALLED)
+  if (status->kind != TARGET_WAITKIND_EXITED
+      && status->kind != TARGET_WAITKIND_SIGNALLED)
     {
-      int pid = ptid_get_pid (ptid);
-      delete_inferior (pid);
-    }
-  else
-    notice_new_inferiors (ptid);
+      notice_new_inferiors (ptid);
 
-  /* Expedited registers.  */
-  if (stop_reply->regcache)
-    {
-      cached_reg_t *reg;
-      int ix;
+      /* Expedited registers.  */
+      if (stop_reply->regcache)
+	{
+	  cached_reg_t *reg;
+	  int ix;
 
-      for (ix = 0;
-	   VEC_iterate(cached_reg_t, stop_reply->regcache, ix, reg);
-	   ix++)
-	regcache_raw_supply (get_thread_regcache (ptid),
-			     reg->num, reg->data);
-      VEC_free (cached_reg_t, stop_reply->regcache);
-    }
+	  for (ix = 0;
+	       VEC_iterate(cached_reg_t, stop_reply->regcache, ix, reg);
+	       ix++)
+	    regcache_raw_supply (get_thread_regcache (ptid),
+				 reg->num, reg->data);
+	  VEC_free (cached_reg_t, stop_reply->regcache);
+	}
 
-  remote_stopped_by_watchpoint_p = stop_reply->stopped_by_watchpoint_p;
-  remote_watch_data_address = stop_reply->watch_data_address;
+      remote_stopped_by_watchpoint_p = stop_reply->stopped_by_watchpoint_p;
+      remote_watch_data_address = stop_reply->watch_data_address;
+    }
 
   stop_reply_xfree (stop_reply);
   return ptid;
@@ -6509,7 +6505,6 @@ extended_remote_kill (void)
   if (res != 0)
     error (_("Can't kill process"));
 
-  delete_inferior (pid);
   target_mourn_inferior ();
 }
 

Re: remote, defer deleting the inferior until it is mourned

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

On Friday 12 December 2008 01:11:13, Pedro Alves wrote:
> To fix this, we either set inferior_ptid to null_ptid before deleting the
> inferior when we want to prevent this, or, we defer deleting the inferior to
> generic_mourn_inferior, which does just that.  I've done the latter.

Next in queue, make the extended-remote version of mourning always
call generic_mourn_inferior, to always get rid of the current inferior,
even if there are more inferiors to debug.  This only became possible
since the change to make handle_inferior_event switch inferior_ptid to the
event ptid on a process exit (we were also discarding leftover
unprocessed events from the wrong inferior before that change, just a
bit above the generic_mourn_inferior call I'm moving).

Tested against an x86_64-linux-gnu gdbserver.  Checked in.


The next issue is that extended_remote_mourn is handling the switching
to another process itself, which was an idea borrowed from the linux
multi-fork support.  This is problematic for a few reasons, and would be
best done in common code.  More on that soon.

-- 
Pedro Alves

[-- Attachment #2: remote_mourn_2.diff --]
[-- Type: text/x-diff, Size: 1163 bytes --]

2008-12-12  Pedro Alves  <pedro@codesourcery.com>

	* remote.c (extended_remote_mourn_1): Always call
	generic_mourn_inferior.

---
 gdb/remote.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-12-12 13:03:23.000000000 +0000
+++ src/gdb/remote.c	2008-12-12 13:03:52.000000000 +0000
@@ -6551,6 +6551,9 @@ extended_remote_mourn_1 (struct target_o
   /* Unlike "target remote", we do not want to unpush the target; then
      the next time the user says "run", we won't be connected.  */
 
+  /* Call common code to mark the inferior as not running.	*/
+  generic_mourn_inferior ();
+
   if (have_inferiors ())
     {
       extern void nullify_last_target_wait_ptid ();
@@ -6562,10 +6565,6 @@ extended_remote_mourn_1 (struct target_o
     }
   else
     {
-      struct remote_state *rs = get_remote_state ();
-
-      /* Call common code to mark the inferior as not running.	*/
-      generic_mourn_inferior ();
       if (!remote_multi_process_p (rs))
 	{
 	  /* Check whether the target is running now - some remote stubs