This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] remote-mips.c: Add support for NEC rockhopper boards
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;
/* *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."), _("\