remote-mips.c, always a thread

remote-mips.c, always a thread

[Pedro Alves]

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

Hi,

This patch makes the remote mips targets (mips,pmon,ddb,lsi) register a
thread in GDB's thread list.

While going through the file, I noticed a couple of other issues:

- nowhere in the file was inferior_ptid set, but there
  were places where it was being set to null_ptid (you can
  see one such place in the patch).

- mips_load was clearing inferior_ptid, and calling clear_symtab_users.  I 
  notice that inferior_ptid is being cleared after a load command, but that
  is wrong, I believe.  See this change of Jim's to monitor.c, that removed
  this clearing from the monitor target:
   http://sourceware.org/ml/gdb/2001-09/msg00125.html

  I'm applying the exact same reasoning and change here.

- There's a FIXME in mips_create_inferior, wondering if it should set
  inferior_ptid there.  I believe the answer is negative.  When you get to
  mips_create_inferior, the target is already with execution -- you have it
  since target_open.  The "run" command against this target merelly sets
  the PC to the entry points, and proceeds.  The user could have also
  just "target mips ..." followed by "continue".  See Jim's email above, his
  very nice explanation makes it clearer.

This hasn't been tested at all, but, does it looks reasonable?

-- 
Pedro Alves

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

2008-08-18  Pedro Alves  <pedro@codesourcery.com>

	* remote-mips.c: Include "gdbthread.h".
	(remote_mips_ptid): New.
	(common_open): Set inferior_ptid to remote_mips_ptid and add it as
	a thread to GDB's thread list.
	(mips_close): Delete remote_mips_ptid from GDB's thread list.
	(mips_create_inferior): Remove FIXME note.
	(mips_load): Don't delete symtab users, or reset inferior_ptid.
	(_initialize_remote_mips): Initialize remote_mips_ptid.

---
 gdb/remote-mips.c |   27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Index: src/gdb/remote-mips.c
===================================================================
--- src.orig/gdb/remote-mips.c	2008-08-17 23:28:19.000000000 +0100
+++ src/gdb/remote-mips.c	2008-08-18 13:09:40.000000000 +0100
@@ -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
@@ -450,6 +451,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-sim target don't have a notion or
+   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)
@@ -1577,6 +1583,9 @@ device is attached to the target board (
   /* Try to figure out the processor model if possible.  */
   deprecated_mips_set_processor_regs_hack ();
 
+  inferior_ptid = remote_mips_ptid;
+  add_thread_silent (remote_mips_ptid);
+
   /* This is really the job of start_remote however, that makes an
      assumption that the target is about to print out a status message
      of some sort.  That doesn't happen here (in fact, it may not be
@@ -1648,6 +1657,8 @@ mips_close (int quitting)
 
       close_ports ();
     }
+
+  delete_thread_silent (remote_mips_ptid);
 }
 
 /* Detach from the remote board.  */
@@ -2208,8 +2219,6 @@ Can't pass arguments to remote MIPS boar
 
   init_wait_for_inferior ();
 
-  /* FIXME: Should we set inferior_ptid here?  */
-
   write_pc (entry_pt);
 }
 
@@ -3288,16 +3297,6 @@ mips_load (char *file, int from_tty)
     }
   if (exec_bfd)
     write_pc (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... */
-
-  clear_symtab_users ();
 }
 
 
@@ -3450,4 +3449,8 @@ Use \"on\" to enable the masking and \"o
 			   NULL,
 			   NULL, /* FIXME: i18n: */
 			   &setlist, &showlist);
+
+  /* Yes, 42000 is arbitrary.  The only sense out of it, is that it
+     isn't 0.  */
+  remote_mips_ptid = ptid_build (42000, 0, 42000);
 }

Re: remote-mips.c, always a thread

[Pedro Alves]

On Monday 18 August 2008 13:29:28, Pedro Alves wrote:

> - mips_load was clearing inferior_ptid, and calling clear_symtab_users.  I
>   notice that inferior_ptid is being cleared after a load command, but that
>   is wrong, I believe.  See this change of Jim's to monitor.c, that removed
>   this clearing from the monitor target:
>    http://sourceware.org/ml/gdb/2001-09/msg00125.html
>
>   I'm applying the exact same reasoning and change here.
>

Hmmm, I just noticed that mips_kill is being used to interrupting
the target ( '\03' / ^C / SIGINT ), instead of installing a SIGINT
handler like at least monitor.c, remote.c and remote-sim.c do.
This looks broken to me -- target_kill is definitelly not the
right target method for this (and neither is the "kill" command).

> This hasn't been tested at all, but, does it looks reasonable?

I forgot to add a new target_thread_alive function, otherwise,
info threads gets rid of the thread I just added.

-- 
Pedro Alves

[02/02] remote-mips.c, always a thread (take 2)

