[RFC] small change for better error reporting in remote.c

[RFC] small change for better error reporting in remote.c

[Michael Snyder]

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

AFAICT, this has been mildly broken forever.

If you look at store_register_using_P and fetch_register_using_p,
they both call remote_send, followed by packet_ok, and then send
error if PACKET_ERROR.

Unfortunately the PACKET_ERROR path is dead code, because remote_send
already bails with a much more general (ie. less useful) error msg
if we get back an Exx reply.

Most of us will have never seen this happen, because how can a
remote target fail to write a register?

But it can happen when the target is a replay recording as with
Virtutech and VMware targets.

This change is to give a more helpful error message, eg.
'Could not write register "eip"', as opposed to
'Remote failure reply: E00'.



[-- Attachment #2: errmsg.txt --]
[-- Type: text/plain, Size: 1969 bytes --]

2009-07-29  Michael Snyder  <msnyder@vmware.com>

	* remote.c (store_register_using_P): Call putpkt and getpkt
	directly instead of calling remote_send.
	(store_register_using_G): Ditto.
	(fetch_register_using_p): Ditto.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.364
diff -u -p -r1.364 remote.c
--- remote.c	14 Jul 2009 21:40:30 -0000	1.364
+++ remote.c	29 Jul 2009 23:52:27 -0000
@@ -4873,7 +4873,8 @@ fetch_register_using_p (struct regcache 
   *p++ = 'p';
   p += hexnumstr (p, reg->pnum);
   *p++ = '\0';
-  remote_send (&rs->buf, &rs->buf_size);
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
 
   buf = rs->buf;
 
@@ -5121,7 +5122,8 @@ remote_prepare_to_store (struct regcache
    packet was not recognized.  */
 
 static int
-store_register_using_P (const struct regcache *regcache, struct packet_reg *reg)
+store_register_using_P (const struct regcache *regcache, 
+			struct packet_reg *reg)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct remote_state *rs = get_remote_state ();
@@ -5141,7 +5143,8 @@ store_register_using_P (const struct reg
   p = buf + strlen (buf);
   regcache_raw_collect (regcache, reg->regnum, regp);
   bin2hex (regp, p, register_size (gdbarch, reg->regnum));
-  remote_send (&rs->buf, &rs->buf_size);
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
 
   switch (packet_ok (rs->buf, &remote_protocol_packets[PACKET_P]))
     {
@@ -5189,7 +5192,10 @@ store_registers_using_G (const struct re
   /* remote_prepare_to_store insures that rsa->sizeof_g_packet gets
      updated.  */
   bin2hex (regs, p, rsa->sizeof_g_packet);
-  remote_send (&rs->buf, &rs->buf_size);
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
+  if (rs->buf[0] == 'E')
+    error (_("Could not write registers"));
 }
 
 /* Store register REGNUM, or all registers if REGNUM == -1, from the contents

Re: [RFC] small change for better error reporting in remote.c

[drow]

On Wed, Jul 29, 2009 at 04:59:36PM -0700, Michael Snyder wrote:
> @@ -5189,7 +5192,10 @@ store_registers_using_G (const struct re
>    /* remote_prepare_to_store insures that rsa->sizeof_g_packet gets
>       updated.  */
>    bin2hex (regs, p, rsa->sizeof_g_packet);
> -  remote_send (&rs->buf, &rs->buf_size);
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf, &rs->buf_size, 0);
> +  if (rs->buf[0] == 'E')
> +    error (_("Could not write registers"));
>  }
>  
>  /* Store register REGNUM, or all registers if REGNUM == -1, from the contents

Can't you use packet_ok like elsewhere?  Otherwise OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] small change for better error reporting in remote.c

[Michael Snyder]

drow@false.org wrote:
> On Wed, Jul 29, 2009 at 04:59:36PM -0700, Michael Snyder wrote:
>> @@ -5189,7 +5192,10 @@ store_registers_using_G (const struct re
>>    /* remote_prepare_to_store insures that rsa->sizeof_g_packet gets
>>       updated.  */
>>    bin2hex (regs, p, rsa->sizeof_g_packet);
>> -  remote_send (&rs->buf, &rs->buf_size);
>> +  putpkt (rs->buf);
>> +  getpkt (&rs->buf, &rs->buf_size, 0);
>> +  if (rs->buf[0] == 'E')
>> +    error (_("Could not write registers"));
>>  }
>>  
>>  /* Store register REGNUM, or all registers if REGNUM == -1, from the contents
> 
> Can't you use packet_ok like elsewhere?  Otherwise OK.

You may not realize it, but you're asking for a much bigger change.
There is no struct packet_config for the 'G' packet.  I'll have to
change set_registers_using_G from void to int, so that it can
return failure if the 'G' packet is unsupported.

But I'll begin working on it, unles I hear "never mind"
from you...   ;-)

Michael

Re: [RFC] small change for better error reporting in remote.c

[Daniel Jacobowitz]

On Fri, Jul 31, 2009 at 01:31:13PM -0700, Michael Snyder wrote:
> You may not realize it, but you're asking for a much bigger change.
> There is no struct packet_config for the 'G' packet.  I'll have to
> change set_registers_using_G from void to int, so that it can
> return failure if the 'G' packet is unsupported.
> 
> But I'll begin working on it, unles I hear "never mind"
> from you...   ;-)

Bah... no, don't bother with that.  What I actually wanted is
just packet_check_result.  Is that easier? :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] small change for better error reporting in remote.c

[Michael Snyder]

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

Daniel Jacobowitz wrote:
> On Fri, Jul 31, 2009 at 01:31:13PM -0700, Michael Snyder wrote:
>> You may not realize it, but you're asking for a much bigger change.
>> There is no struct packet_config for the 'G' packet.  I'll have to
>> change set_registers_using_G from void to int, so that it can
>> return failure if the 'G' packet is unsupported.
>>
>> But I'll begin working on it, unles I hear "never mind"
>> from you...   ;-)
> 
> Bah... no, don't bother with that.  What I actually wanted is
> just packet_check_result.  Is that easier? :-) 

Yep, checked in as attached.


[-- Attachment #2: errmsg.txt --]
[-- Type: text/plain, Size: 1997 bytes --]

2009-07-29  Michael Snyder  <msnyder@vmware.com>

	* remote.c (store_register_using_P): Call putpkt and getpkt
	directly instead of calling remote_send.
	(store_register_using_G): Ditto.
	(fetch_register_using_p): Ditto.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.364
diff -u -p -r1.364 remote.c
--- remote.c	14 Jul 2009 21:40:30 -0000	1.364
+++ remote.c	31 Jul 2009 21:31:34 -0000
@@ -4873,7 +4873,8 @@ fetch_register_using_p (struct regcache 
   *p++ = 'p';
   p += hexnumstr (p, reg->pnum);
   *p++ = '\0';
-  remote_send (&rs->buf, &rs->buf_size);
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
 
   buf = rs->buf;
 
@@ -5121,7 +5122,8 @@ remote_prepare_to_store (struct regcache
    packet was not recognized.  */
 
 static int
-store_register_using_P (const struct regcache *regcache, struct packet_reg *reg)
+store_register_using_P (const struct regcache *regcache, 
+			struct packet_reg *reg)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct remote_state *rs = get_remote_state ();
@@ -5141,7 +5143,8 @@ store_register_using_P (const struct reg
   p = buf + strlen (buf);
   regcache_raw_collect (regcache, reg->regnum, regp);
   bin2hex (regp, p, register_size (gdbarch, reg->regnum));
-  remote_send (&rs->buf, &rs->buf_size);
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
 
   switch (packet_ok (rs->buf, &remote_protocol_packets[PACKET_P]))
     {
@@ -5189,7 +5192,10 @@ store_registers_using_G (const struct re
   /* remote_prepare_to_store insures that rsa->sizeof_g_packet gets
      updated.  */
   bin2hex (regs, p, rsa->sizeof_g_packet);
-  remote_send (&rs->buf, &rs->buf_size);
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
+  if (packet_check_result (rs->buf) == PACKET_ERROR)
+    error (_("Could not write registers"));
 }
 
 /* Store register REGNUM, or all registers if REGNUM == -1, from the contents

Re: [RFC] small change for better error reporting in remote.c

[Pedro Alves]

On Friday 31 July 2009 21:31:13, Michael Snyder wrote:
> drow@false.org wrote:
> > On Wed, Jul 29, 2009 at 04:59:36PM -0700, Michael Snyder wrote:
> >> @@ -5189,7 +5192,10 @@ store_registers_using_G (const struct re
> >>    /* remote_prepare_to_store insures that rsa->sizeof_g_packet gets
> >>       updated.  */
> >>    bin2hex (regs, p, rsa->sizeof_g_packet);
> >> -  remote_send (&rs->buf, &rs->buf_size);
> >> +  putpkt (rs->buf);
> >> +  getpkt (&rs->buf, &rs->buf_size, 0);
> >> +  if (rs->buf[0] == 'E')
> >> +    error (_("Could not write registers"));

Can you still include the remote error string in this message?
Otherwise, this loses the "feature" of being able to send
more verbose error down the pipe.  E.g., notice:

static enum packet_result
packet_check_result (const char *buf)
{
  if (buf[0] != '\0')
    {
      /* The stub recognized the packet request.  Check that the
	 operation succeeded.  */
      if (buf[0] == 'E'
	  && isxdigit (buf[1]) && isxdigit (buf[2])
	  && buf[3] == '\0')
	/* "Enn"  - definitly an error.  */
	return PACKET_ERROR;

      /* Always treat "E." as an error.  This will be used for
	 more verbose error messages, such as E.memtypes.  */
      if (buf[0] == 'E' && buf[1] == '.')
	return PACKET_ERROR;

      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

      /* The packet may or may not be OK.  Just assume it is.  */
      return PACKET_OK;
    }
  else
    /* The stub does not support the packet.  */
    return PACKET_UNKNOWN;
}


> >>  }
> >>  
> >>  /* Store register REGNUM, or all registers if REGNUM == -1, from the contents
> > 
> > Can't you use packet_ok like elsewhere?  Otherwise OK.
> 
> You may not realize it, but you're asking for a much bigger change.
> There is no struct packet_config for the 'G' packet.  I'll have to
> change set_registers_using_G from void to int, so that it can
> return failure if the 'G' packet is unsupported.
> 
> But I'll begin working on it, unles I hear "never mind"
> from you...   ;-)

Heh, not sure how useful it would be to have an optional G
currently.  Daniel was probably thinking of packet_check_result.
Maybe you could pick up, integrate and finish this patch
for PR9665?

-- 
Pedro Alves

---
 gdb/remote.c |   64 ++++++++++++++++++++++++-----------------------------------
 1 file changed, 27 insertions(+), 37 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2009-07-31 21:48:21.000000000 +0100
+++ src/gdb/remote.c	2009-07-31 21:49:03.000000000 +0100
@@ -107,8 +107,6 @@ static void extended_remote_mourn (struc
 
 static void remote_mourn_1 (struct target_ops *);
 
-static void remote_send (char **buf, long *sizeof_buf_p);
-
 static int readchar (int timeout);
 
 static void remote_kill (struct target_ops *ops);
@@ -2423,11 +2421,12 @@ get_offsets (void)
   getpkt (&rs->buf, &rs->buf_size, 0);
   buf = rs->buf;
 
-  if (buf[0] == '\000')
-    return;			/* Return silently.  Stub doesn't support
-				   this command.  */
-  if (buf[0] == 'E')
+  switch (packet_check_result (buf))
     {
+    case PACKET_UNKNOWN:
+      /* Return silently.  Stub doesn't support this command.  */
+      return;
+    case PACKET_ERROR:
       warning (_("Remote failure reply: %s"), buf);
       return;
     }
@@ -4873,7 +4872,9 @@ fetch_register_using_p (struct regcache 
   *p++ = 'p';
   p += hexnumstr (p, reg->pnum);
   *p++ = '\0';
-  remote_send (&rs->buf, &rs->buf_size);
+
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
 
   buf = rs->buf;
 
@@ -4921,7 +4922,12 @@ send_g_packet (void)
   char *regs;
 
   sprintf (rs->buf, "g");
-  remote_send (&rs->buf, &rs->buf_size);
+
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
+
+  if (packet_check_result (rs->buf) == PACKET_ERROR)
+    error (_("Remote failure reply: %s"), rs->buf);
 
   /* We can get out of synch in various cases.  If the first character
      in the buffer is not a hex character, assume that has happened
@@ -5141,7 +5147,9 @@ store_register_using_P (const struct reg
   p = buf + strlen (buf);
   regcache_raw_collect (regcache, reg->regnum, regp);
   bin2hex (regp, p, register_size (gdbarch, reg->regnum));
-  remote_send (&rs->buf, &rs->buf_size);
+
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
 
   switch (packet_ok (rs->buf, &remote_protocol_packets[PACKET_P]))
     {
@@ -5189,7 +5197,10 @@ store_registers_using_G (const struct re
   /* remote_prepare_to_store insures that rsa->sizeof_g_packet gets
      updated.  */
   bin2hex (regs, p, rsa->sizeof_g_packet);
-  remote_send (&rs->buf, &rs->buf_size);
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
+  if (packet_check_result (rs->buf) == PACKET_ERROR)
+    error (_("Remote failure reply: %s"), rs->buf);
 }
 
 /* Store register REGNUM, or all registers if REGNUM == -1, from the contents
@@ -5600,7 +5611,7 @@ remote_write_bytes_aux (const char *head
   putpkt_binary (rs->buf, (int) (p - rs->buf));
   getpkt (&rs->buf, &rs->buf_size, 0);
 
-  if (rs->buf[0] == 'E')
+  if (packet_check_result (rs->buf) == PACKET_ERROR)
     {
       /* There is no correspondance between what the remote protocol
 	 uses for errors and errno codes.  We would like a cleaner way
@@ -5702,9 +5713,7 @@ remote_read_bytes (CORE_ADDR memaddr, gd
       putpkt (rs->buf);
       getpkt (&rs->buf, &rs->buf_size, 0);
 
-      if (rs->buf[0] == 'E'
-	  && isxdigit (rs->buf[1]) && isxdigit (rs->buf[2])
-	  && rs->buf[3] == '\0')
+      if (packet_check_result (rs->buf) == PACKET_ERROR)
 	{
 	  /* There is no correspondance between what the remote
 	     protocol uses for errors and errno codes.  We would like
@@ -5935,22 +5944,6 @@ readchar (int timeout)
   return ch;
 }
 
-/* Send the command in *BUF to the remote machine, and read the reply
-   into *BUF.  Report an error if we get an error reply.  Resize
-   *BUF using xrealloc if necessary to hold the result, and update
-   *SIZEOF_BUF.  */
-
-static void
-remote_send (char **buf,
-	     long *sizeof_buf)
-{
-  putpkt (*buf);
-  getpkt (buf, sizeof_buf, 0);
-
-  if ((*buf)[0] == 'E')
-    error (_("Remote failure reply: %s"), *buf);
-}
-
 /* Return a pointer to an xmalloc'ed string representing an escaped
    version of BUF, of len N.  E.g. \n is converted to \\n, \t to \\t,
    etc.  The caller is responsible for releasing the returned
@@ -6879,7 +6872,7 @@ remote_remove_breakpoint (struct gdbarch
       putpkt (rs->buf);
       getpkt (&rs->buf, &rs->buf_size, 0);
 
-      return (rs->buf[0] == 'E');
+      return (packet_check_result (rs->buf) == PACKET_ERROR);
     }
 
   return memory_remove_breakpoint (gdbarch, bp_tgt);
@@ -7186,7 +7179,7 @@ compare_sections_command (char *args, in
       host_crc = crc32 ((unsigned char *) sectdata, size, 0xffffffff);
 
       getpkt (&rs->buf, &rs->buf_size, 0);
-      if (rs->buf[0] == 'E')
+      if (packet_check_result (rs->buf) == PACKET_ERROR)
 	error (_("target memory fault, section %s, range %s -- %s"), sectname,
 	       paddress (target_gdbarch, lma),
 	       paddress (target_gdbarch, lma + size));
@@ -7645,11 +7638,8 @@ remote_rcmd (char *command,
 	}
       if (strcmp (buf, "OK") == 0)
 	break;
-      if (strlen (buf) == 3 && buf[0] == 'E'
-	  && isdigit (buf[1]) && isdigit (buf[2]))
-	{
-	  error (_("Protocol error with Rcmd"));
-	}
+      if (packet_check_result (buf) == PACKET_ERROR)
+	error (_("Protocol error with Rcmd"));
       for (p = buf; p[0] != '\0' && p[1] != '\0'; p += 2)
 	{
 	  char c = (fromhex (p[0]) << 4) + fromhex (p[1]);

Re: [RFC] small change for better error reporting in remote.c

[Michael Snyder]

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

Pedro Alves wrote:
> On Friday 31 July 2009 21:31:13, Michael Snyder wrote:
>> drow@false.org wrote:
>>> On Wed, Jul 29, 2009 at 04:59:36PM -0700, Michael Snyder wrote:
>>>> @@ -5189,7 +5192,10 @@ store_registers_using_G (const struct re
>>>>    /* remote_prepare_to_store insures that rsa->sizeof_g_packet gets
>>>>       updated.  */
>>>>    bin2hex (regs, p, rsa->sizeof_g_packet);
>>>> -  remote_send (&rs->buf, &rs->buf_size);
>>>> +  putpkt (rs->buf);
>>>> +  getpkt (&rs->buf, &rs->buf_size, 0);
>>>> +  if (rs->buf[0] == 'E')
>>>> +    error (_("Could not write registers"));
> 
> Can you still include the remote error string in this message?
> Otherwise, this loses the "feature" of being able to send
> more verbose error down the pipe.  

Darn, crossed messages.

How's this, Pedro?


[-- Attachment #2: errmsg2.txt --]
[-- Type: text/plain, Size: 1592 bytes --]

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.365
diff -u -p -r1.365 remote.c
--- remote.c	31 Jul 2009 21:33:39 -0000	1.365
+++ remote.c	31 Jul 2009 21:41:36 -0000
@@ -4885,8 +4885,10 @@ fetch_register_using_p (struct regcache 
     case PACKET_UNKNOWN:
       return 0;
     case PACKET_ERROR:
-      error (_("Could not fetch register \"%s\""),
-	     gdbarch_register_name (get_regcache_arch (regcache), reg->regnum));
+      error (_("Could not fetch register \"%s\"; remote failure reply '%s'"),
+	     gdbarch_register_name (get_regcache_arch (regcache), 
+				    reg->regnum), 
+	     buf);
     }
 
   /* If this register is unfetchable, tell the regcache.  */
@@ -5151,8 +5153,8 @@ store_register_using_P (const struct reg
     case PACKET_OK:
       return 1;
     case PACKET_ERROR:
-      error (_("Could not write register \"%s\""),
-	     gdbarch_register_name (gdbarch, reg->regnum));
+      error (_("Could not write register \"%s\"; remote failure reply '%s'"),
+	     gdbarch_register_name (gdbarch, reg->regnum), rs->buf);
     case PACKET_UNKNOWN:
       return 0;
     default:
@@ -5195,7 +5197,8 @@ store_registers_using_G (const struct re
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
   if (packet_check_result (rs->buf) == PACKET_ERROR)
-    error (_("Could not write registers"));
+    error (_("Could not write registers; remote failure reply '%s'"), 
+	   rs->buf);
 }
 
 /* Store register REGNUM, or all registers if REGNUM == -1, from the contents

Re: [RFC] small change for better error reporting in remote.c

[Pedro Alves]

On Friday 31 July 2009 22:42:47, Michael Snyder wrote:

> Darn, crossed messages.
> 
> How's this, Pedro?

It's good, thanks!

-- 
Pedro Alves