[RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Kevin Buettner]

The patch below updates remote-mips.c so inferior_ptid is set after
successfully connecting to a target board.  A suitable inferior
and thread are also created at that time.  I was surprised to find
that I also needed to fiddle with the inferior's aspace and pspace.

Calls to target_mourn_inferior() have also been added.

Using this patch in conjunction with some other soon-to-be-submitted
patches allows me to connect to a board and debug a program on the
board.

Comments?

	* remote-mips.c (gdbthread.h): Include.
	(magic_null_ptid): Declare.
	(common_open): Set inferior_ptid, add it as an inferior, and
	as a thread too.  Set up the inferior's address spaces.
	(mips_kill): Make sure that target_mourn_inferior is invoked.
	(mips_load): Don't null out inferior_ptid.
	(_initialize_remote_mips): Initialize magic_null_ptid.

Index: remote-mips.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-mips.c,v
retrieving revision 1.108
diff -u -p -r1.108 remote-mips.c
--- remote-mips.c	26 Feb 2010 23:11:24 -0000	1.108
+++ remote-mips.c	2 Mar 2010 00:17:56 -0000
@@ -35,6 +35,7 @@
 #include "regcache.h"
 #include <ctype.h>
 #include "mips-tdep.h"
+#include "gdbthread.h"
 \f
 
 /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
@@ -1457,6 +1458,8 @@ mips_initialize (void)
   mips_request ('r', 0, 0, &err, mips_receive_wait, NULL);
 }
 