[Pedro Alves]

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

On Monday 18 August 2008 14:07:34, Pedro Alves wrote:

> I forgot to add a new target_thread_alive function, otherwise,
> info threads gets rid of the thread I just added.

And here's a new patch that fixes that.  Here are the same
notes I posted yesterday:

- nowhere in the file was inferior_ptid set, but there
  were places where it was being set to null_ptid (you can
  see one such place in the patch).

- mips_load was clearing inferior_ptid, and calling clear_symtab_users.  I 
  notice that inferior_ptid is being cleared after a load command, but that
  is wrong, I believe.  See this change of Jim's to monitor.c, that removed
  this clearing from the monitor target:
   http://sourceware.org/ml/gdb/2001-09/msg00125.html

  I'm applying the exact same reasoning and change here.

  If this patch goes in without the previous patch, issuing "run" and the
  user answering yes to "The program being debugged has been
  started already.  Start it from the beginning?" ends up calling target_kill
  which sends the remote a break.  With the previous patch installed,
  The user can use ctrl-c at the terminal to interrupt the remote, and
  target_kill will do nothing, just like monitor.c does (which seems to
  share some history with remote-mips.c).

- There's a FIXME in mips_create_inferior, wondering if it should set
  inferior_ptid there.  I believe the answer is negative.  When you get to
  mips_create_inferior, the target is already with execution -- you have it
  since target_open.  The "run" command against this target merelly sets
  the PC to the entry points, and proceeds.  The user could have also
  just "target mips ..." followed by "continue".  See Jim's email above, his
  very nice explanation makes it clearer.

I've checked that this at least compiles.  Keep in mind that there is no
current configuration using this file (which I had noticed before).

I would definitelly not opposse GCing this file instead.

-- 
Pedro Alves

[-- Attachment #2: 002-remote_mips_always_a_thread.diff --]
[-- Type: text/x-diff, Size: 4154 bytes --]

2008-08-19  Pedro Alves  <pedro@codesourcery.com>

	* remote-mips.c: Include "gdbthread.h".
	(remote_mips_ptid): New.
	(common_open): Set inferior_ptid to remote_mips_ptid and add it as
	a thread to GDB's thread list.
	(mips_close): Delete remote_mips_ptid from GDB's thread list.
	(mips_create_inferior): Remove FIXME note.
	(mips_load): Don't delete symtab users, or reset inferior_ptid.
	(_initialize_remote_mips): Initialize remote_mips_ptid.

---
 gdb/remote-mips.c |   52 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 9 deletions(-)

Index: src/gdb/remote-mips.c
===================================================================
--- src.orig/gdb/remote-mips.c	2008-08-19 20:04:11.000000000 +0100
+++ src/gdb/remote-mips.c	2008-08-19 20:04:40.000000000 +0100
@@ -36,6 +36,7 @@
 #include <ctype.h>
 #include "mips-tdep.h"
 #include <signal.h>
+#include "gdbthread.h"
 \f
 
 /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
@@ -457,6 +458,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-sim target don't have a notion or
+   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)
@@ -1584,6 +1590,9 @@ device is attached to the target board (
   /* Try to figure out the processor model if possible.  */
   deprecated_mips_set_processor_regs_hack ();
 
+  inferior_ptid = remote_mips_ptid;
+  add_thread_silent (remote_mips_ptid);
+
   /* This is really the job of start_remote however, that makes an
      assumption that the target is about to print out a status message
      of some sort.  That doesn't happen here (in fact, it may not be
@@ -1655,6 +1664,8 @@ mips_close (int quitting)
 
       close_ports ();
     }
+
+  delete_thread_silent (remote_mips_ptid);
 }
 
 /* Detach from the remote board.  */
@@ -2264,8 +2275,6 @@ Can't pass arguments to remote MIPS boar
 
   init_wait_for_inferior ();
 
-  /* FIXME: Should we set inferior_ptid here?  */
-
   write_pc (entry_pt);
 }
 
