This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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;
 /* *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."), _("\


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]