[RFC] remote-mips.c: Add support for NEC rockhopper boards

[RFC] remote-mips.c: Add support for NEC rockhopper boards

[Kevin Buettner]

The patch below was written back in the fall of 2002 by Richard
Sandiford and Martin Hunt.  It adds support for the NEC rockhopper
boards (which use pmon variant) to remote-mips.c.  Corinna Vinschen
and I have tweaked it every so often over the years to keep it going
as GDB changed.

Comments?

	From Richard Sandiford, Martin M. Hunt, Corinna Vinschen,
	and Kevin Buettner:

	* remote-mips.c (rockhopper_ops): New target_ops struct.
	(MON_ROCKHOPPER): New mips_monitor_type.
	(read_hex_value): New function.
	(mips_request): Send 8-byte values with a 'T' packet.  Read the
	packet argument as a string and use read_hex_value to parse it.  
	(mips_exit_debug): Wait for response when using MON_ROCKHOPPER.
	(rockhopper_open): New function.
	(mips_wait): Read the PC, FP and SP fields as strings.  Use
	read_hex_value to parse them and mips_set_register to commit them.
	(mips_set_register): New function.
	(mips_fetch_registers): Do not cast register value to "unsigned"
	when reading a MON_ROCKHOPPER 't' packet.  Use mips_set_register.
	(mips_store_registers): Use a 'T' packet to set registers when
	using MON_ROCKHOPPER.
	(pmon_end_download): Don't run initEther if using MON_ROCKHOPPER
	and expect the total to be printed before the entry address.
	(_initialize_remote_mips): Initialize and add rockhopper_ops.

--- ../../../sourceware-remote-mips-2/src/gdb/remote-mips.c	2010-03-05 14:52:45.000000000 -0700
+++ remote-mips.c	2010-03-05 14:47:39.000000000 -0700
@@ -89,6 +89,8 @@ static void mips_detach (struct target_o
 
 static int mips_map_regno (struct gdbarch *, int);
 
+static void mips_set_register (int regno, ULONGEST value);
+
 static void mips_prepare_to_store (struct regcache *regcache);
 
 static int mips_fetch_word (CORE_ADDR addr, unsigned int *valp);
@@ -143,6 +145,7 @@ static int mips_common_breakpoint (int s
 extern struct target_ops mips_ops;
 extern struct target_ops pmon_ops;
 extern struct target_ops ddb_ops;
+extern struct target_ops rockhopper_ops;
 \f/* *INDENT-OFF* */
 /* The MIPS remote debugging interface is built on top of a simple
    packet protocol.  Each packet is organized as follows:
@@ -288,7 +291,7 @@ extern struct target_ops ddb_ops;
    These are initialized with code in _initialize_remote_mips instead
    of static initializers, to make it easier to extend the target_ops
    vector later.  */
-struct target_ops mips_ops, pmon_ops, ddb_ops, lsi_ops;
+struct target_ops mips_ops, pmon_ops, ddb_ops, rockhopper_ops, lsi_ops;
 
 enum mips_monitor_type
   {
@@ -298,6 +301,7 @@ enum mips_monitor_type
     MON_PMON,			/* 3.0.83 [COGENT,EB,FP,NET] Algorithmics Ltd. Nov  9 1995 17:19:50 */
     MON_DDB,			/* 2.7.473 [DDBVR4300,EL,FP,NET] Risq Modular Systems,  Thu Jun 6 09:28:40 PDT 1996 */
     MON_LSI,			/* 4.3.12 [EB,FP], LSI LOGIC Corp. Tue Feb 25 13:22:14 1997 */
+    MON_ROCKHOPPER,
     /* Last and unused value, for sizing vectors, etc. */
     MON_LAST
   };
@@ -527,6 +531,33 @@ fputs_readable (const char *string, stru
 }
 
 
+/* Read P as a hex value.  Return true if every character made sense,
+   storing the result in *RESULT.  Leave *RESULT unchanged otherwise.  */
+
+static int
+read_hex_value (const char *p, ULONGEST *result)
+{
+  ULONGEST retval;
+
+  retval = 0;
+  while (*p != 0)
+    {
+      retval <<= 4;
+      if (*p >= '0' && *p <= '9')
+	retval |= *p - '0';
+      else if (*p >= 'A' && *p <= 'F')
+	retval |= *p - 'A' + 10;
+      else if (*p >= 'a' && *p <= 'f')
+	retval |= *p - 'a' + 10;
+      else
+	return 0;
+      p++;
+    }
+  *result = retval;
+  return 1;
+}
+
+
 /* Wait until STRING shows up in mips_desc.  Returns 1 if successful, else 0 if
    timed out.  TIMEOUT specifies timeout value in seconds.
  */
@@ -1196,11 +1227,12 @@ mips_request (int cmd,
 {
   int addr_size = gdbarch_addr_bit (target_gdbarch) / 8;
   char myBuff[DATA_MAXLEN + 1];
+  char response_string[17];
   int len;
   int rpid;
   char rcmd;
   int rerrflg;
-  unsigned long rresponse;
+  ULONGEST rresponse;
 
   if (buff == (char *) NULL)
     buff = myBuff;
@@ -1210,8 +1242,15 @@ mips_request (int cmd,
       if (mips_need_reply)
 	internal_error (__FILE__, __LINE__,
 			_("mips_request: Trying to send command before reply"));
-      sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
-	       phex_nz (addr, addr_size), phex_nz (data, addr_size));
+      /* 'T' sets a register to a 64-bit value, so make sure we use
+	 the right conversion function.  */
+      if (cmd == 'T')
+	sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
+		 phex_nz (addr, addr_size), phex_nz (data, 8));
+      else
+	sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
+	         phex_nz (addr, addr_size), phex_nz (data, addr_size));
+
       mips_send_packet (buff, 1);
       mips_need_reply = 1;
     }
@@ -1228,8 +1267,9 @@ mips_request (int cmd,
   len = mips_receive_packet (buff, 1, timeout);
   buff[len] = '\0';
 
-  if (sscanf (buff, "0x%x %c 0x%x 0x%lx",
-	      &rpid, &rcmd, &rerrflg, &rresponse) != 4
+  if (sscanf (buff, "0x%x %c 0x%x 0x%16s",
+	      &rpid, &rcmd, &rerrflg, response_string) != 4
+      || !read_hex_value (response_string, &rresponse)
       || (cmd != '\0' && rcmd != cmd))
     mips_error ("Bad response from remote board");
 
@@ -1311,7 +1351,7 @@ mips_exit_debug (void)
 
   mips_exiting = 1;
 
-  if (mips_monitor != MON_IDT)
+  if (mips_monitor != MON_IDT && mips_monitor != MON_ROCKHOPPER)
     {
       /* The DDB (NEC) and MiniRISC (LSI) versions of PMON exit immediately,
          so we do not get a reply to this command: */
@@ -1625,6 +1665,12 @@ ddb_open (char *name, int from_tty)
 }
 
 static void
+rockhopper_open (char *name, int from_tty)
+{
+  common_open (&rockhopper_ops, name, from_tty, MON_ROCKHOPPER, "NEC01>");
+}
+
+static void
 lsi_open (char *name, int from_tty)
 {
   int i;
@@ -1704,6 +1750,32 @@ mips_signal_from_protocol (int sig)
   return (enum target_signal) sig;
 }
 
+static void
+mips_set_register (int regno, ULONGEST value)
+{
+  char buf[MAX_REGISTER_SIZE];
+  struct regcache *regcache = get_current_regcache ();
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+
+  /* We got the number the register holds, but gdb expects to see a
+     value in the target byte ordering.  */
+
+  if (mips_monitor != MON_ROCKHOPPER
+      && (regno == mips_regnum (gdbarch)->pc || regno < 32))
+    /* Some 64-bit boards have monitors that only send the bottom 32 bits.
+       In such cases we can only really debug 32-bit code properly so,
+       when reading a GPR or the PC, assume that the full 64-bit
+       value is the sign extension of the lower 32 bits.  */
+    store_signed_integer (buf, register_size (gdbarch, regno), byte_order,
+                          value);
+  else
+    store_unsigned_integer (buf, register_size (gdbarch, regno), byte_order,
+                            value);
+
+  regcache_raw_supply (regcache, regno, buf);
+}
+
 /* Wait until the remote stops, and return a wait status.  */
 
 static ptid_t
@@ -1713,8 +1785,8 @@ mips_wait (struct target_ops *ops,
   int rstatus;
   int err;
   char buff[DATA_MAXLEN];
-  int rpc, rfp, rsp;
-  char flags[20];
+  ULONGEST rpc, rfp, rsp;
+  char pc_string[17], fp_string[17], sp_string[17], flags[20];
   int nfields;
   int i;
 
@@ -1754,35 +1826,19 @@ mips_wait (struct target_ops *ops,
 
   /* See if we got back extended status.  If so, pick out the pc, fp, sp, etc... */
 
-  nfields = sscanf (buff, "0x%*x %*c 0x%*x 0x%*x 0x%x 0x%x 0x%x 0x%*x %s",
-		    &rpc, &rfp, &rsp, flags);
-  if (nfields >= 3)
+  nfields = sscanf (buff, "0x%*x %*c 0x%*x 0x%*x 0x%16s 0x%16s 0x%16s 0x%*x %s",
+		    pc_string, fp_string, sp_string, flags);
+  if (nfields >= 3
+      && read_hex_value (pc_string, &rpc)
+      && read_hex_value (fp_string, &rfp)
+      && read_hex_value (sp_string, &rsp))
     {
       struct regcache *regcache = get_current_regcache ();
       struct gdbarch *gdbarch = get_regcache_arch (regcache);
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      char buf[MAX_REGISTER_SIZE];
 
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_pc_regnum (gdbarch)),
-	 byte_order, rpc);
-      regcache_raw_supply (regcache, gdbarch_pc_regnum (gdbarch), buf);
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_pc_regnum (gdbarch)),
-	 byte_order, rfp);
-      regcache_raw_supply (regcache, 30, buf);	/* This register they are avoiding and so it is unnamed */
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_sp_regnum (gdbarch)),
-	 byte_order, rsp);
-      regcache_raw_supply (regcache, gdbarch_sp_regnum (gdbarch), buf);
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_deprecated_fp_regnum (gdbarch)),
-	 byte_order, 0);
-      regcache_raw_supply (regcache,
-			   gdbarch_deprecated_fp_regnum (gdbarch), buf);
+      mips_set_register (gdbarch_pc_regnum (gdbarch), rpc);
+      mips_set_register (30, rfp);
+      mips_set_register (gdbarch_sp_regnum (gdbarch), rsp);
 
       if (nfields == 9)
 	{
@@ -1942,6 +1998,9 @@ mips_fetch_registers (struct target_ops 
 	  if (mips_monitor == MON_DDB)
 	    val = (unsigned) mips_request ('t', pmon_reg, 0,
 					   &err, mips_receive_wait, NULL);
+	  else if (mips_monitor == MON_ROCKHOPPER)
+	    val = mips_request ('t', pmon_reg, 0,
+				&err, mips_receive_wait, NULL);
 	  else
 	    val = mips_request ('r', pmon_reg, 0,
 				&err, mips_receive_wait, NULL);
@@ -1951,15 +2010,7 @@ mips_fetch_registers (struct target_ops 
 	}
     }
 
-  {
-    char buf[MAX_REGISTER_SIZE];
-
-    /* We got the number the register holds, but gdb expects to see a
-       value in the target byte ordering.  */
-    store_unsigned_integer (buf, register_size (gdbarch, regno),
-			    byte_order, val);
-    regcache_raw_supply (regcache, regno, buf);
-  }
+  mips_set_register (regno, val);
 }
 
 /* Prepare to store registers.  The MIPS protocol can store individual
@@ -1988,7 +2039,9 @@ mips_store_registers (struct target_ops 
     }
 
   regcache_cooked_read_unsigned (regcache, regno, &val);
-  mips_request ('R', mips_map_regno (gdbarch, regno), val,
+  mips_request (mips_monitor == MON_ROCKHOPPER ? 'T' : 'R',
+  		mips_map_regno (gdbarch, regno),
+		val,
 		&err, mips_receive_wait, NULL);
   if (err)
     mips_error ("Can't write register %d: %s", regno, safe_strerror (errno));
@@ -2374,7 +2427,7 @@ static int
 mips_check_lsi_error (CORE_ADDR addr, int rerrflg)
 {
   struct lsi_error *err;
-  char *saddr = paddress (target_gdbarch, addr);
+  const char *saddr = paddress (target_gdbarch, addr);
 
   if (rerrflg == 0)		/* no error */
     return 0;