@@ -3343,18 +3352,37 @@ mips_load (char *file, int from_tty)
     }
   if (exec_bfd)
     write_pc (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.  */
 
-  clear_symtab_users ();
+static int
+mips_thread_alive (ptid_t ptid)
+{
+  if (ptid_equal (ptid, remote_mips_ptid))
+    /* The monitor's task is always alive.  */
+    return 1;
+
+  return 0;
 }
 
+/* Convert a thread ID to a string.  Returns the string in a static
+   buffer.  */
+
+static char *
+mips_pid_to_str (ptid_t ptid)
+{
+  static char buf[64];
+
+  if (ptid_equal (remote_mips_ptid, 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.  */
 
@@ -3399,6 +3427,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 = 1;
@@ -3506,4 +3536,8 @@ Use \"on\" to enable the masking and \"o
 			   NULL,
 			   NULL, /* FIXME: i18n: */
 			   &setlist, &showlist);
+
+  /* Yes, 42000 is arbitrary.  The only sense out of it, is that it
+     isn't 0.  */
+  remote_mips_ptid = ptid_build (42000, 0, 42000);
 }

[01/02] remote-mips.c, always a thread (take 2)

[Pedro Alves]

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

On Monday 18 August 2008 14:07:34, Pedro Alves wrote:
> On Monday 18 August 2008 13:29:28, Pedro Alves wrote:
> > - mips_load was clearing inferior_ptid, and calling clear_symtab_users. 
> > I notice that inferior_ptid is being cleared after a load command, but
> > that is wrong, I believe.  See this change of Jim's to monitor.c, that
> > removed this clearing from the monitor target:
> >    http://sourceware.org/ml/gdb/2001-09/msg00125.html
> >
> >   I'm applying the exact same reasoning and change here.
>
> Hmmm, I just noticed that mips_kill is being used to interrupting
> the target ( '\03' / ^C / SIGINT ), instead of installing a SIGINT
> handler like at least monitor.c, remote.c and remote-sim.c do.
> This looks broken to me -- target_kill is definitelly not the
> right target method for this (and neither is the "kill" command).

This patch makes the remote-mips target register a SIGINT handler
instead, like monitor.c does.

Of course, only after doing this, I noticed that that there's
no configuration currently that builds this file...

I've checked that this at least builds after tweaking configure.tgt
to include it in a --target=mips-elf build.  CS has a similar tweak
in its internal trees, but AFAIK we don't have customers
using this target either.

To make this build, I also needed to change regcache_set_valid_p to
regcache_invalidate.  regcache_set_valid_p isn't defined anywhere.

It seems it would supposed to go in with this:
http://sourceware.org/ml/gdb-patches/2007-12/msg00150.html

But only the remote-mips.c bits went in.

Anyway, as long as I've written it the patch here it is.

-- 
Pedro Alves

[-- Attachment #2: 001-remote_mips_kill.diff --]
[-- Type: text/x-diff, Size: 6272 bytes --]

2008-08-19  Pedro Alves  <pedro@codesourcery.com>

	* remote-mips.c (ofunc): New.
	(mips_wait_cleanup): New.
	(mips_wait): Set SIGINT so mips_interrupt.
	(mips_kill): Do nothing.
	(mips_stop): New, based on old mips_kill.
	(mips_interrupt, mips_interrupt_twice, mips_interrupt_query): New.
	(mips_load): Use regcache_invalidate instead of
	regcache_set_valid_p.
	(_initialize_remote_mips): Register mips_stop.

---
 gdb/remote-mips.c |  118 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 87 insertions(+), 31 deletions(-)

Index: src/gdb/remote-mips.c
===================================================================
--- src.orig/gdb/remote-mips.c	2008-08-19 19:58:54.000000000 +0100
+++ src/gdb/remote-mips.c	2008-08-19 20:04:11.000000000 +0100
@@ -35,6 +35,7 @@
 #include "regcache.h"
 #include <ctype.h>
 #include "mips-tdep.h"
+#include <signal.h>
 \f
 
 /* Breakpoint types.  Values 0, 1, and 2 must agree with the watch
@@ -148,6 +149,10 @@ static int mips_clear_breakpoint (CORE_A
 static int mips_common_breakpoint (int set, CORE_ADDR addr, int len,
 				   enum break_type type);
 
+static void mips_interrupt (int signo);
+static void mips_interrupt_twice (int signo);
+static void mips_interrupt_query (void);
+
 /* Forward declarations.  */
 extern struct target_ops mips_ops;
 extern struct target_ops pmon_ops;
@@ -385,6 +390,8 @@ static int mips_wait_flag = 0;
 /* If non-zero, monitor supports breakpoint commands. */
 static int monitor_supports_breakpoints = 0;
 
+static void (*ofunc) ();	/* Old SIGINT signal handler */
+
 /* Data cache header.  */
 
 #if 0				/* not used (yet?) */
@@ -1701,6 +1708,13 @@ mips_signal_from_protocol (int sig)
   return (enum target_signal) sig;
 }
 
+static void
+mips_wait_cleanup (void *arg)
+{
+  signal (SIGINT, ofunc);
+  mips_wait_flag = 0;
+}
+
 /* Wait until the remote stops, and return a wait status.  */
 
 static ptid_t
@@ -1713,6 +1727,7 @@ mips_wait (ptid_t ptid, struct target_wa
   char flags[20];
   int nfields;
   int i;
+  struct cleanup *old_chain;
 
   interrupt_count = 0;
   hit_watchpoint = 0;
@@ -1727,10 +1742,17 @@ mips_wait (ptid_t ptid, struct target_wa
       return inferior_ptid;
     }
 
+  old_chain = make_cleanup (mips_wait_cleanup, NULL);
+
   /* No timeout; we sit here as long as the program continues to execute.  */
   mips_wait_flag = 1;
+  ofunc = (void (*)()) signal (SIGINT, mips_interrupt);
   rstatus = mips_request ('\000', 0, 0, &err, -1, buff);
+  signal (SIGINT, ofunc);
   mips_wait_flag = 0;
+
+  discard_cleanups (old_chain);
+
   if (err)
     mips_error ("Remote failure: %s", safe_strerror (errno));
 
@@ -2130,43 +2152,24 @@ mips_files_info (struct target_ops *igno
   printf_unfiltered ("Debugging a MIPS board over a serial line.\n");
 }
 
-/* Kill the process running on the board.  This will actually only
+static void
+mips_kill (void)
+{
+  /* ignore attempts to kill target system */
+  return;
+}
+
+/* Interrupt the process running on the board.  This will actually only
    work if we are doing remote debugging over the console input.  I
    think that if IDT/sim had the remote debug interrupt enabled on the
    right port, we could interrupt the process with a break signal.  */
 
 static void
-mips_kill (void)
+mips_stop (ptid_t ptid)
 {
   if (!mips_wait_flag)
     return;
 
-  interrupt_count++;
-
-  if (interrupt_count >= 2)
-    {
-      interrupt_count = 0;
-
-      target_terminal_ours ();
-
-      if (query ("Interrupted while waiting for the program.\n\
-Give up (and stop debugging it)? "))
-	{
-	  /* Clean up in such a way that mips_close won't try to talk to the
-	     board (it almost surely won't work since we weren't able to talk to
-	     it).  */
-	  mips_wait_flag = 0;
-	  close_ports ();
-
-	  printf_unfiltered ("Ending remote MIPS debugging.\n");
-	  target_mourn_inferior ();
-
-	  deprecated_throw_reason (RETURN_QUIT);
-	}
-
-      target_terminal_inferior ();
-    }
-
   if (remote_debug > 0)
     printf_unfiltered ("Sending break\n");
 
@@ -2186,6 +2189,59 @@ Give up (and stop debugging it)? "))
 #endif
 }
 
+/* Send ^C to target to halt it.  Target will respond, and send us a
+   packet.  */
+
+static void
+mips_interrupt (int signo)
+{
+  /* If this doesn't work, try more severe steps.  */
+  signal (signo, mips_interrupt_twice);
+
+  if (remote_debug)
+    fprintf_unfiltered (gdb_stdlog, "mips_interrupt called\n");
+
+  target_stop (inferior_ptid);
+}
+
+/* The user typed ^C twice.  */
+
+static void
+mips_interrupt_twice (int signo)
+{
+  signal (signo, ofunc);
+
+  mips_interrupt_query ();
+
+  signal (signo, mips_interrupt);
+}
+
+/* Ask the user what to do when an interrupt is received.  */
+
+static void
+mips_interrupt_query (void)
+{
+  target_terminal_ours ();
+
+  if (query ("Interrupted while waiting for the program.\n\
+Give up (and stop debugging it)? "))
+    {
+      /* Clean up in such a way that mips_close won't try to talk to the
+	 board (it almost surely won't work since we weren't able to talk to
+	 it).  */
+      mips_wait_flag = 0;
+      close_ports ();
+
+      printf_unfiltered ("Ending remote MIPS debugging.\n");
+      target_mourn_inferior ();
+
+      deprecated_throw_reason (RETURN_QUIT);
+    }
+
+  target_terminal_inferior ();
+}
+
+
 /* Start running on the target board.  */
 
 static void
@@ -3282,9 +3338,8 @@ mips_load (char *file, int from_tty)
          to a different value than GDB thinks it has. The following ensures
          that the write_pc() WILL update the PC value: */
       struct regcache *regcache = get_current_regcache ();
-      regcache_set_valid_p (regcache,
-			    gdbarch_pc_regnum (get_regcache_arch (regcache)),
-					       0);
+      regcache_invalidate (regcache,
+			   gdbarch_pc_regnum (get_regcache_arch (regcache)));
     }
   if (exec_bfd)
     write_pc (bfd_get_start_address (exec_bfd));
@@ -3340,6 +3395,7 @@ _initialize_remote_mips (void)
   mips_ops.to_stopped_by_watchpoint = mips_stopped_by_watchpoint;
   mips_ops.to_can_use_hw_breakpoint = mips_can_use_watchpoint;
   mips_ops.to_kill = mips_kill;
+  mips_ops.to_stop = mips_stop;
   mips_ops.to_load = mips_load;
   mips_ops.to_create_inferior = mips_create_inferior;
   mips_ops.to_mourn_inferior = mips_mourn_inferior;