+static ptid_t magic_null_ptid;
+
 /* Open a connection to the remote board.  */
 static void
 common_open (struct target_ops *ops, char *name, int from_tty,
@@ -1468,6 +1471,7 @@ common_open (struct target_ops *ops, cha
   char *remote_name = 0;
   char *local_name = 0;
   char **argv;
+  struct inferior *inf;
 
   if (name == 0)
     error (
@@ -1563,6 +1567,12 @@ device is attached to the target board (
   /* Switch to using remote target now.  */
   push_target (ops);
 
+  inferior_ptid = magic_null_ptid;
+  inf = add_inferior_silent (ptid_get_pid (inferior_ptid));
+  inf->aspace = current_program_space->aspace;
+  inf->pspace = current_program_space;
+  add_thread_silent (inferior_ptid);
+
   /* FIXME: Should we call start_remote here?  */
 
   /* Try to figure out the processor model if possible.  */
@@ -2140,7 +2150,10 @@ static void
 mips_kill (struct target_ops *ops)
 {
   if (!mips_wait_flag)
-    return;
+    {
+      target_mourn_inferior ();
+      return;
+    }
 
   interrupt_count++;
 
@@ -2173,6 +2186,8 @@ Give up (and stop debugging it)? ")))
 
   serial_send_break (mips_desc);
 
+  target_mourn_inferior ();
+
 #if 0
   if (mips_is_open)
     {
@@ -3297,8 +3312,6 @@ mips_load (char *file, int from_tty)
   if (exec_bfd)
     regcache_write_pc (regcache, bfd_get_start_address (exec_bfd));
 
-  inferior_ptid = null_ptid;	/* No process now */
-
 /* This is necessary because many things were based on the PC at the time that
    we attached to the monitor, which is no longer valid now that we have loaded
    new code (and just changed the PC).  Another way to do this might be to call
@@ -3458,4 +3471,5 @@ Use \"on\" to enable the masking and \"o
 			   NULL,
 			   NULL, /* FIXME: i18n: */
 			   &setlist, &showlist);
+  magic_null_ptid = ptid_build (42000, 1, -1);
 }

Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Kevin Buettner]

Below is a slight revision over yesterday's patch.   The only difference
between this patch and the previous one is the addition of the change to
mips_error().  That change avoids an internal error when inferior_ptid
is the same as null_ptid.

I'll wait a few more days for comments before checking this in.

Kevin

	* remote-mips.c (gdbthread.h): Include.
	(mips_error): Only mourn the inferior when inferior_ptid is non-null.
	(magic_null_ptid): Declare.
	(common_open): Set inferior_ptid, add it as an inferior, and
	as a thread too.  Set up the inferior's address spaces.
	(mips_kill): Make sure that target_mourn_inferior is invoked.
	(mips_load): Don't null out inferior_ptid.
	(_initialize_remote_mips): Initialize magic_null_ptid.

Index: remote-mips.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-mips.c,v
retrieving revision 1.108
diff -u -p -r1.108 remote-mips.c
--- remote-mips.c	26 Feb 2010 23:11:24 -0000	1.108
+++ remote-mips.c	2 Mar 2010 23:02:32 -0000
@@ -35,6 +35,7 @@
 #include "regcache.h"
 #include <ctype.h>
 #include "mips-tdep.h"
+#include "gdbthread.h"
 \f
 
 /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
@@ -483,7 +484,8 @@ mips_error (char *string,...)
   close_ports ();
 
   printf_unfiltered ("Ending remote MIPS debugging.\n");
-  target_mourn_inferior ();
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    target_mourn_inferior ();
 
   deprecated_throw_reason (RETURN_ERROR);
 }
@@ -1457,6 +1459,8 @@ mips_initialize (void)
   mips_request ('r', 0, 0, &err, mips_receive_wait, NULL);
 }
 
+static ptid_t magic_null_ptid;
+
 /* Open a connection to the remote board.  */
 static void
 common_open (struct target_ops *ops, char *name, int from_tty,
@@ -1468,6 +1472,7 @@ common_open (struct target_ops *ops, cha
   char *remote_name = 0;
   char *local_name = 0;
   char **argv;
+  struct inferior *inf;
 
   if (name == 0)
     error (
@@ -1563,6 +1568,12 @@ device is attached to the target board (
   /* Switch to using remote target now.  */
   push_target (ops);
 
+  inferior_ptid = magic_null_ptid;
+  inf = add_inferior_silent (ptid_get_pid (inferior_ptid));
+  inf->aspace = current_program_space->aspace;
+  inf->pspace = current_program_space;
+  add_thread_silent (inferior_ptid);
+
   /* FIXME: Should we call start_remote here?  */
 
   /* Try to figure out the processor model if possible.  */
@@ -2140,7 +2151,10 @@ static void
 mips_kill (struct target_ops *ops)
 {
   if (!mips_wait_flag)
-    return;
+    {
+      target_mourn_inferior ();
+      return;
+    }
 
   interrupt_count++;
 
@@ -2173,6 +2187,8 @@ Give up (and stop debugging it)? ")))
 
   serial_send_break (mips_desc);
 
+  target_mourn_inferior ();
+
 #if 0
   if (mips_is_open)
     {
@@ -3297,8 +3313,6 @@ mips_load (char *file, int from_tty)
   if (exec_bfd)
     regcache_write_pc (regcache, bfd_get_start_address (exec_bfd));
 
-  inferior_ptid = null_ptid;	/* No process now */
-
 /* This is necessary because many things were based on the PC at the time that
    we attached to the monitor, which is no longer valid now that we have loaded
    new code (and just changed the PC).  Another way to do this might be to call
@@ -3458,4 +3472,5 @@ Use \"on\" to enable the masking and \"o
 			   NULL,
 			   NULL, /* FIXME: i18n: */
 			   &setlist, &showlist);
+  magic_null_ptid = ptid_build (42000, 1, -1);
 }

Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Pedro Alves]

On Tuesday 02 March 2010 23:09:20, Kevin Buettner wrote:
> Below is a slight revision over yesterday's patch.   The only difference
> between this patch and the previous one is the addition of the change to
> mips_error().  That change avoids an internal error when inferior_ptid
> is the same as null_ptid.
> 
> I'll wait a few more days for comments before checking this in.
> 
> Kevin
> 
> 	* remote-mips.c (gdbthread.h): Include.
> 	(mips_error): Only mourn the inferior when inferior_ptid is non-null.
> 	(magic_null_ptid): Declare.
> 	(common_open): Set inferior_ptid, add it as an inferior, and
> 	as a thread too.  Set up the inferior's address spaces.
> 	(mips_kill): Make sure that target_mourn_inferior is invoked.
> 	(mips_load): Don't null out inferior_ptid.
> 	(_initialize_remote_mips): Initialize magic_null_ptid.

Looks mostly okay, but are you missing a target_thread_alive
implementation?  See these:

 http://sourceware.org/ml/gdb-patches/2008-08/msg00475.html
 http://sourceware.org/ml/gdb-patches/2008-08/msg00479.html
 http://sourceware.org/ml/gdb-patches/2008-08/msg00531.html

Could you take a look and see if the other changes make
sense to you too?

You may also be interested in this:

 http://sourceware.org/ml/gdb-patches/2008-08/msg00530.html


> 
> Index: remote-mips.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote-mips.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 remote-mips.c
> --- remote-mips.c	26 Feb 2010 23:11:24 -0000	1.108
> +++ remote-mips.c	2 Mar 2010 23:02:32 -0000
> @@ -35,6 +35,7 @@
>  #include "regcache.h"
>  #include <ctype.h>
>  #include "mips-tdep.h"
> +#include "gdbthread.h"
>  \f
>  
>  /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
> @@ -483,7 +484,8 @@ mips_error (char *string,...)
>    close_ports ();
>  
>    printf_unfiltered ("Ending remote MIPS debugging.\n");
> -  target_mourn_inferior ();
> +  if (!ptid_equal (inferior_ptid, null_ptid))
> +    target_mourn_inferior ();
>  
>    deprecated_throw_reason (RETURN_ERROR);
>  }
> @@ -1457,6 +1459,8 @@ mips_initialize (void)
>    mips_request ('r', 0, 0, &err, mips_receive_wait, NULL);
>  }
>  
> +static ptid_t magic_null_ptid;
> +
>  /* Open a connection to the remote board.  */
>  static void
>  common_open (struct target_ops *ops, char *name, int from_tty,
> @@ -1468,6 +1472,7 @@ common_open (struct target_ops *ops, cha
>    char *remote_name = 0;
>    char *local_name = 0;
>    char **argv;
> +  struct inferior *inf;
>  
>    if (name == 0)
>      error (
> @@ -1563,6 +1568,12 @@ device is attached to the target board (
>    /* Switch to using remote target now.  */
>    push_target (ops);
>  
> +  inferior_ptid = magic_null_ptid;
> +  inf = add_inferior_silent (ptid_get_pid (inferior_ptid));
> +  inf->aspace = current_program_space->aspace;
> +  inf->pspace = current_program_space;
> +  add_thread_silent (inferior_ptid);
> +
>    /* FIXME: Should we call start_remote here?  */
>  
>    /* Try to figure out the processor model if possible.  */
> @@ -2140,7 +2151,10 @@ static void
>  mips_kill (struct target_ops *ops)
>  {
>    if (!mips_wait_flag)
> -    return;
> +    {
> +      target_mourn_inferior ();
> +      return;
> +    }
>  
>    interrupt_count++;
>  
> @@ -2173,6 +2187,8 @@ Give up (and stop debugging it)? ")))
>  
>    serial_send_break (mips_desc);
>  
> +  target_mourn_inferior ();
> +
>  #if 0
>    if (mips_is_open)
>      {
> @@ -3297,8 +3313,6 @@ mips_load (char *file, int from_tty)
>    if (exec_bfd)
>      regcache_write_pc (regcache, bfd_get_start_address (exec_bfd));
>  
> -  inferior_ptid = null_ptid;	/* No process now */
> -
>  /* This is necessary because many things were based on the PC at the time that
>     we attached to the monitor, which is no longer valid now that we have loaded
>     new code (and just changed the PC).  Another way to do this might be to call
> @@ -3458,4 +3472,5 @@ Use \"on\" to enable the masking and \"o
>  			   NULL,
>  			   NULL, /* FIXME: i18n: */
>  			   &setlist, &showlist);
> +  magic_null_ptid = ptid_build (42000, 1, -1);
>  }
> 
> 


-- 
Pedro Alves

Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Kevin Buettner]

On Tue, 2 Mar 2010 23:37:46 +0000
Pedro Alves <pedro@codesourcery.com> wrote:

> Looks mostly okay, but are you missing a target_thread_alive
> implementation?

Yes, I was.

Thanks for the review.  Thanks, too, for the pointers to your work on
this file.

> See these:
> 
>  http://sourceware.org/ml/gdb-patches/2008-08/msg00475.html

This looks like an earlier version of msg00531, below.  (Missing
from it are the thread_alive and pid_to_str implementations.)

>  http://sourceware.org/ml/gdb-patches/2008-08/msg00479.html
>  http://sourceware.org/ml/gdb-patches/2008-08/msg00531.html
> 
> Could you take a look and see if the other changes make
> sense to you too?

I've incorporated much of msg0051, "[02/02] remote-mips.c, always a
thread (take 2)", into my current patch.  (So, yes, the other changes
in your patch did make sense to me.)  I'll include your name in the
ChangeLog entry when it eventually gets committed.

I'd appreciate it if you could look over the changes to mips_close(),
below, since my current version differs from what you proposed in your
patch.  (I cribbed off of remote-sim.c for those changes.)

Also new from my last patch, in addition to your changes, is the
fact that mips_mourn_inferior() no longer calls generic_mourn_inferior().
That's now done in mips_close() which is called when the target is
popped.

> You may also be interested in this:
> 
> http://sourceware.org/ml/gdb-patches/2008-08/msg00530.html

This looks interesting, but I haven't tried it yet.  I'd like to get
the current patch and my other pending changes in first.

Below is my current patch, incorporating your work, plus a few
additional changes of my own.  Thanks again for looking this over.

Kevin

	* remote-mips.c (gdbthread.h): Include.
	(remote_mips_ptid): Declare.
	(mips_error): Only mourn the inferior when inferior_ptid is non-null.
	(common_open): Set inferior_ptid, add it as an inferior, and
	as a thread too.  Set up the inferior's address spaces.
	(mips_close): Invoke generic_mourn_inferior().  Delete the thread
	and the inferior.  Delete FIXME comment regarding common_open().
	(mips_kill): Make sure that target_mourn_inferior is invoked.
	(mips_mourn_inferior): Don't invoke generic_mourn_inferior, as
	it's now invoked from mips_close().
	(mips_load): Don't null out inferior_ptid.  Don't call
	clear_symtab_users().
	(mips_thread_alive, mips_pid_to_str): New functions.
	(_initialize_remote_mips): Initialize remote_mips_ptid.  Initialize
	to_thread_alive and to_pid_to_str operations.

Index: remote-mips.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-mips.c,v
retrieving revision 1.108
diff -u -p -r1.108 remote-mips.c
--- remote-mips.c	26 Feb 2010 23:11:24 -0000	1.108
+++ remote-mips.c	3 Mar 2010 22:57:34 -0000
@@ -35,6 +35,7 @@
 #include "regcache.h"
 #include <ctype.h>
 #include "mips-tdep.h"
+#include "gdbthread.h"
 \f
 
 /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
@@ -440,6 +441,11 @@ struct lsi_error lsi_error_table[] =
    of warnings returned by PMON when hardware breakpoints are used.  */
 static int monitor_warnings;
 
+/* This is the ptid we use while we're connected to the remote.  Its
+   value is arbitrary, as the remote-mips target doesn't have a notion of
+   processes or threads, but we need something non-null to place in
+   inferior_ptid.  */
+static ptid_t remote_mips_ptid;
 
 static void
 close_ports (void)
@@ -483,7 +489,8 @@ mips_error (char *string,...)
   close_ports ();
 
   printf_unfiltered ("Ending remote MIPS debugging.\n");
-  target_mourn_inferior ();
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    target_mourn_inferior ();
 
   deprecated_throw_reason (RETURN_ERROR);
 }
@@ -1468,6 +1475,7 @@ common_open (struct target_ops *ops, cha
   char *remote_name = 0;
   char *local_name = 0;
   char **argv;
+  struct inferior *inf;
 
   if (name == 0)
     error (
@@ -1563,7 +1571,11 @@ device is attached to the target board (
   /* Switch to using remote target now.  */
   push_target (ops);
 
-  /* FIXME: Should we call start_remote here?  */
+  inferior_ptid = remote_mips_ptid;
+  inf = add_inferior_silent (ptid_get_pid (inferior_ptid));
+  inf->aspace = current_program_space->aspace;
+  inf->pspace = current_program_space;
+  add_thread_silent (inferior_ptid);
 
   /* Try to figure out the processor model if possible.  */
   deprecated_mips_set_processor_regs_hack ();
@@ -1639,6 +1651,10 @@ mips_close (int quitting)
 
       close_ports ();
     }
+
+  generic_mourn_inferior ();
+  delete_thread_silent (remote_mips_ptid);
+  delete_inferior_silent (ptid_get_pid (remote_mips_ptid));
 }
 
 /* Detach from the remote board.  */
@@ -2140,7 +2156,10 @@ static void
 mips_kill (struct target_ops *ops)
 {
   if (!mips_wait_flag)
-    return;
+    {
+      target_mourn_inferior ();
+      return;
+    }
 
   interrupt_count++;
 
@@ -2173,6 +2192,8 @@ Give up (and stop debugging it)? ")))
 
   serial_send_break (mips_desc);
 
+  target_mourn_inferior ();
+
 #if 0
   if (mips_is_open)
     {
@@ -2210,19 +2231,17 @@ Can't pass arguments to remote MIPS boar
 
   init_wait_for_inferior ();
 
-  /* FIXME: Should we set inferior_ptid here?  */
-
   regcache_write_pc (get_current_regcache (), entry_pt);
 }
 
-/* Clean up after a process.  Actually nothing to do.  */
+/* Clean up after a process. The bulk of the work is done in mips_close(),
+   which is called when unpushing the target.  */
 
 static void
 mips_mourn_inferior (struct target_ops *ops)
 {
   if (current_ops != NULL)
     unpush_target (current_ops);
-  generic_mourn_inferior ();
 }
 \f
 /* We can write a breakpoint and read the shadow contents in one
@@ -3296,18 +3315,36 @@ mips_load (char *file, int from_tty)
     }
   if (exec_bfd)
     regcache_write_pc (regcache, bfd_get_start_address (exec_bfd));
+}
 
-  inferior_ptid = null_ptid;	/* No process now */
-
-/* This is necessary because many things were based on the PC at the time that
-   we attached to the monitor, which is no longer valid now that we have loaded
-   new code (and just changed the PC).  Another way to do this might be to call
-   normal_stop, except that the stack may not be valid, and things would get
-   horribly confused... */
+/* Check to see if a thread is still alive.  */
+ 
+static int
+mips_thread_alive (struct target_ops *ops, ptid_t ptid)
+{
+  if (ptid_equal (ptid, remote_mips_ptid))
+    /* The monitor's task is always alive.  */
+    return 1;
 
-  clear_symtab_users ();
+  return 0;
 }
 
+/* Convert a thread ID to a string.  Returns the string in a static
+   buffer.  */
+
+static char *
+mips_pid_to_str (struct target_ops *ops, ptid_t ptid)
+{
+  static char buf[64];
+
+  if (ptid_equal (ptid, remote_mips_ptid))
+    {
+      xsnprintf (buf, sizeof buf, "Thread <main>");
+      return buf;
+    }
+
+  return normal_pid_to_str (ptid);
+}
 
 /* Pass the command argument as a packet to PMON verbatim.  */
 
@@ -3351,6 +3388,8 @@ _initialize_remote_mips (void)
   mips_ops.to_load = mips_load;
   mips_ops.to_create_inferior = mips_create_inferior;
   mips_ops.to_mourn_inferior = mips_mourn_inferior;
+  mips_ops.to_thread_alive = mips_thread_alive;
+  mips_ops.to_pid_to_str = mips_pid_to_str;
   mips_ops.to_log_command = serial_log_command;
   mips_ops.to_stratum = process_stratum;
   mips_ops.to_has_all_memory = default_child_has_all_memory;
@@ -3458,4 +3497,5 @@ Use \"on\" to enable the masking and \"o
 			   NULL,
 			   NULL, /* FIXME: i18n: */
 			   &setlist, &showlist);
+  remote_mips_ptid = ptid_build (42000, 0, 42000);
 }

Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Pedro Alves]

On Wednesday 03 March 2010 23:44:22, Kevin Buettner wrote:
> On Tue, 2 Mar 2010 23:37:46 +0000
> Pedro Alves <pedro@codesourcery.com> wrote:
> 
> > Looks mostly okay, but are you missing a target_thread_alive
> > implementation?
> 
> Yes, I was.
> 
> Thanks for the review.  Thanks, too, for the pointers to your work on
> this file.
> 
> > See these:
> > 
> >  http://sourceware.org/ml/gdb-patches/2008-08/msg00475.html
> 
> This looks like an earlier version of msg00531, below.  (Missing
> from it are the thread_alive and pid_to_str implementations.)
> 
> >  http://sourceware.org/ml/gdb-patches/2008-08/msg00479.html
> >  http://sourceware.org/ml/gdb-patches/2008-08/msg00531.html
> > 
> > Could you take a look and see if the other changes make
> > sense to you too?
> 
> I've incorporated much of msg0051, "[02/02] remote-mips.c, always a
> thread (take 2)", into my current patch.  (So, yes, the other changes
> in your patch did make sense to me.)  I'll include your name in the
> ChangeLog entry when it eventually gets committed.
> 
> I'd appreciate it if you could look over the changes to mips_close(),
> below, since my current version differs from what you proposed in your
> patch.  (I cribbed off of remote-sim.c for those changes.)

Hmm, remote-sim.c shouldn't be calling delete_inferior_silent,
that's a bit of bitrot, but, I can see how it isn't
causing problems in the remote-sim.c's case.  You'll need
to change this copied bit in remote-mips.c.  See below.
Sorry about that.  :-(

> 
> Also new from my last patch, in addition to your changes, is the
> fact that mips_mourn_inferior() no longer calls generic_mourn_inferior().
> That's now done in mips_close() which is called when the target is
> popped.
> 
> > You may also be interested in this:
> > 
> > http://sourceware.org/ml/gdb-patches/2008-08/msg00530.html
> 
> This looks interesting, but I haven't tried it yet.  I'd like to get
> the current patch and my other pending changes in first.
> 
> Below is my current patch, incorporating your work, plus a few
> additional changes of my own.  Thanks again for looking this over.

Thank you.

> 
> Kevin
> 
> 	* remote-mips.c (gdbthread.h): Include.
> 	(remote_mips_ptid): Declare.
> 	(mips_error): Only mourn the inferior when inferior_ptid is non-null.
> 	(common_open): Set inferior_ptid, add it as an inferior, and
> 	as a thread too.  Set up the inferior's address spaces.
> 	(mips_close): Invoke generic_mourn_inferior().  Delete the thread
> 	and the inferior.  Delete FIXME comment regarding common_open().
> 	(mips_kill): Make sure that target_mourn_inferior is invoked.
> 	(mips_mourn_inferior): Don't invoke generic_mourn_inferior, as
> 	it's now invoked from mips_close().
> 	(mips_load): Don't null out inferior_ptid.  Don't call
> 	clear_symtab_users().
> 	(mips_thread_alive, mips_pid_to_str): New functions.
> 	(_initialize_remote_mips): Initialize remote_mips_ptid.  Initialize
> 	to_thread_alive and to_pid_to_str operations.

Looks almost good to me, with comments below.

> 
> Index: remote-mips.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote-mips.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 remote-mips.c
> --- remote-mips.c	26 Feb 2010 23:11:24 -0000	1.108
> +++ remote-mips.c	3 Mar 2010 22:57:34 -0000
> @@ -35,6 +35,7 @@
>  #include "regcache.h"
>  #include <ctype.h>
>  #include "mips-tdep.h"
> +#include "gdbthread.h"
>  \f
>  
>  /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
> @@ -440,6 +441,11 @@ struct lsi_error lsi_error_table[] =
>     of warnings returned by PMON when hardware breakpoints are used.  */
>  static int monitor_warnings;
>  
> +/* This is the ptid we use while we're connected to the remote.  Its
> +   value is arbitrary, as the remote-mips target doesn't have a notion of
> +   processes or threads, but we need something non-null to place in
> +   inferior_ptid.  */
> +static ptid_t remote_mips_ptid;
>  
>  static void
>  close_ports (void)
> @@ -483,7 +489,8 @@ mips_error (char *string,...)
>    close_ports ();
>  
>    printf_unfiltered ("Ending remote MIPS debugging.\n");
> -  target_mourn_inferior ();
> +  if (!ptid_equal (inferior_ptid, null_ptid))
> +    target_mourn_inferior ();
>  
>    deprecated_throw_reason (RETURN_ERROR);
>  }
> @@ -1468,6 +1475,7 @@ common_open (struct target_ops *ops, cha
>    char *remote_name = 0;
>    char *local_name = 0;
>    char **argv;
> +  struct inferior *inf;
>  
>    if (name == 0)
>      error (
> @@ -1563,7 +1571,11 @@ device is attached to the target board (
>    /* Switch to using remote target now.  */
>    push_target (ops);
>  
> -  /* FIXME: Should we call start_remote here?  */
> +  inferior_ptid = remote_mips_ptid;
> +  inf = add_inferior_silent (ptid_get_pid (inferior_ptid));

Sorry, I missed this before.  Been staring at too many different
GDB versions in this regard.  :-)  You should be using
inferior_appeared instead of add_inferior_silent, and
exit_inferior_silent instead of delete_inferior_silent.  Ever
since GDB gained multi-exec support, there should always
be an inferior in GDBs inferior list, even after the "process"
exits.  With the current patch, after opening a connection, 
you'll be seeing two inferiors in "info inferiors", but you
should only see one, and you should still see it after
the connection having been closed.  This also gets
rid of the need to much with the aspaces below.

> +  inf->aspace = current_program_space->aspace;
> +  inf->pspace = current_program_space;
> +  add_thread_silent (inferior_ptid);
>  
>    /* Try to figure out the processor model if possible.  */
>    deprecated_mips_set_processor_regs_hack ();
> @@ -1639,6 +1651,10 @@ mips_close (int quitting)
>  
>        close_ports ();
>      }
> +
> +  generic_mourn_inferior ();
> +  delete_thread_silent (remote_mips_ptid);
> +  delete_inferior_silent (ptid_get_pid (remote_mips_ptid));

Note that generic_mourn_inferior already
calls exit_inferior and that already deletes all the
inferior's threads, so, if the output exit_inferior gives
is okay for you, _and_ inferior_ptid is never null_ptid
when you get here, you can simply get rid of the
delete_thread_silent and delete_inferior_silent calls.
Otherwise, you could also call discard_all_inferiors
instead (see remote.c:remote_close) -- despite the
name, which is stale, it doesn't really discard
the inferiors, it calls exit_inferior on them.


>  }
>  
>  /* Detach from the remote board.  */
> @@ -2140,7 +2156,10 @@ static void
>  mips_kill (struct target_ops *ops)
>  {
>    if (!mips_wait_flag)
> -    return;
> +    {
> +      target_mourn_inferior ();
> +      return;
> +    }

Hmmm, so, "kill" will just disconnect without
actually killing the target.  But, when mips_wait_flag
is set, mips_kill will just try to interrupt
the target.  Actually, I can't see how to get here
with mips_wait_flag set.  Bah.  This code is
sanatized (I think) by that other patch I pointed you at.
Anyway, if there's no way to actually kill the
target, this is fine.  But you do have one other
option:  that is, to forbid "kill" (make mips_kill
throw an error), and set attach_flag in the inferior,
so that gdb offers to "detach" it instead of "kill"ing
it on "(gdb) quit".  Users would have to "detach" instead.

>  
>    interrupt_count++;
>  
> @@ -2173,6 +2192,8 @@ Give up (and stop debugging it)? ")))
>  
>    serial_send_break (mips_desc);
>  
> +  target_mourn_inferior ();
> +
>  #if 0
>    if (mips_is_open)
>      {
> @@ -2210,19 +2231,17 @@ Can't pass arguments to remote MIPS boar
>  
>    init_wait_for_inferior ();
>  
> -  /* FIXME: Should we set inferior_ptid here?  */
> -
>    regcache_write_pc (get_current_regcache (), entry_pt);
>  }
>  
> -/* Clean up after a process.  Actually nothing to do.  */
> +/* Clean up after a process. The bulk of the work is done in mips_close(),
> +   which is called when unpushing the target.  */
>  
>  static void
>  mips_mourn_inferior (struct target_ops *ops)
>  {
>    if (current_ops != NULL)
>      unpush_target (current_ops);
> -  generic_mourn_inferior ();
>  }
>  \f
>  /* We can write a breakpoint and read the shadow contents in one
> @@ -3296,18 +3315,36 @@ mips_load (char *file, int from_tty)
>      }
>    if (exec_bfd)
>      regcache_write_pc (regcache, bfd_get_start_address (exec_bfd));
> +}
>  
> -  inferior_ptid = null_ptid;	/* No process now */
> -
> -/* This is necessary because many things were based on the PC at the time that
> -   we attached to the monitor, which is no longer valid now that we have loaded
> -   new code (and just changed the PC).  Another way to do this might be to call
> -   normal_stop, except that the stack may not be valid, and things would get
> -   horribly confused... */
> +/* Check to see if a thread is still alive.  */
> + 
> +static int
> +mips_thread_alive (struct target_ops *ops, ptid_t ptid)
> +{
> +  if (ptid_equal (ptid, remote_mips_ptid))
> +    /* The monitor's task is always alive.  */
> +    return 1;
>  
> -  clear_symtab_users ();
> +  return 0;
>  }
>  
> +/* Convert a thread ID to a string.  Returns the string in a static
> +   buffer.  */
> +
> +static char *
> +mips_pid_to_str (struct target_ops *ops, ptid_t ptid)
> +{
> +  static char buf[64];
> +
> +  if (ptid_equal (ptid, remote_mips_ptid))
> +    {
> +      xsnprintf (buf, sizeof buf, "Thread <main>");
> +      return buf;
> +    }
> +
> +  return normal_pid_to_str (ptid);
> +}
>  
>  /* Pass the command argument as a packet to PMON verbatim.  */
>  
> @@ -3351,6 +3388,8 @@ _initialize_remote_mips (void)
>    mips_ops.to_load = mips_load;
>    mips_ops.to_create_inferior = mips_create_inferior;
>    mips_ops.to_mourn_inferior = mips_mourn_inferior;
> +  mips_ops.to_thread_alive = mips_thread_alive;
> +  mips_ops.to_pid_to_str = mips_pid_to_str;
>    mips_ops.to_log_command = serial_log_command;
>    mips_ops.to_stratum = process_stratum;
>    mips_ops.to_has_all_memory = default_child_has_all_memory;
> @@ -3458,4 +3497,5 @@ Use \"on\" to enable the masking and \"o
>  			   NULL,
>  			   NULL, /* FIXME: i18n: */
>  			   &setlist, &showlist);
> +  remote_mips_ptid = ptid_build (42000, 0, 42000);
>  }
> 
> 