@@ -3091,7 +3144,8 @@ pmon_end_download (int final, int bintot
 	chmod (tftp_localname, stbuf.st_mode | S_IROTH);
 
       /* Must reinitialize the board to prevent PMON from crashing.  */
-      mips_send_command ("initEther\r", -1);
+      if (mips_monitor != MON_ROCKHOPPER)
+	mips_send_command ("initEther\r", -1);
 
       /* Send the load command.  */
       cmd = xmalloc (strlen (load_cmd_prefix) + strlen (tftp_name) + 2);
@@ -3119,6 +3173,11 @@ pmon_end_download (int final, int bintot
       if (!pmon_check_total (bintotal))
 	return;
       break;
+    case MON_ROCKHOPPER:
+      if (!pmon_check_total (bintotal))
+	return;
+      pmon_check_entry_address ("Entry Address  = ", final);
+      break;
     default:
       pmon_check_entry_address ("Entry Address  = ", final);
       pmon_check_ack ("termination");
@@ -3406,7 +3465,7 @@ _initialize_remote_mips (void)
   mips_ops.to_magic = OPS_MAGIC;
 
   /* Copy the common fields to all four target vectors.  */
-  pmon_ops = ddb_ops = lsi_ops = mips_ops;
+  rockhopper_ops = pmon_ops = ddb_ops = lsi_ops = mips_ops;
 
   /* Initialize target-specific fields in the target vectors.  */
   mips_ops.to_shortname = "mips";
@@ -3436,6 +3495,11 @@ of the TFTP temporary file, if it differ
   ddb_ops.to_open = ddb_open;
   ddb_ops.to_wait = mips_wait;
 
+  rockhopper_ops.to_shortname = "rockhopper";
+  rockhopper_ops.to_doc = ddb_ops.to_doc;
+  rockhopper_ops.to_open = rockhopper_open;
+  rockhopper_ops.to_wait = mips_wait;
+
   lsi_ops.to_shortname = "lsi";
   lsi_ops.to_doc = pmon_ops.to_doc;
   lsi_ops.to_open = lsi_open;
@@ -3446,6 +3510,7 @@ of the TFTP temporary file, if it differ
   add_target (&pmon_ops);
   add_target (&ddb_ops);
   add_target (&lsi_ops);
+  add_target (&rockhopper_ops);
 
   add_setshow_zinteger_cmd ("timeout", no_class, &mips_receive_wait, _("\
 Set timeout in seconds for remote MIPS serial I/O."), _("\

Re: [RFC] remote-mips.c: Add support for NEC rockhopper boards

[Joel Brobecker]

> 	* remote-mips.c (rockhopper_ops): New target_ops struct.
> 	(MON_ROCKHOPPER): New mips_monitor_type.
> 	(read_hex_value): New function.
> 	(mips_request): Send 8-byte values with a 'T' packet.  Read the
> 	packet argument as a string and use read_hex_value to parse it.  
> 	(mips_exit_debug): Wait for response when using MON_ROCKHOPPER.
> 	(rockhopper_open): New function.
> 	(mips_wait): Read the PC, FP and SP fields as strings.  Use
> 	read_hex_value to parse them and mips_set_register to commit them.
> 	(mips_set_register): New function.
> 	(mips_fetch_registers): Do not cast register value to "unsigned"
> 	when reading a MON_ROCKHOPPER 't' packet.  Use mips_set_register.
> 	(mips_store_registers): Use a 'T' packet to set registers when
> 	using MON_ROCKHOPPER.
> 	(pmon_end_download): Don't run initEther if using MON_ROCKHOPPER
> 	and expect the total to be printed before the entry address.
> 	(_initialize_remote_mips): Initialize and add rockhopper_ops.

I have a few questions on a few things I don't understand.

> @@ -1196,11 +1227,12 @@ mips_request (int cmd,
>  {
>    int addr_size = gdbarch_addr_bit (target_gdbarch) / 8;
>    char myBuff[DATA_MAXLEN + 1];
> +  char response_string[17];
[...]
> -  if (sscanf (buff, "0x%x %c 0x%x 0x%lx",
> -	      &rpid, &rcmd, &rerrflg, &rresponse) != 4
> +  if (sscanf (buff, "0x%x %c 0x%x 0x%16s",
> +	      &rpid, &rcmd, &rerrflg, response_string) != 4
> +      || !read_hex_value (response_string, &rresponse)

I don't understand the need to change the sscanf format for the last
item from %lx to %16s. It definitely makes the code more complex...

> +static void
> +mips_set_register (int regno, ULONGEST value)
[...]
> +  if (mips_monitor != MON_ROCKHOPPER
> +      && (regno == mips_regnum (gdbarch)->pc || regno < 32))
> +    /* Some 64-bit boards have monitors that only send the bottom 32 bits.
> +       In such cases we can only really debug 32-bit code properly so,
> +       when reading a GPR or the PC, assume that the full 64-bit
> +       value is the sign extension of the lower 32 bits.  */
> +    store_signed_integer (buf, register_size (gdbarch, regno), byte_order,
> +                          value);
> +  else
> +    store_unsigned_integer (buf, register_size (gdbarch, regno), byte_order,
> +                            value);

I just wanted to make sure that you are sure that this is correct
(you must be - otherwise I think it'd have shown up in testing).
But it's significantly different from the code that it replaces,
so it attracted my attention.  I guess the initial code had a bug
that's unrelated to the rockhopper, which this hunk fixes.

> @@ -1942,6 +1998,9 @@ mips_fetch_registers (struct target_ops 
>  	  if (mips_monitor == MON_DDB)
>  	    val = (unsigned) mips_request ('t', pmon_reg, 0,
>  					   &err, mips_receive_wait, NULL);
> +	  else if (mips_monitor == MON_ROCKHOPPER)
> +	    val = mips_request ('t', pmon_reg, 0,
> +				&err, mips_receive_wait, NULL);

In this function, val is declared as "unsigned LONGEST" (first time
I ever see this!). Can you simplify this to "ULONGEST"? And also remove
the cast to unsigned in the call to mips_request when mips_monitor ==
MON_DDB. And finally, since the call to mips_request seems to be
identical if mips_monitor == MON_DDB or mips_monitor == MON_ROCKHOPPER,
how about merging them the two branches into one block:

     if (mips_monitor == MON_DDB || mips_monitor == MON_ROCKHOPPER)
         val = mips_request ('t', pmon_reg, 0, [...]);

?


-- 
Joel

Re: [RFC] remote-mips.c: Add support for NEC rockhopper boards

[Kevin Buettner]

On Sat, 6 Mar 2010 08:42:23 +0400
Joel Brobecker <brobecker@adacore.com> wrote:

> > @@ -1196,11 +1227,12 @@ mips_request (int cmd,
> >  {
> >    int addr_size = gdbarch_addr_bit (target_gdbarch) / 8;
> >    char myBuff[DATA_MAXLEN + 1];
> > +  char response_string[17];
> [...]
> > -  if (sscanf (buff, "0x%x %c 0x%x 0x%lx",
> > -	      &rpid, &rcmd, &rerrflg, &rresponse) != 4
> > +  if (sscanf (buff, "0x%x %c 0x%x 0x%16s",
> > +	      &rpid, &rcmd, &rerrflg, response_string) != 4
> > +      || !read_hex_value (response_string, &rresponse)
> 
> I don't understand the need to change the sscanf format for the last
> item from %lx to %16s. It definitely makes the code more complex...

I think this change was made to accomodate the 64-bit rockhopper
boards.  The concern is that sscanf() using %lx might not be able to
handle a 64-bit value.

> > +static void
> > +mips_set_register (int regno, ULONGEST value)
> [...]
> > +  if (mips_monitor != MON_ROCKHOPPER
> > +      && (regno == mips_regnum (gdbarch)->pc || regno < 32))
> > +    /* Some 64-bit boards have monitors that only send the bottom 32 bits.
> > +       In such cases we can only really debug 32-bit code properly so,
> > +       when reading a GPR or the PC, assume that the full 64-bit
> > +       value is the sign extension of the lower 32 bits.  */
> > +    store_signed_integer (buf, register_size (gdbarch, regno), byte_order,
> > +                          value);
> > +  else
> > +    store_unsigned_integer (buf, register_size (gdbarch, regno), byte_order,
> > +                            value);
> 
> I just wanted to make sure that you are sure that this is correct
> (you must be - otherwise I think it'd have shown up in testing).
> But it's significantly different from the code that it replaces,
> so it attracted my attention.  I guess the initial code had a bug
> that's unrelated to the rockhopper, which this hunk fixes.

I see what you mean.  The non-rockhopper case is calling
store_signed_integer(), whereas before store_unsigned_integer() was
used instead.  The comment sounds right to me though - recall that
there were a lot changes doing s/unsigned/signed/ in mips-tdep.c
a while back.

I agree this hunk seems to be fixing a 32-bit -> 64-bit bug instead of
only adding rockhopper support.  I can, if you'd like, revise this
patch, the rockhopper support patch, to use store_unsigned_integer()
(in the non-rockhopper case) and then submit a separate patch which
changes store_unsigned_integer() to store_signed_integer().  For the
time being though, I've left this apparent bug fix in in the patch
below.

> > @@ -1942,6 +1998,9 @@ mips_fetch_registers (struct target_ops 
> >  	  if (mips_monitor == MON_DDB)
> >  	    val = (unsigned) mips_request ('t', pmon_reg, 0,
> >  					   &err, mips_receive_wait, NULL);
> > +	  else if (mips_monitor == MON_ROCKHOPPER)
> > +	    val = mips_request ('t', pmon_reg, 0,
> > +				&err, mips_receive_wait, NULL);
> 
> In this function, val is declared as "unsigned LONGEST" (first time
> I ever see this!). Can you simplify this to "ULONGEST"? And also remove
> the cast to unsigned in the call to mips_request when mips_monitor ==
> MON_DDB. And finally, since the call to mips_request seems to be
> identical if mips_monitor == MON_DDB or mips_monitor == MON_ROCKHOPPER,
> how about merging them the two branches into one block:
> 
>      if (mips_monitor == MON_DDB || mips_monitor == MON_ROCKHOPPER)
>          val = mips_request ('t', pmon_reg, 0, [...]);
> 
> ?

Yes, I agree with both of your suggestions.  See below for a revised
patch which incorporates these suggestions.  Specifically, that
occurrence of `unsigned LONGEST' has been changed to `ULONGEST' and
I've merged the two tests as you show above.  (I couldn't think
of any reason to retain the cast.)

Thanks again for your review.

Kevin

	From Richard Sandiford, Martin M. Hunt, Corinna Vinschen,
	and Kevin Buettner:

	* remote-mips.c (rockhopper_ops): New target_ops struct.
	(MON_ROCKHOPPER): New mips_monitor_type.
	(read_hex_value): New function.
	(mips_request): Send 8-byte values with a 'T' packet.  Read the
	packet argument as a string and use read_hex_value to parse it.  
	(mips_exit_debug): Wait for response when using MON_ROCKHOPPER.
	(rockhopper_open): New function.
	(mips_wait): Read the PC, FP and SP fields as strings.  Use
	read_hex_value to parse them and mips_set_register to commit them.
	(mips_set_register): New function.
	(mips_fetch_registers): Do not cast register value to "unsigned"
	when reading a MON_ROCKHOPPER 't' packet.  Use mips_set_register.
	(mips_store_registers): Use a 'T' packet to set registers when
	using MON_ROCKHOPPER.
	(pmon_end_download): Don't run initEther if using MON_ROCKHOPPER
	and expect the total to be printed before the entry address.
	(_initialize_remote_mips): Initialize and add rockhopper_ops.

Index: remote-mips.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-mips.c,v
retrieving revision 1.109
diff -u -p -r1.109 remote-mips.c
--- remote-mips.c	5 Mar 2010 16:18:54 -0000	1.109
+++ remote-mips.c	6 Mar 2010 06:35:58 -0000
@@ -89,9 +89,11 @@ static void mips_detach (struct target_o
 
 static int mips_map_regno (struct gdbarch *, int);
 
+static void mips_set_register (int regno, ULONGEST value);
+
 static void mips_prepare_to_store (struct regcache *regcache);
 
-static unsigned int mips_fetch_word (CORE_ADDR addr);
+static int mips_fetch_word (CORE_ADDR addr, unsigned int *valp);
 
 static int mips_store_word (CORE_ADDR addr, unsigned int value,
 			    int *old_contents);
@@ -143,6 +145,7 @@ static int mips_common_breakpoint (int s
 extern struct target_ops mips_ops;
 extern struct target_ops pmon_ops;
 extern struct target_ops ddb_ops;
+extern struct target_ops rockhopper_ops;
 \f/* *INDENT-OFF* */
 /* The MIPS remote debugging interface is built on top of a simple
    packet protocol.  Each packet is organized as follows:
@@ -288,7 +291,7 @@ extern struct target_ops ddb_ops;
    These are initialized with code in _initialize_remote_mips instead
    of static initializers, to make it easier to extend the target_ops
    vector later.  */
-struct target_ops mips_ops, pmon_ops, ddb_ops, lsi_ops;
+struct target_ops mips_ops, pmon_ops, ddb_ops, rockhopper_ops, lsi_ops;
 
 enum mips_monitor_type
   {
@@ -298,6 +301,7 @@ enum mips_monitor_type
     MON_PMON,			/* 3.0.83 [COGENT,EB,FP,NET] Algorithmics Ltd. Nov  9 1995 17:19:50 */
     MON_DDB,			/* 2.7.473 [DDBVR4300,EL,FP,NET] Risq Modular Systems,  Thu Jun 6 09:28:40 PDT 1996 */
     MON_LSI,			/* 4.3.12 [EB,FP], LSI LOGIC Corp. Tue Feb 25 13:22:14 1997 */
+    MON_ROCKHOPPER,
     /* Last and unused value, for sizing vectors, etc. */
     MON_LAST
   };
@@ -527,6 +531,33 @@ fputs_readable (const char *string, stru
 }
 
 
+/* Read P as a hex value.  Return true if every character made sense,
+   storing the result in *RESULT.  Leave *RESULT unchanged otherwise.  */
+
+static int
+read_hex_value (const char *p, ULONGEST *result)
+{
+  ULONGEST retval;
+
+  retval = 0;
+  while (*p != 0)
+    {
+      retval <<= 4;
+      if (*p >= '0' && *p <= '9')
+	retval |= *p - '0';
+      else if (*p >= 'A' && *p <= 'F')
+	retval |= *p - 'A' + 10;
+      else if (*p >= 'a' && *p <= 'f')
+	retval |= *p - 'a' + 10;
+      else
+	return 0;
+      p++;
+    }
+  *result = retval;
+  return 1;
+}
+
+
 /* Wait until STRING shows up in mips_desc.  Returns 1 if successful, else 0 if
    timed out.  TIMEOUT specifies timeout value in seconds.
  */
@@ -1196,11 +1227,12 @@ mips_request (int cmd,
 {
   int addr_size = gdbarch_addr_bit (target_gdbarch) / 8;
   char myBuff[DATA_MAXLEN + 1];
+  char response_string[17];
   int len;
   int rpid;
   char rcmd;
   int rerrflg;
-  unsigned long rresponse;
+  ULONGEST rresponse;
 
   if (buff == (char *) NULL)
     buff = myBuff;
@@ -1210,8 +1242,15 @@ mips_request (int cmd,
       if (mips_need_reply)
 	internal_error (__FILE__, __LINE__,
 			_("mips_request: Trying to send command before reply"));
-      sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
-	       phex_nz (addr, addr_size), phex_nz (data, addr_size));
+      /* 'T' sets a register to a 64-bit value, so make sure we use
+	 the right conversion function.  */
+      if (cmd == 'T')
+	sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
+		 phex_nz (addr, addr_size), phex_nz (data, 8));
+      else
+	sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
+	         phex_nz (addr, addr_size), phex_nz (data, addr_size));
+
       mips_send_packet (buff, 1);
       mips_need_reply = 1;
     }
@@ -1228,8 +1267,9 @@ mips_request (int cmd,
   len = mips_receive_packet (buff, 1, timeout);
   buff[len] = '\0';
 
-  if (sscanf (buff, "0x%x %c 0x%x 0x%lx",
-	      &rpid, &rcmd, &rerrflg, &rresponse) != 4
+  if (sscanf (buff, "0x%x %c 0x%x 0x%16s",
+	      &rpid, &rcmd, &rerrflg, response_string) != 4
+      || !read_hex_value (response_string, &rresponse)
       || (cmd != '\0' && rcmd != cmd))
     mips_error ("Bad response from remote board");
 
@@ -1311,7 +1351,7 @@ mips_exit_debug (void)
 
   mips_exiting = 1;
 
-  if (mips_monitor != MON_IDT)
+  if (mips_monitor != MON_IDT && mips_monitor != MON_ROCKHOPPER)
     {
       /* The DDB (NEC) and MiniRISC (LSI) versions of PMON exit immediately,
          so we do not get a reply to this command: */
@@ -1625,6 +1665,12 @@ ddb_open (char *name, int from_tty)
 }
 
 static void
+rockhopper_open (char *name, int from_tty)
+{
+  common_open (&rockhopper_ops, name, from_tty, MON_ROCKHOPPER, "NEC01>");
+}
+
+static void
 lsi_open (char *name, int from_tty)
 {
   int i;
@@ -1704,6 +1750,32 @@ mips_signal_from_protocol (int sig)
   return (enum target_signal) sig;
 }
 
+static void
+mips_set_register (int regno, ULONGEST value)
+{
+  char buf[MAX_REGISTER_SIZE];
+  struct regcache *regcache = get_current_regcache ();
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+
+  /* We got the number the register holds, but gdb expects to see a
+     value in the target byte ordering.  */
+
+  if (mips_monitor != MON_ROCKHOPPER
+      && (regno == mips_regnum (gdbarch)->pc || regno < 32))
+    /* Some 64-bit boards have monitors that only send the bottom 32 bits.
+       In such cases we can only really debug 32-bit code properly so,
+       when reading a GPR or the PC, assume that the full 64-bit
+       value is the sign extension of the lower 32 bits.  */
+    store_signed_integer (buf, register_size (gdbarch, regno), byte_order,
+                          value);
+  else
+    store_unsigned_integer (buf, register_size (gdbarch, regno), byte_order,
+                            value);
+
+  regcache_raw_supply (regcache, regno, buf);
+}
+
 /* Wait until the remote stops, and return a wait status.  */
 
 static ptid_t
@@ -1713,8 +1785,8 @@ mips_wait (struct target_ops *ops,
   int rstatus;
   int err;
   char buff[DATA_MAXLEN];
-  int rpc, rfp, rsp;
-  char flags[20];
+  ULONGEST rpc, rfp, rsp;
+  char pc_string[17], fp_string[17], sp_string[17], flags[20];
   int nfields;
   int i;
 
@@ -1754,35 +1826,19 @@ mips_wait (struct target_ops *ops,
 
   /* See if we got back extended status.  If so, pick out the pc, fp, sp, etc... */
 
-  nfields = sscanf (buff, "0x%*x %*c 0x%*x 0x%*x 0x%x 0x%x 0x%x 0x%*x %s",
-		    &rpc, &rfp, &rsp, flags);
-  if (nfields >= 3)
+  nfields = sscanf (buff, "0x%*x %*c 0x%*x 0x%*x 0x%16s 0x%16s 0x%16s 0x%*x %s",
+		    pc_string, fp_string, sp_string, flags);
+  if (nfields >= 3
+      && read_hex_value (pc_string, &rpc)
+      && read_hex_value (fp_string, &rfp)
+      && read_hex_value (sp_string, &rsp))
     {
       struct regcache *regcache = get_current_regcache ();
       struct gdbarch *gdbarch = get_regcache_arch (regcache);
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      char buf[MAX_REGISTER_SIZE];
 
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_pc_regnum (gdbarch)),
-	 byte_order, rpc);
-      regcache_raw_supply (regcache, gdbarch_pc_regnum (gdbarch), buf);
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_pc_regnum (gdbarch)),
-	 byte_order, rfp);
-      regcache_raw_supply (regcache, 30, buf);	/* This register they are avoiding and so it is unnamed */
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_sp_regnum (gdbarch)),
-	 byte_order, rsp);
-      regcache_raw_supply (regcache, gdbarch_sp_regnum (gdbarch), buf);
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_deprecated_fp_regnum (gdbarch)),
-	 byte_order, 0);
-      regcache_raw_supply (regcache,
-			   gdbarch_deprecated_fp_regnum (gdbarch), buf);
+      mips_set_register (gdbarch_pc_regnum (gdbarch), rpc);
+      mips_set_register (30, rfp);
+      mips_set_register (gdbarch_sp_regnum (gdbarch), rsp);
 
       if (nfields == 9)
 	{
@@ -1912,7 +1968,7 @@ mips_fetch_registers (struct target_ops 
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  unsigned LONGEST val;
+  ULONGEST val;
   int err;
 
   if (regno == -1)
@@ -1939,9 +1995,9 @@ mips_fetch_registers (struct target_ops 
 	  /* Unfortunately the PMON version in the Vr4300 board has been
 	     compiled without the 64bit register access commands. This
 	     means we cannot get hold of the full register width. */
-	  if (mips_monitor == MON_DDB)
-	    val = (unsigned) mips_request ('t', pmon_reg, 0,
-					   &err, mips_receive_wait, NULL);
+	  if (mips_monitor == MON_DDB || mips_monitor == MON_ROCKHOPPER)
+	    val = mips_request ('t', pmon_reg, 0,
+				&err, mips_receive_wait, NULL);
 	  else
 	    val = mips_request ('r', pmon_reg, 0,
 				&err, mips_receive_wait, NULL);
@@ -1951,15 +2007,7 @@ mips_fetch_registers (struct target_ops 
 	}
     }
 
-  {
-    char buf[MAX_REGISTER_SIZE];
-
-    /* We got the number the register holds, but gdb expects to see a
-       value in the target byte ordering.  */
-    store_unsigned_integer (buf, register_size (gdbarch, regno),
-			    byte_order, val);
-    regcache_raw_supply (regcache, regno, buf);
-  }
+  mips_set_register (regno, val);
 }
 
 /* Prepare to store registers.  The MIPS protocol can store individual
@@ -1988,31 +2036,31 @@ mips_store_registers (struct target_ops 
     }
 
   regcache_cooked_read_unsigned (regcache, regno, &val);
-  mips_request ('R', mips_map_regno (gdbarch, regno), val,
+  mips_request (mips_monitor == MON_ROCKHOPPER ? 'T' : 'R',
+  		mips_map_regno (gdbarch, regno),
+		val,
 		&err, mips_receive_wait, NULL);
   if (err)
     mips_error ("Can't write register %d: %s", regno, safe_strerror (errno));
 }
 
-/* Fetch a word from the target board.  */
+/* Fetch a word from the target board.  Return word fetched in location
+   addressed by VALP.  Return 0 when successful; return positive error
+   code when not.  */
 
-static unsigned int
-mips_fetch_word (CORE_ADDR addr)
+static int
+mips_fetch_word (CORE_ADDR addr, unsigned int *valp)
 {
-  unsigned int val;
   int err;
 
-  val = mips_request ('d', addr, 0, &err, mips_receive_wait, NULL);
+  *valp = mips_request ('d', addr, 0, &err, mips_receive_wait, NULL);
   if (err)
     {
       /* Data space failed; try instruction space.  */
-      val = mips_request ('i', addr, 0, &err,
-			  mips_receive_wait, NULL);
-      if (err)
-	mips_error ("Can't read address %s: %s",
-		    paddress (target_gdbarch, addr), safe_strerror (errno));
+      *valp = mips_request ('i', addr, 0, &err,
+			    mips_receive_wait, NULL);
     }
-  return val;
+  return err;
 }
 
 /* Store a word to the target board.  Returns errno code or zero for
@@ -2078,17 +2126,25 @@ mips_xfer_memory (CORE_ADDR memaddr, gdb
       /* Fill start and end extra bytes of buffer with existing data.  */
       if (addr != memaddr || len < 4)
 	{
+	  unsigned int val;
+
+	  if (mips_fetch_word (addr, &val))
+	    return 0;
+
 	  /* Need part of initial word -- fetch it.  */
-	  store_unsigned_integer (&buffer[0], 4, byte_order,
-				  mips_fetch_word (addr));
+	  store_unsigned_integer (&buffer[0], 4, byte_order, val);
 	}
 
       if (count > 1)
 	{
+	  unsigned int val;
+
 	  /* Need part of last word -- fetch it.  FIXME: we do this even
 	     if we don't need it.  */
-	  store_unsigned_integer (&buffer[(count - 1) * 4], 4, byte_order,
-				  mips_fetch_word (addr + (count - 1) * 4));
+	  if (mips_fetch_word (addr + (count - 1) * 4, &val))
+	    return 0;
+
+	  store_unsigned_integer (&buffer[(count - 1) * 4], 4, byte_order, val);
 	}
 
       /* Copy data to be written over corresponding part of buffer */
@@ -2123,8 +2179,12 @@ mips_xfer_memory (CORE_ADDR memaddr, gdb
       /* Read all the longwords */
       for (i = 0; i < count; i++, addr += 4)
 	{
-	  store_unsigned_integer (&buffer[i * 4], 4, byte_order,
-				  mips_fetch_word (addr));
+	  unsigned int val;
+
+	  if (mips_fetch_word (addr, &val))
+	    return 0;
+
+	  store_unsigned_integer (&buffer[i * 4], 4, byte_order, val);
 	  QUIT;
 	}
 
@@ -2363,7 +2423,7 @@ static int
 mips_check_lsi_error (CORE_ADDR addr, int rerrflg)
 {
   struct lsi_error *err;
-  char *saddr = paddress (target_gdbarch, addr);
+  const char *saddr = paddress (target_gdbarch, addr);
 
   if (rerrflg == 0)		/* no error */
     return 0;
@@ -3080,7 +3140,8 @@ pmon_end_download (int final, int bintot
 	chmod (tftp_localname, stbuf.st_mode | S_IROTH);
 
       /* Must reinitialize the board to prevent PMON from crashing.  */
-      mips_send_command ("initEther\r", -1);
+      if (mips_monitor != MON_ROCKHOPPER)
+	mips_send_command ("initEther\r", -1);
 
       /* Send the load command.  */
       cmd = xmalloc (strlen (load_cmd_prefix) + strlen (tftp_name) + 2);
@@ -3108,6 +3169,11 @@ pmon_end_download (int final, int bintot
       if (!pmon_check_total (bintotal))
 	return;
       break;
+    case MON_ROCKHOPPER:
+      if (!pmon_check_total (bintotal))
+	return;
+      pmon_check_entry_address ("Entry Address  = ", final);
+      break;
     default:
       pmon_check_entry_address ("Entry Address  = ", final);
       pmon_check_ack ("termination");
@@ -3395,7 +3461,7 @@ _initialize_remote_mips (void)
   mips_ops.to_magic = OPS_MAGIC;
 
   /* Copy the common fields to all four target vectors.  */
-  pmon_ops = ddb_ops = lsi_ops = mips_ops;
+  rockhopper_ops = pmon_ops = ddb_ops = lsi_ops = mips_ops;
 
   /* Initialize target-specific fields in the target vectors.  */
   mips_ops.to_shortname = "mips";
@@ -3425,6 +3491,11 @@ of the TFTP temporary file, if it differ
   ddb_ops.to_open = ddb_open;
   ddb_ops.to_wait = mips_wait;
 
+  rockhopper_ops.to_shortname = "rockhopper";
+  rockhopper_ops.to_doc = ddb_ops.to_doc;
+  rockhopper_ops.to_open = rockhopper_open;
+  rockhopper_ops.to_wait = mips_wait;
+
   lsi_ops.to_shortname = "lsi";
   lsi_ops.to_doc = pmon_ops.to_doc;
   lsi_ops.to_open = lsi_open;
@@ -3435,6 +3506,7 @@ of the TFTP temporary file, if it differ
   add_target (&pmon_ops);
   add_target (&ddb_ops);
   add_target (&lsi_ops);
+  add_target (&rockhopper_ops);
 
   add_setshow_zinteger_cmd ("timeout", no_class, &mips_receive_wait, _("\
 Set timeout in seconds for remote MIPS serial I/O."), _("\

Re: [RFC] remote-mips.c: Add support for NEC rockhopper boards

[Joel Brobecker]

> I think this change was made to accomodate the 64-bit rockhopper
> boards.  The concern is that sscanf() using %lx might not be able to
> handle a 64-bit value.

Hmmm, that's a good point.  The most obvious case is when the host is
a 32bit arch, where long will most likely be too small to hold a 64bit
value.  But even on 64bit machines, long might still be 32bits (this is
the case on x64 Windows).

> I agree this hunk seems to be fixing a 32-bit -> 64-bit bug instead of
> only adding rockhopper support.  I can, if you'd like, revise this
> patch, the rockhopper support patch, to use store_unsigned_integer()
> (in the non-rockhopper case) and then submit a separate patch which
> changes store_unsigned_integer() to store_signed_integer().

I don't think that this will be necessary - it would be creating
extra work for little gain, IMO.  It makes things easier on the reviewer
if things are split, but once it's reviewed, the only advantage in
splitting the patch that I know of is that it makes it easier to revert
one piece or the other, in case of a revert is needed.

> 	From Richard Sandiford, Martin M. Hunt, Corinna Vinschen,
> 	and Kevin Buettner:
> 
> 	* remote-mips.c (rockhopper_ops): New target_ops struct.
> 	(MON_ROCKHOPPER): New mips_monitor_type.
> 	(read_hex_value): New function.
> 	(mips_request): Send 8-byte values with a 'T' packet.  Read the
> 	packet argument as a string and use read_hex_value to parse it.  
> 	(mips_exit_debug): Wait for response when using MON_ROCKHOPPER.
> 	(rockhopper_open): New function.
> 	(mips_wait): Read the PC, FP and SP fields as strings.  Use
> 	read_hex_value to parse them and mips_set_register to commit them.
> 	(mips_set_register): New function.
> 	(mips_fetch_registers): Do not cast register value to "unsigned"
> 	when reading a MON_ROCKHOPPER 't' packet.  Use mips_set_register.
> 	(mips_store_registers): Use a 'T' packet to set registers when
> 	using MON_ROCKHOPPER.
> 	(pmon_end_download): Don't run initEther if using MON_ROCKHOPPER
> 	and expect the total to be printed before the entry address.
> 	(_initialize_remote_mips): Initialize and add rockhopper_ops.

Overall, the patch looks good to me.  However, watch out that some
hunks of your patch contain hunks for another earlier patch (about
changing memory reads so as not to throw an exception).  The rest
looks about the same, with the mods we discussed.

Just one minor nit:

+static void
+mips_set_register (int regno, ULONGEST value)

Would you mind adding a short description of what this function does.
It's sort of obvious from the name, but we're trying to document every
function, and I'd rather we document all functions, including the obvious
ones, rather than having to decide which ones deserve a descriptions
and which ones don't.

I don't think I will have much else to add, so you shouldn't wait for me
to approve a final version.

-- 
Joel

Re: [RFC] remote-mips.c: Add support for NEC rockhopper boards

[Kevin Buettner]

On Sun, 7 Mar 2010 09:33:17 +0400
Joel Brobecker <brobecker@adacore.com> wrote:

> > 	From Richard Sandiford, Martin M. Hunt, Corinna Vinschen,
> > 	and Kevin Buettner:
> > 
> > 	* remote-mips.c (rockhopper_ops): New target_ops struct.
> > 	(MON_ROCKHOPPER): New mips_monitor_type.
> > 	(read_hex_value): New function.
> > 	(mips_request): Send 8-byte values with a 'T' packet.  Read the
> > 	packet argument as a string and use read_hex_value to parse it.  
> > 	(mips_exit_debug): Wait for response when using MON_ROCKHOPPER.
> > 	(rockhopper_open): New function.
> > 	(mips_wait): Read the PC, FP and SP fields as strings.  Use
> > 	read_hex_value to parse them and mips_set_register to commit them.
> > 	(mips_set_register): New function.
> > 	(mips_fetch_registers): Do not cast register value to "unsigned"
> > 	when reading a MON_ROCKHOPPER 't' packet.  Use mips_set_register.
> > 	(mips_store_registers): Use a 'T' packet to set registers when
> > 	using MON_ROCKHOPPER.
> > 	(pmon_end_download): Don't run initEther if using MON_ROCKHOPPER
> > 	and expect the total to be printed before the entry address.
> > 	(_initialize_remote_mips): Initialize and add rockhopper_ops.
> 
> Overall, the patch looks good to me.  However, watch out that some
> hunks of your patch contain hunks for another earlier patch (about
> changing memory reads so as not to throw an exception).

Yes, I messed up the diff.  (The tree in question actually had both sets
of changes.  It was my intention to diff it against another tree that had
the earlier changes, but not the latter changes.  But, instead of doing
that, I just did a "cvs diff -up" and got the whole thing.)

> Just one minor nit:
> 
> +static void
> +mips_set_register (int regno, ULONGEST value)
> 
> Would you mind adding a short description of what this function does.
> It's sort of obvious from the name, but we're trying to document every
> function, and I'd rather we document all functions, including the obvious
> ones, rather than having to decide which ones deserve a descriptions
> and which ones don't.

Yes, I agree.

I've added a comments for mips_set_register() and rockhopper_open().
I noticed that several of the other _open functions are missing
comments describing what they do also.  I was tempted to add those
comments to this patch too, but decided against it since that would go
beyond the intended scope of this patch.  I'll commit another patch
shortly which adds the needed comments.

> I don't think I will have much else to add, so you shouldn't wait for me
> to approve a final version.

Thanks again for looking it over.  I appreciate seeing your thoughtful
comments; the resulting code is better for it.

Below is the patch that I committed.

Kevin


2010-03-08  Kevin Buettner  <kevinb@redhat.com>

	From Richard Sandiford, Martin M. Hunt, Corinna Vinschen,
	and Kevin Buettner:

	* remote-mips.c (rockhopper_ops): New target_ops struct.
	(MON_ROCKHOPPER): New mips_monitor_type.
	(read_hex_value): New function.
	(mips_request): Send 8-byte values with a 'T' packet.  Read the
	packet argument as a string and use read_hex_value to parse it.  
	(mips_exit_debug): Wait for response when using MON_ROCKHOPPER.
	(rockhopper_open): New function.
	(mips_wait): Read the PC, FP and SP fields as strings.  Use
	read_hex_value to parse them and mips_set_register to commit them.
	(mips_set_register): New function.
	(mips_fetch_registers): Do not cast register value to "unsigned"
	when reading a MON_ROCKHOPPER 't' packet.  Use mips_set_register.
	(mips_store_registers): Use a 'T' packet to set registers when
	using MON_ROCKHOPPER.
	(pmon_end_download): Don't run initEther if using MON_ROCKHOPPER
	and expect the total to be printed before the entry address.
	(_initialize_remote_mips): Initialize and add rockhopper_ops.

Index: remote-mips.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-mips.c,v
retrieving revision 1.110
diff -u -p -r1.110 remote-mips.c
--- remote-mips.c	8 Mar 2010 18:22:06 -0000	1.110
+++ remote-mips.c	8 Mar 2010 19:05:00 -0000
@@ -89,6 +89,8 @@ static void mips_detach (struct target_o
 
 static int mips_map_regno (struct gdbarch *, int);
 
+static void mips_set_register (int regno, ULONGEST value);
+
 static void mips_prepare_to_store (struct regcache *regcache);
 
 static int mips_fetch_word (CORE_ADDR addr, unsigned int *valp);
@@ -143,6 +145,7 @@ static int mips_common_breakpoint (int s
 extern struct target_ops mips_ops;
 extern struct target_ops pmon_ops;
 extern struct target_ops ddb_ops;
+extern struct target_ops rockhopper_ops;
 \f/* *INDENT-OFF* */
 /* The MIPS remote debugging interface is built on top of a simple
    packet protocol.  Each packet is organized as follows:
@@ -288,7 +291,7 @@ extern struct target_ops ddb_ops;
    These are initialized with code in _initialize_remote_mips instead
    of static initializers, to make it easier to extend the target_ops
    vector later.  */
-struct target_ops mips_ops, pmon_ops, ddb_ops, lsi_ops;
+struct target_ops mips_ops, pmon_ops, ddb_ops, rockhopper_ops, lsi_ops;
 
 enum mips_monitor_type
   {
@@ -298,6 +301,7 @@ enum mips_monitor_type
     MON_PMON,			/* 3.0.83 [COGENT,EB,FP,NET] Algorithmics Ltd. Nov  9 1995 17:19:50 */
     MON_DDB,			/* 2.7.473 [DDBVR4300,EL,FP,NET] Risq Modular Systems,  Thu Jun 6 09:28:40 PDT 1996 */
     MON_LSI,			/* 4.3.12 [EB,FP], LSI LOGIC Corp. Tue Feb 25 13:22:14 1997 */
+    MON_ROCKHOPPER,
     /* Last and unused value, for sizing vectors, etc. */
     MON_LAST
   };
@@ -527,6 +531,33 @@ fputs_readable (const char *string, stru
 }
 
 
+/* Read P as a hex value.  Return true if every character made sense,
+   storing the result in *RESULT.  Leave *RESULT unchanged otherwise.  */
+
+static int
+read_hex_value (const char *p, ULONGEST *result)
+{
+  ULONGEST retval;
+
+  retval = 0;
+  while (*p != 0)
+    {
+      retval <<= 4;
+      if (*p >= '0' && *p <= '9')
+	retval |= *p - '0';
+      else if (*p >= 'A' && *p <= 'F')
+	retval |= *p - 'A' + 10;
+      else if (*p >= 'a' && *p <= 'f')
+	retval |= *p - 'a' + 10;
+      else
+	return 0;
+      p++;
+    }
+  *result = retval;
+  return 1;
+}
+
+
 /* Wait until STRING shows up in mips_desc.  Returns 1 if successful, else 0 if
    timed out.  TIMEOUT specifies timeout value in seconds.
  */
@@ -1196,11 +1227,12 @@ mips_request (int cmd,
 {
   int addr_size = gdbarch_addr_bit (target_gdbarch) / 8;
   char myBuff[DATA_MAXLEN + 1];
+  char response_string[17];
   int len;
   int rpid;
   char rcmd;
   int rerrflg;
-  unsigned long rresponse;
+  ULONGEST rresponse;
 
   if (buff == (char *) NULL)
     buff = myBuff;
@@ -1210,8 +1242,15 @@ mips_request (int cmd,
       if (mips_need_reply)
 	internal_error (__FILE__, __LINE__,
 			_("mips_request: Trying to send command before reply"));
-      sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
-	       phex_nz (addr, addr_size), phex_nz (data, addr_size));
+      /* 'T' sets a register to a 64-bit value, so make sure we use
+	 the right conversion function.  */
+      if (cmd == 'T')
+	sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
+		 phex_nz (addr, addr_size), phex_nz (data, 8));
+      else
+	sprintf (buff, "0x0 %c 0x%s 0x%s", cmd,
+	         phex_nz (addr, addr_size), phex_nz (data, addr_size));
+
       mips_send_packet (buff, 1);
       mips_need_reply = 1;
     }
@@ -1228,8 +1267,9 @@ mips_request (int cmd,
   len = mips_receive_packet (buff, 1, timeout);
   buff[len] = '\0';
 
-  if (sscanf (buff, "0x%x %c 0x%x 0x%lx",
-	      &rpid, &rcmd, &rerrflg, &rresponse) != 4
+  if (sscanf (buff, "0x%x %c 0x%x 0x%16s",
+	      &rpid, &rcmd, &rerrflg, response_string) != 4
+      || !read_hex_value (response_string, &rresponse)
       || (cmd != '\0' && rcmd != cmd))
     mips_error ("Bad response from remote board");
 
@@ -1311,7 +1351,7 @@ mips_exit_debug (void)
 
   mips_exiting = 1;
 
-  if (mips_monitor != MON_IDT)
+  if (mips_monitor != MON_IDT && mips_monitor != MON_ROCKHOPPER)
     {
       /* The DDB (NEC) and MiniRISC (LSI) versions of PMON exit immediately,
          so we do not get a reply to this command: */
@@ -1624,6 +1664,14 @@ ddb_open (char *name, int from_tty)
   common_open (&ddb_ops, name, from_tty, MON_DDB, "NEC010>");
 }
 
+/* Open a connection to a rockhopper board.  */
+
+static void
+rockhopper_open (char *name, int from_tty)
+{
+  common_open (&rockhopper_ops, name, from_tty, MON_ROCKHOPPER, "NEC01>");
+}
+
 static void
 lsi_open (char *name, int from_tty)
 {
@@ -1704,6 +1752,34 @@ mips_signal_from_protocol (int sig)
   return (enum target_signal) sig;
 }
 
+/* Set the register designated by REGNO to the value designated by VALUE.  */
+
+static void
+mips_set_register (int regno, ULONGEST value)
+{
+  char buf[MAX_REGISTER_SIZE];
+  struct regcache *regcache = get_current_regcache ();
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+
+  /* We got the number the register holds, but gdb expects to see a
+     value in the target byte ordering.  */
+
+  if (mips_monitor != MON_ROCKHOPPER
+      && (regno == mips_regnum (gdbarch)->pc || regno < 32))
+    /* Some 64-bit boards have monitors that only send the bottom 32 bits.
+       In such cases we can only really debug 32-bit code properly so,
+       when reading a GPR or the PC, assume that the full 64-bit
+       value is the sign extension of the lower 32 bits.  */
+    store_signed_integer (buf, register_size (gdbarch, regno), byte_order,
+                          value);
+  else
+    store_unsigned_integer (buf, register_size (gdbarch, regno), byte_order,
+                            value);
+
+  regcache_raw_supply (regcache, regno, buf);
+}
+
 /* Wait until the remote stops, and return a wait status.  */
 
 static ptid_t
@@ -1713,8 +1789,8 @@ mips_wait (struct target_ops *ops,
   int rstatus;
   int err;
   char buff[DATA_MAXLEN];
-  int rpc, rfp, rsp;
-  char flags[20];
+  ULONGEST rpc, rfp, rsp;
+  char pc_string[17], fp_string[17], sp_string[17], flags[20];
   int nfields;
   int i;
 
@@ -1754,35 +1830,19 @@ mips_wait (struct target_ops *ops,
 
   /* See if we got back extended status.  If so, pick out the pc, fp, sp, etc... */
 
-  nfields = sscanf (buff, "0x%*x %*c 0x%*x 0x%*x 0x%x 0x%x 0x%x 0x%*x %s",
-		    &rpc, &rfp, &rsp, flags);
-  if (nfields >= 3)
+  nfields = sscanf (buff, "0x%*x %*c 0x%*x 0x%*x 0x%16s 0x%16s 0x%16s 0x%*x %s",
+		    pc_string, fp_string, sp_string, flags);
+  if (nfields >= 3
+      && read_hex_value (pc_string, &rpc)
+      && read_hex_value (fp_string, &rfp)
+      && read_hex_value (sp_string, &rsp))
     {
       struct regcache *regcache = get_current_regcache ();
       struct gdbarch *gdbarch = get_regcache_arch (regcache);
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      char buf[MAX_REGISTER_SIZE];
 
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_pc_regnum (gdbarch)),
-	 byte_order, rpc);
-      regcache_raw_supply (regcache, gdbarch_pc_regnum (gdbarch), buf);
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_pc_regnum (gdbarch)),
-	 byte_order, rfp);
-      regcache_raw_supply (regcache, 30, buf);	/* This register they are avoiding and so it is unnamed */
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_sp_regnum (gdbarch)),
-	 byte_order, rsp);
-      regcache_raw_supply (regcache, gdbarch_sp_regnum (gdbarch), buf);
-
-      store_unsigned_integer
-	(buf, register_size (gdbarch, gdbarch_deprecated_fp_regnum (gdbarch)),
-	 byte_order, 0);
-      regcache_raw_supply (regcache,
-			   gdbarch_deprecated_fp_regnum (gdbarch), buf);
+      mips_set_register (gdbarch_pc_regnum (gdbarch), rpc);
+      mips_set_register (30, rfp);
+      mips_set_register (gdbarch_sp_regnum (gdbarch), rsp);
 
       if (nfields == 9)
 	{
@@ -1912,7 +1972,7 @@ mips_fetch_registers (struct target_ops 
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  unsigned LONGEST val;
+  ULONGEST val;
   int err;
 
   if (regno == -1)
@@ -1939,9 +1999,9 @@ mips_fetch_registers (struct target_ops 
 	  /* Unfortunately the PMON version in the Vr4300 board has been
 	     compiled without the 64bit register access commands. This
 	     means we cannot get hold of the full register width. */
-	  if (mips_monitor == MON_DDB)
-	    val = (unsigned) mips_request ('t', pmon_reg, 0,
-					   &err, mips_receive_wait, NULL);
+	  if (mips_monitor == MON_DDB || mips_monitor == MON_ROCKHOPPER)
+	    val = mips_request ('t', pmon_reg, 0,
+				&err, mips_receive_wait, NULL);
 	  else
 	    val = mips_request ('r', pmon_reg, 0,
 				&err, mips_receive_wait, NULL);
@@ -1951,15 +2011,7 @@ mips_fetch_registers (struct target_ops 
 	}
     }
 
-  {
-    char buf[MAX_REGISTER_SIZE];
-
-    /* We got the number the register holds, but gdb expects to see a
-       value in the target byte ordering.  */
-    store_unsigned_integer (buf, register_size (gdbarch, regno),
-			    byte_order, val);
-    regcache_raw_supply (regcache, regno, buf);
-  }
+  mips_set_register (regno, val);
 }
 
 /* Prepare to store registers.  The MIPS protocol can store individual
@@ -1988,7 +2040,9 @@ mips_store_registers (struct target_ops 
     }
 
   regcache_cooked_read_unsigned (regcache, regno, &val);
-  mips_request ('R', mips_map_regno (gdbarch, regno), val,
+  mips_request (mips_monitor == MON_ROCKHOPPER ? 'T' : 'R',
+  		mips_map_regno (gdbarch, regno),
+		val,
 		&err, mips_receive_wait, NULL);
   if (err)
     mips_error ("Can't write register %d: %s", regno, safe_strerror (errno));
@@ -2373,7 +2427,7 @@ static int
 mips_check_lsi_error (CORE_ADDR addr, int rerrflg)
 {
   struct lsi_error *err;
-  char *saddr = paddress (target_gdbarch, addr);
+  const char *saddr = paddress (target_gdbarch, addr);
 
   if (rerrflg == 0)		/* no error */
     return 0;
@@ -3090,7 +3144,8 @@ pmon_end_download (int final, int bintot
 	chmod (tftp_localname, stbuf.st_mode | S_IROTH);
 
       /* Must reinitialize the board to prevent PMON from crashing.  */
-      mips_send_command ("initEther\r", -1);
+      if (mips_monitor != MON_ROCKHOPPER)
+	mips_send_command ("initEther\r", -1);
 
       /* Send the load command.  */
       cmd = xmalloc (strlen (load_cmd_prefix) + strlen (tftp_name) + 2);
@@ -3118,6 +3173,11 @@ pmon_end_download (int final, int bintot
       if (!pmon_check_total (bintotal))
 	return;
       break;
+    case MON_ROCKHOPPER:
+      if (!pmon_check_total (bintotal))
+	return;
+      pmon_check_entry_address ("Entry Address  = ", final);
+      break;
     default:
       pmon_check_entry_address ("Entry Address  = ", final);
       pmon_check_ack ("termination");
@@ -3405,7 +3465,7 @@ _initialize_remote_mips (void)
   mips_ops.to_magic = OPS_MAGIC;
 
   /* Copy the common fields to all four target vectors.  */
-  pmon_ops = ddb_ops = lsi_ops = mips_ops;
+  rockhopper_ops = pmon_ops = ddb_ops = lsi_ops = mips_ops;
 
   /* Initialize target-specific fields in the target vectors.  */
   mips_ops.to_shortname = "mips";
@@ -3435,6 +3495,11 @@ of the TFTP temporary file, if it differ
   ddb_ops.to_open = ddb_open;
   ddb_ops.to_wait = mips_wait;
 
+  rockhopper_ops.to_shortname = "rockhopper";
+  rockhopper_ops.to_doc = ddb_ops.to_doc;
+  rockhopper_ops.to_open = rockhopper_open;
+  rockhopper_ops.to_wait = mips_wait;
+
   lsi_ops.to_shortname = "lsi";
   lsi_ops.to_doc = pmon_ops.to_doc;
   lsi_ops.to_open = lsi_open;
@@ -3445,6 +3510,7 @@ of the TFTP temporary file, if it differ
   add_target (&pmon_ops);
   add_target (&ddb_ops);
   add_target (&lsi_ops);
+  add_target (&rockhopper_ops);
 
   add_setshow_zinteger_cmd ("timeout", no_class, &mips_receive_wait, _("\
 Set timeout in seconds for remote MIPS serial I/O."), _("\

Re: [RFC] remote-mips.c: Add support for NEC rockhopper boards

[Joel Brobecker]

> I noticed that several of the other _open functions are missing
> comments describing what they do also.  I was tempted to add those
> comments to this patch too, but decided against it since that would go
> beyond the intended scope of this patch.

Definitely agreed. It's great that you're willing to improve the situation
in this case, but it's only up to you (it would be unfair to ask this
everytime a file is touched...).

-- 
Joel