-- 
Pedro Alves

Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Kevin Buettner]

Hi Pedro,

Thanks again for your review of these patches.  See my replies below...

On Thu, 4 Mar 2010 00:33:24 +0000
Pedro Alves <pedro@codesourcery.com> wrote:

> > -  /* FIXME: Should we call start_remote here?  */
> > +  inferior_ptid = remote_mips_ptid;
> > +  inf = add_inferior_silent (ptid_get_pid (inferior_ptid));
> 
> Sorry, I missed this before.  Been staring at too many different
> GDB versions in this regard.  :-)  You should be using
> inferior_appeared instead of add_inferior_silent, and
> exit_inferior_silent instead of delete_inferior_silent.  Ever
> since GDB gained multi-exec support, there should always
> be an inferior in GDBs inferior list, even after the "process"
> exits.  With the current patch, after opening a connection, 
> you'll be seeing two inferiors in "info inferiors", but you
> should only see one, and you should still see it after
> the connection having been closed.  This also gets
> rid of the need to much with the aspaces below.

Okay, I think I've fixed this now.  I tried "info inferiors" with my
patch from yesterday and I actually saw four inferiors on the list. (!)
Don't know why though.  With the current patch, below, I only see one.

> > +  generic_mourn_inferior ();
> > +  delete_thread_silent (remote_mips_ptid);
> > +  delete_inferior_silent (ptid_get_pid (remote_mips_ptid));
> 
> Note that generic_mourn_inferior already
> calls exit_inferior and that already deletes all the
> inferior's threads, so, if the output exit_inferior gives
> is okay for you, _and_ inferior_ptid is never null_ptid
> when you get here, you can simply get rid of the
> delete_thread_silent and delete_inferior_silent calls.
> Otherwise, you could also call discard_all_inferiors
> instead (see remote.c:remote_close) -- despite the
> name, which is stale, it doesn't really discard
> the inferiors, it calls exit_inferior on them.

I've fixed this too.

> >  mips_kill (struct target_ops *ops)
> >  {
> >    if (!mips_wait_flag)
> > -    return;
> > +    {
> > +      target_mourn_inferior ();
> > +      return;
> > +    }
> 
> Hmmm, so, "kill" will just disconnect without
> actually killing the target.  But, when mips_wait_flag
> is set, mips_kill will just try to interrupt
> the target.  Actually, I can't see how to get here
> with mips_wait_flag set.  Bah.  This code is
> sanatized (I think) by that other patch I pointed you at.
> Anyway, if there's no way to actually kill the
> target, this is fine.  But you do have one other
> option:  that is, to forbid "kill" (make mips_kill
> throw an error), and set attach_flag in the inferior,
> so that gdb offers to "detach" it instead of "kill"ing
> it on "(gdb) quit".  Users would have to "detach" instead.

Long term, I'm not sure what to do about this.  Short term, I'd like
to leave it as shown in the patch below - which is the same as the
earlier patch that you've looked at.

I tried setting attach_flag in the inferior and making mips_kill throw
an error, but that messes up gdb.base/opaque.exp.  Here's the relevant
bit from the log file:

(gdb) PASS: gdb.base/opaque.exp: ptype on opaque struct tagname (statically)
dir
Reinitialize source path to empty? (y or n) y
Source directories searched: $cdir:$cwd
(gdb) dir /ironwood1/sourceware-remote-mips/mips64vrel-elf/bld32/../../src/gdb/testsuite/gdb.base
Source directories searched: /ironwood1/sourceware-remote-mips/mips64vrel-elf/bld32/../../src/gdb/testsuite/gdb.base:$cdir:$cwd
(gdb) kill
Kill the program being debugged? (y or n) y
Can't kill this target.
Use `detach' to disconnect from the target board's monitor.
(gdb) file /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/gdb.base/opaque
A program is being debugged already.
Are you sure you want to change the file? (y or n) ERROR: couldn't load /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/gdb.base/opaque into /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/../../gdb/gdb (timed out).
delete breakpoints
Please answer y or n.
A program is being debugged already.
Are you sure you want to change the file? (y or n) ERROR: Delete all breakpoints in delete_breakpoints (timeout)
break main
Please answer y or n.
A program is being debugged already.
Are you sure you want to change the file? (y or n) UNRESOLVED: gdb.base/opaque.exp: setting breakpoint at main (timeout)
ERROR: cannot run to breakpoint at main

I do have an explanation for why mips_kill() is trying to interrupt
the target though.  When mips_wait_flag is set, the program is running.
When it's not set, the monitor is, presumably, at the monitor prompt,
waiting for a command.  That command could come from GDB or from user
interaction.  When we're sitting at that prompt, the program hasn't
exactly been killed, but it's probably about as close as we can get
unless we manage to reset the board somehow.

With regard to entering mips_kill() with mips_wait_flag being set...
I agree with you; I don't see how it can happen.  I'm still looking
at old code to see if there's a way that it somehow used to work, but
doesn't any longer.  So far, it appears that it's always been broken,
but I have some more investigation to do.

So... given that the current code tries to kill the target as much as
is reasonable, i.e. either do nothing when at the monitor prompt, or
interrupt the board to get to the monitor prompt when the board is running
(even though we can't get here when that happens), it seems to me that
this code isn't entirely unreasonable.

Below is my current patch.

	* remote-mips.c (gdbthread.h): Include.
	(remote_mips_ptid): Declare.
	(mips_error): Only mourn the inferior when inferior_ptid is non-null.
	(common_open): Set inferior_ptid, add it as an inferior, and
	as a thread too.  Delete FIXME comment regarding start_remote().
	(mips_close): Invoke generic_mourn_inferior().
	(mips_kill): Make sure that target_mourn_inferior is invoked.
	(mips_mourn_inferior): Don't invoke generic_mourn_inferior, as
	it's now invoked from mips_close().
	(mips_load): Don't null out inferior_ptid.  Don't call
	clear_symtab_users().
	(mips_thread_alive, mips_pid_to_str): New functions.
	(_initialize_remote_mips): Initialize remote_mips_ptid.  Initialize
	to_thread_alive and to_pid_to_str operations.

Index: remote-mips.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-mips.c,v
retrieving revision 1.108
diff -u -p -r1.108 remote-mips.c
--- remote-mips.c	26 Feb 2010 23:11:24 -0000	1.108
+++ remote-mips.c	4 Mar 2010 23:03:07 -0000
@@ -35,6 +35,7 @@
 #include "regcache.h"
 #include <ctype.h>
 #include "mips-tdep.h"
+#include "gdbthread.h"
 \f
 
 /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
@@ -440,6 +441,11 @@ struct lsi_error lsi_error_table[] =
    of warnings returned by PMON when hardware breakpoints are used.  */
 static int monitor_warnings;
 
+/* This is the ptid we use while we're connected to the remote.  Its
+   value is arbitrary, as the remote-mips target doesn't have a notion of
+   processes or threads, but we need something non-null to place in
+   inferior_ptid.  */
+static ptid_t remote_mips_ptid;
 
 static void
 close_ports (void)
@@ -483,7 +489,8 @@ mips_error (char *string,...)
   close_ports ();
 
   printf_unfiltered ("Ending remote MIPS debugging.\n");
-  target_mourn_inferior ();
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    target_mourn_inferior ();
 
   deprecated_throw_reason (RETURN_ERROR);
 }
@@ -1563,7 +1570,9 @@ device is attached to the target board (
   /* Switch to using remote target now.  */
   push_target (ops);
 
-  /* FIXME: Should we call start_remote here?  */
+  inferior_ptid = remote_mips_ptid;
+  inferior_appeared (current_inferior (), ptid_get_pid (inferior_ptid));
+  add_thread_silent (inferior_ptid);
 
   /* Try to figure out the processor model if possible.  */
   deprecated_mips_set_processor_regs_hack ();
@@ -1639,6 +1648,8 @@ mips_close (int quitting)
 
       close_ports ();
     }
+
+  generic_mourn_inferior ();
 }
 
 /* Detach from the remote board.  */
@@ -2140,7 +2151,10 @@ static void
 mips_kill (struct target_ops *ops)
 {
   if (!mips_wait_flag)
-    return;
+    {
+      target_mourn_inferior ();
+      return;
+    }
 
   interrupt_count++;
 
@@ -2173,6 +2187,8 @@ Give up (and stop debugging it)? ")))
 
   serial_send_break (mips_desc);
 
+  target_mourn_inferior ();
+
 #if 0
   if (mips_is_open)
     {
@@ -2210,19 +2226,17 @@ Can't pass arguments to remote MIPS boar
 
   init_wait_for_inferior ();
 
-  /* FIXME: Should we set inferior_ptid here?  */
-
   regcache_write_pc (get_current_regcache (), entry_pt);
 }
 
-/* Clean up after a process.  Actually nothing to do.  */
+/* Clean up after a process. The bulk of the work is done in mips_close(),
+   which is called when unpushing the target.  */
 
 static void
 mips_mourn_inferior (struct target_ops *ops)
 {
   if (current_ops != NULL)
     unpush_target (current_ops);
-  generic_mourn_inferior ();
 }
 \f
 /* We can write a breakpoint and read the shadow contents in one
@@ -3296,18 +3310,36 @@ mips_load (char *file, int from_tty)
     }
   if (exec_bfd)
     regcache_write_pc (regcache, bfd_get_start_address (exec_bfd));
+}
 
-  inferior_ptid = null_ptid;	/* No process now */
-
-/* This is necessary because many things were based on the PC at the time that
-   we attached to the monitor, which is no longer valid now that we have loaded
-   new code (and just changed the PC).  Another way to do this might be to call
-   normal_stop, except that the stack may not be valid, and things would get
-   horribly confused... */
+/* Check to see if a thread is still alive.  */
+ 
+static int
+mips_thread_alive (struct target_ops *ops, ptid_t ptid)
+{
+  if (ptid_equal (ptid, remote_mips_ptid))
+    /* The monitor's task is always alive.  */
+    return 1;
 
-  clear_symtab_users ();
+  return 0;
 }
 
+/* Convert a thread ID to a string.  Returns the string in a static
+   buffer.  */
+
+static char *
+mips_pid_to_str (struct target_ops *ops, ptid_t ptid)
+{
+  static char buf[64];
+
+  if (ptid_equal (ptid, remote_mips_ptid))
+    {
+      xsnprintf (buf, sizeof buf, "Thread <main>");
+      return buf;
+    }
+
+  return normal_pid_to_str (ptid);
+}
 
 /* Pass the command argument as a packet to PMON verbatim.  */
 
@@ -3351,6 +3383,8 @@ _initialize_remote_mips (void)
   mips_ops.to_load = mips_load;
   mips_ops.to_create_inferior = mips_create_inferior;
   mips_ops.to_mourn_inferior = mips_mourn_inferior;
+  mips_ops.to_thread_alive = mips_thread_alive;
+  mips_ops.to_pid_to_str = mips_pid_to_str;
   mips_ops.to_log_command = serial_log_command;
   mips_ops.to_stratum = process_stratum;
   mips_ops.to_has_all_memory = default_child_has_all_memory;
@@ -3458,4 +3492,5 @@ Use \"on\" to enable the masking and \"o
 			   NULL,
 			   NULL, /* FIXME: i18n: */
 			   &setlist, &showlist);
+  remote_mips_ptid = ptid_build (42000, 0, 42000);
 }

Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Pedro Alves]

On Thursday 04 March 2010 23:33:19, Kevin Buettner wrote:

> Long term, I'm not sure what to do about this.  Short term, I'd like
> to leave it as shown in the patch below - which is the same as the
> earlier patch that you've looked at.

That's fine.

> 
> I tried setting attach_flag in the inferior and making mips_kill throw
> an error, but that messes up gdb.base/opaque.exp.  Here's the relevant
> bit from the log file:
> 
> (gdb) PASS: gdb.base/opaque.exp: ptype on opaque struct tagname (statically)
> dir
> Reinitialize source path to empty? (y or n) y
> Source directories searched: $cdir:$cwd
> (gdb) dir /ironwood1/sourceware-remote-mips/mips64vrel-elf/bld32/../../src/gdb/testsuite/gdb.base
> Source directories searched: /ironwood1/sourceware-remote-mips/mips64vrel-elf/bld32/../../src/gdb/testsuite/gdb.base:$cdir:$cwd
> (gdb) kill
> Kill the program being debugged? (y or n) y
> Can't kill this target.
> Use `detach' to disconnect from the target board's monitor.
> (gdb) file /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/gdb.base/opaque
> A program is being debugged already.
> Are you sure you want to change the file? (y or n) ERROR: couldn't load /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/gdb.base/opaque into /mesquite2/sourceware-remote-mips/mips64vrel-elf/bld32/gdb/testsuite/../../gdb/gdb (timed out).
> delete breakpoints
> Please answer y or n.
> A program is being debugged already.
> Are you sure you want to change the file? (y or n) ERROR: Delete all breakpoints in delete_breakpoints (timeout)
> break main
> Please answer y or n.
> A program is being debugged already.
> Are you sure you want to change the file? (y or n) UNRESOLVED: gdb.base/opaque.exp: setting breakpoint at main (timeout)
> ERROR: cannot run to breakpoint at main
> 

I'd call it a test bug to be trying to kill on such
configuration, but then again, it's really strange that
there's no defined way to kill/reset the board.  But really,
I'm super fine with what you have, as long as it works for you.
I'm surprised to find there's still use for this target (and
I confess that it was on my TODO list to garbage collect it);
AFAIK, modern MIPS debugging nowadays either goes either
through the remote protocol, or through MDI (remote-mdi.c).

> Below is my current patch.
> 
> 	* remote-mips.c (gdbthread.h): Include.
> 	(remote_mips_ptid): Declare.
> 	(mips_error): Only mourn the inferior when inferior_ptid is non-null.
> 	(common_open): Set inferior_ptid, add it as an inferior, and
> 	as a thread too.  Delete FIXME comment regarding start_remote().
> 	(mips_close): Invoke generic_mourn_inferior().
> 	(mips_kill): Make sure that target_mourn_inferior is invoked.
> 	(mips_mourn_inferior): Don't invoke generic_mourn_inferior, as
> 	it's now invoked from mips_close().
> 	(mips_load): Don't null out inferior_ptid.  Don't call
> 	clear_symtab_users().
> 	(mips_thread_alive, mips_pid_to_str): New functions.
> 	(_initialize_remote_mips): Initialize remote_mips_ptid.  Initialize
> 	to_thread_alive and to_pid_to_str operations.

Looks good to me.  Thanks.

-- 
Pedro Alves

Re: [RFC] remote-mips.c: Fix bit rot associated with the inferior's state

[Kevin Buettner]

On Fri, 5 Mar 2010 15:53:46 +0000
Pedro Alves <pedro@codesourcery.com> wrote:

> > I tried setting attach_flag in the inferior and making mips_kill throw
> > an error, but that messes up gdb.base/opaque.exp.
[...]
> 
> I'd call it a test bug to be trying to kill on such
> configuration...

I think you might be right since gdb.base/opaque.exp is one of the few
tests for which this is a problem.  (It might even be the only test for
which this is a problem.)

> > 	* remote-mips.c (gdbthread.h): Include.
> > 	(remote_mips_ptid): Declare.
> > 	(mips_error): Only mourn the inferior when inferior_ptid is non-null.
> > 	(common_open): Set inferior_ptid, add it as an inferior, and
> > 	as a thread too.  Delete FIXME comment regarding start_remote().
> > 	(mips_close): Invoke generic_mourn_inferior().
> > 	(mips_kill): Make sure that target_mourn_inferior is invoked.
> > 	(mips_mourn_inferior): Don't invoke generic_mourn_inferior, as
> > 	it's now invoked from mips_close().
> > 	(mips_load): Don't null out inferior_ptid.  Don't call
> > 	clear_symtab_users().
> > 	(mips_thread_alive, mips_pid_to_str): New functions.
> > 	(_initialize_remote_mips): Initialize remote_mips_ptid.  Initialize
> > 	to_thread_alive and to_pid_to_str operations.
> 
> Looks good to me.  Thanks.

I've committed this patch.  Thanks again for your reviews.

Kevin