new gdb remote packet type

new gdb remote packet type

[Robert Picco]

Hi:

I encountered an issue when  attempting to get the kernel debugger (kgdb) running for IA64 on Linux.  
The debugger works fine over ethernet but won't work over a serial line 
because of the size of the g-packet (>10K bytes).  Most registers in the 
g-packet aren't required for kernel debugging.  The patch below
has introduced a new gdb packet which asks the target (kgdb) for the 
register offsets in the g-packet.  In my kernel kgdb stub  all stacked 
registers, nearly all floating point (kernel uses small subset of 
floating point), all predicate registers, all nat registers and most of 
the application registers are excluded.  The patch won't break any 
existing targets of gdb because the new packet won't be recognized.  No other 
architectures (at least those I'm aware of) suffer from such a huge 
g-packet. 

Do you think a solution like this is possible for inclusion into gdb?

thanks,

Bob


--- gdb-6.0-orig/gdb/remote.c	2003-06-30 11:51:49.000000000 -0400
+++ gdb-6.0/gdb/remote.c	2004-04-13 11:46:38.000000000 -0400
@@ -2131,6 +2131,44 @@
   return 1;
 }
 
+static int register_init;
+
+static void
+remote_register_size(void)
+{
+  struct remote_state *rs = get_remote_state ();
+  char *buf = alloca (rs->remote_packet_size);
+  int regnum, offset, size;
+
+  
+  register_init = 1;
+
+  for (size = 0, regnum = 0; regnum < NUM_REGS + NUM_PSEUDO_REGS; regnum++)
+    {
+        struct packet_reg *r = &rs->regs[regnum];
+	buf[0] = 'r';
+	bin2hex ((char *) &regnum, &buf[1], sizeof(regnum));
+  	putpkt(buf);
+	getpkt (buf, (rs->remote_packet_size), 0);
+	if (buf[0] != 'r') {
+		if (regnum)
+			fprintf_unfiltered (gdb_stdlog, 
+				"Received some valid register packets and later failed\n");
+		return;
+	}
+        hex2bin(&buf[1], (char *)&offset, sizeof(offset));
+	r->offset = offset;
+	if (offset != -1) {
+		r->in_g_packet = 1;
+		size += register_size (current_gdbarch, regnum);
+	}
+	else
+		r->in_g_packet = 0;
+
+    }
+    rs->sizeof_g_packet = size;
+}
+
 static int
 remote_start_remote (struct ui_out *uiout, void *dummy)
 {
@@ -2139,9 +2177,13 @@
   /* Ack any packet which the remote side has already sent.  */
   serial_write (remote_desc, "+", 1);
 
+
   /* Let the stub know that we want it to return the thread.  */
   set_thread (-1, 0);
 
+  if (!register_init)
+	remote_register_size();
+
   inferior_ptid = remote_current_thread (inferior_ptid);
 
   get_offsets ();		/* Get text, data & bss offsets */
@@ -3438,8 +3480,8 @@
       gdb_assert (reg != NULL);
       if (!reg->in_g_packet)
 	internal_error (__FILE__, __LINE__,
-			"Attempt to fetch a non G-packet register when this "
-			"remote.c does not support the p-packet.");
+			"Attempt to fetch a non G-packet register (%d) when this "
+			"remote.c does not support the p-packet.", regnum);
     }
 
   sprintf (buf, "g");

Re: new gdb remote packet type

[Andrew Cagney]

I'd investigate the qPart packet:
http://sources.redhat.com/gdb/current/onlinedocs/gdb_33.html#SEC658
and how that, along with xfer_partial, can be extended to fetch an 
arbitrary part of /proc/PID/registers.

Andrew

Re: new gdb remote packet type

[Robert Picco]

Andrew Cagney wrote:

> I'd investigate the qPart packet:
> http://sources.redhat.com/gdb/current/onlinedocs/gdb_33.html#SEC658
> and how that, along with xfer_partial, can be extended to fetch an 
> arbitrary part of /proc/PID/registers.
>
> Andrew
>
So you aren't objecting to modifying what is sent in the g-packet?  
Instead of defining a new "r" packet, you'd prefer that I use the 
q-packet mechanism?

thanks,

Bob

Re: new gdb remote packet type

[Andrew Cagney]

>  you'd prefer that I use the q-packet mechanism? 

Yes, the qPart packet was designed for exactly the situtation you 
encountered.  It can specify a length/offset and return a short transfer.

Andrew

Re: new gdb remote packet type

[Robert Picco]

Andrew Cagney wrote:

>>  you'd prefer that I use the q-packet mechanism? 
>
>
> Yes, the qPart packet was designed for exactly the situtation you 
> encountered.  It can specify a length/offset and return a short transfer.
>
> Andrew
>
>
Hi:

Does this represent what you are expecting of the qPart packet?  I saw 
no reason to export the type because this
packet is only useful in remote.c

thanks,

Bob

--- gdb-6.1-orig/gdb/remote.c	2004-02-25 15:41:00.000000000 -0500
+++ gdb-6.1/gdb/remote.c	2004-04-29 09:16:36.000000000 -0400
@@ -2003,6 +2003,57 @@
   objfile_relocate (symfile_objfile, offs);
 }
 
+static int register_init;
+
+static void
+remote_register_size(void)
+{
+  struct remote_state *rs = get_remote_state ();
+  char *buf = alloca (rs->remote_packet_size);
+  int regnum, offset, size, i, sz = 4;
+
+  
+  register_init = 1;
+
+  for (size = 0, regnum = 0; regnum < NUM_REGS + NUM_PSEUDO_REGS; regnum++)
+    {
+        struct packet_reg *r = &rs->regs[regnum];
+        snprintf (buf, rs->remote_packet_size,
+			"qPart:gpacket:read::");
+	bin2hex((char *) &regnum, &buf[20], sizeof (regnum));
+	buf[20 + sizeof(regnum) * 2] = ',';
+	bin2hex((char *) &sz, &buf[20 + sizeof(regnum) * 2 + 1], sizeof (sz));
+	buf[20 + sizeof(regnum) * 2 + 1 + sizeof(size) * 2] = 0;
+	i = putpkt (buf);
+	if (i < 0)
+		goto bad;
+	buf[0] = '\0';
+	getpkt (buf, rs->remote_packet_size, 0);
+        if (buf[0] == 'O' && buf[1] == 'K' && buf[2] == '\0')
+		goto bad;
+	else if (buf[0] == 'E')
+		goto bad;
+
+        hex2bin(&buf[0], (char *)&offset, sizeof(offset));
+	r->offset = offset;
+	if (offset != -1) {
+		r->in_g_packet = 1;
+		size += register_size (current_gdbarch, regnum);
+	}
+	else
+		r->in_g_packet = 0;
+
+    }
+    rs->sizeof_g_packet = size;
+    return;
+
+bad:
+    if (regnum)
+		fprintf_unfiltered (gdb_stdlog, 
+			"Received some valid register packets and later failed\n");
+    return;
+}
+
 /* Stub for catch_errors.  */
 
 static int
@@ -2025,6 +2076,8 @@
   /* Let the stub know that we want it to return the thread.  */
   set_thread (-1, 0);
 
+  if (!register_init)
+	remote_register_size();
   inferior_ptid = remote_current_thread (inferior_ptid);
 
   get_offsets ();		/* Get text, data & bss offsets */
@@ -3257,8 +3310,8 @@
       gdb_assert (reg != NULL);
       if (!reg->in_g_packet)
 	internal_error (__FILE__, __LINE__,
-			"Attempt to fetch a non G-packet register when this "
-			"remote.c does not support the p-packet.");
+			"Attempt to fetch a non G-packet register (%d) when this "
+			"remote.c does not support the p-packet.", regnum);
     }
 
   sprintf (buf, "g");

Re: new gdb remote packet type

[Andrew Cagney]

> Andrew Cagney wrote:
> 
>>>  you'd prefer that I use the q-packet mechanism? 
>>
>>
>>
>> Yes, the qPart packet was designed for exactly the situtation you encountered.  It can specify a length/offset and return a short transfer.
>>
>> Andrew
>>
>>
> Hi:
> 
> Does this represent what you are expecting of the qPart packet?  I saw no reason to export the type because this
> packet is only useful in remote.c

While not immediatly, it is going to eventually be exposed out side of 
remote.c, so we need to get what goes across the wire right up front. 
To that end:

- use register sets

Unlike the G-packet, where the byte layout is determined by GDB internal 
data structure, the byte layout needs to be specified by the inferior's 
regsets.  That in turn means specifying separate packets for fetching 
all or part of each of "gregs", "fpregs", and "xpregs"(?).

- specify offset/length

I think your code is specifying a register number, it should view the 
region as a sequence of bytes and fetch using offset/length.  This 
information can be obtained using "regset.h" (although that interface 
might need some massaging).

- fetch lazy

Just request the region for the requested register.  It's then up to the 
remote end to determine that more/less of the region can be supplied.

Eventually this will also be added to the target vector, however, for 
the moment something more local is definitly fine.

Andrew

Re: new gdb remote packet type

[Robert Picco]

Andrew Cagney wrote:

>> Andrew Cagney wrote:
>>
>>>>  you'd prefer that I use the q-packet mechanism? 
>>>
>>>
>>>
>>>
>>> Yes, the qPart packet was designed for exactly the situtation you 
>>> encountered.  It can specify a length/offset and return a short 
>>> transfer.
>>>
>>> Andrew
>>>
>>>
>> Hi:
>>
>> Does this represent what you are expecting of the qPart packet?  I 
>> saw no reason to export the type because this
>> packet is only useful in remote.c
>
>
> While not immediatly, it is going to eventually be exposed out side of 
> remote.c, so we need to get what goes across the wire right up front. 
> To that end:
>
> - use register sets
>
> Unlike the G-packet, where the byte layout is determined by GDB 
> internal data structure, the byte layout needs to be specified by the 
> inferior's regsets.  That in turn means specifying separate packets 
> for fetching all or part of each of "gregs", "fpregs", and "xpregs"(?).
>
> - specify offset/length
>
> I think your code is specifying a register number, it should view the 
> region as a sequence of bytes and fetch using offset/length.  This 
> information can be obtained using "regset.h" (although that interface 
> might need some massaging).
>
> - fetch lazy
>
> Just request the region for the requested register.  It's then up to 
> the remote end to determine that more/less of the region can be supplied.
>
> Eventually this will also be added to the target vector, however, for 
> the moment something more local is definitly fine.
>
> Andrew
>
>
Hi:

Well I must admit to being a little confused:)  My intent is to not 
introduce any architecture dependency in remote.c and just restructure 
the g-packet to match what the target wants.  I could be missing a point 
here but the regset stuff looks very architecture dependent and it would 
seem incorrect to use it in remote.c. 

Well for offset I am using the regnum.  We are inquirying whether the 
target wants the reg mapped in the g-packet .

We could fetch lazy but this isn't currently done by remote.c because 
most target g-packet payloads aren't very large. IA64 is the exception 
with a payload in excess of 10,000 bytes. 

I must be missing your larger objective here because  my limited 
knowledge of gdb is in remote.c.

thanks,

Bob

Re: new gdb remote packet type

[Andrew Cagney]

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

>>>> Yes, the qPart packet was designed for exactly the situtation you encountered.  It can specify a length/offset and return a short transfer.
>>>>
>>>> Andrew
>>>>
>>>>
>>> Hi:
>>>
>>> Does this represent what you are expecting of the qPart packet?  I saw no reason to export the type because this
>>> packet is only useful in remote.c
>>
>>
>>
>> While not immediatly, it is going to eventually be exposed out side of remote.c, so we need to get what goes across the wire right up front. To that end:
>>
>> - use register sets
>>
>> Unlike the G-packet, where the byte layout is determined by GDB internal data structure, the byte layout needs to be specified by the inferior's regsets.  That in turn means specifying separate packets for fetching all or part of each of "gregs", "fpregs", and "xpregs"(?).
>>
>> - specify offset/length
>>
>> I think your code is specifying a register number, it should view the region as a sequence of bytes and fetch using offset/length.  This information can be obtained using "regset.h" (although that interface might need some massaging).
>>
>> - fetch lazy
>>
>> Just request the region for the requested register.  It's then up to the remote end to determine that more/less of the region can be supplied.
>>
>> Eventually this will also be added to the target vector, however, for the moment something more local is definitly fine.
>>
>> Andrew
>>
>>
> Hi:
> 
> Well I must admit to being a little confused:)  My intent is to not introduce any architecture dependency in remote.c and just restructure the g-packet to match what the target wants.  I could be missing a point here but the regset stuff looks very architecture dependent and it would seem incorrect to use it in remote.c.
> Well for offset I am using the regnum.  We are inquirying whether the target wants the reg mapped in the g-packet .

There are two, equally correct, ways to view the registers being fetched:

- as array of values indexed by register number
- as flat byte addressable region with implied format

The current G packet format uses the latter, badly.  The implied format 
is determined somewhat arbitrarially by GDB-internals.  The qPart I'm 
suggesting, while still viewing registers as a flat sequence of bytes, 
is at least citing /proc and regsets for that bytes format (and a 
standard GDB is expected to understand anyway).

One way to think of qPart is that it provides a mechanism for accessing 
a number of separate address spaces - memory, gregset, auxv, etc.  For 
each, it always specifies an offset/length and gets back at least part 
of that request.  Passing a register index, doesn't fit into that model.

> We could fetch lazy but this isn't currently done by remote.c because most target g-packet payloads aren't very large. IA64 is the exception with a payload in excess of 10,000 bytes.
> I must be missing your larger objective here because  my limited knowledge of gdb is in remote.c.

It's still large enough to hurt - people are still debugging across very 
  low bandwidth links.

Anyway, it sounds like you might (dig dig) be more interested  in the 
attached patch+hack (fernando nasser gets credit for the patch, I deny 
responsibility for the hack bit :-).  It adds a P packet for fetching 
individual registers which is something the existing remote protocol 
spec doco hints at.

Andrew



[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2942 bytes --]

--- /home/scratch/GDB/src/gdb/remote.c	Wed Apr 28 09:15:50 2004
+++ remote.c	Wed May  5 13:32:31 2004
@@ -267,7 +267,11 @@
       r->pnum = regnum;
       r->regnum = regnum;
       r->offset = DEPRECATED_REGISTER_BYTE (regnum);
+#ifdef GPACKET_UPPER_BOUND_HACK
+      r->in_g_packet = (regnum < GPACKET_UPPER_BOUND_HACK (gdbarch));
+#else
       r->in_g_packet = (regnum < NUM_REGS);
+#endif
       /* ...name = REGISTER_NAME (regnum); */
 
       /* Compute packet size by accumulating the size of all registers. */
@@ -3240,6 +3244,65 @@
 
 static int register_bytes_found;
 
+/* Helper for remote_fetch_registers.  Fetch a register using the
+   ``p'' packet.  */
+
+static void
+fetch_register_using_p (int regnum)
+{
+  struct remote_state *rs = get_remote_state ();
+  char *regs;
+  char *buf;
+  int i;
+  char *p;
+
+  buf = alloca (rs->remote_packet_size);
+  sprintf (buf, "p%x", regnum);
+  remote_send (buf, rs->remote_packet_size);
+
+  /* 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
+     and try to fetch another packet to read.  */
+  while ((buf[0] < '0' || buf[0] > '9')
+	 && (buf[0] < 'a' || buf[0] > 'f')
+	 && buf[0] != 'x')	/* New: unavailable register value */
+    {
+      if (remote_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "Bad register packet; fetching a new packet\n");
+      getpkt (buf, rs->remote_packet_size, 0);
+    }
+
+  /* Reply describes registers byte by byte, each byte encoded as two
+     hex characters.  Suck them all up, then supply them to the
+     register cacheing/storage mechanism.  */
+
+  regs = alloca (DEPRECATED_REGISTER_RAW_SIZE (regnum));
+  memset (regs, DEPRECATED_REGISTER_RAW_SIZE (regnum), 0);
+  p = buf;
+  for (i = 0; i < DEPRECATED_REGISTER_RAW_SIZE (regnum); i++)
+    {
+      if (p[0] == 0)
+	break;
+      if (p[1] == 0)
+	{
+	  warning ("Remote reply is of odd length: %s", buf);
+	  /* Don't change register_bytes_found in this case, and don't
+	     print a second warning.  */
+	  break;
+	}
+      if (p[0] == 'x' && p[1] == 'x')
+	regs[i] = 0;		/* 'x' */
+      else
+	regs[i] = fromhex (p[0]) * 16 + fromhex (p[1]);
+      p += 2;
+    }
+
+  supply_register (regnum, &regs[0]);
+  if (buf[0] == 'x')
+    deprecated_register_valid[i] = -1;	/* register value not available */
+}
+
 /* Read the remote registers into the block REGS.  */
 /* Currently we just read all the registers, so we don't use regnum.  */
 
@@ -3258,10 +3321,18 @@
     {
       struct packet_reg *reg = packet_reg_from_regnum (rs, regnum);
       gdb_assert (reg != NULL);
+#if 0
       if (!reg->in_g_packet)
 	internal_error (__FILE__, __LINE__,
 			"Attempt to fetch a non G-packet register when this "
 			"remote.c does not support the p-packet.");
+#else
+      if (!reg->in_g_packet)
+	{
+	  fetch_register_using_p (regnum);
+	  return;
+	}
+#endif
     }
 
   sprintf (buf, "g");

Re: new gdb remote packet type

[Robert Picco]

Andrew Cagney wrote:

>
> Anyway, it sounds like you might (dig dig) be more interested  in the 
> attached patch+hack (fernando nasser gets credit for the patch, I deny 
> responsibility for the hack bit :-).  It adds a P packet for fetching 
> individual registers which is something the existing remote protocol 
> spec doco hints at.
>
> Andrew

Seems remote.c currently supports "P" packet for storing a single 
register would you find a patch supporting the "p" packet for fetching a 
single register to be acceptable?  This would put the minimum payload on 
the wire and complement the "P" packet.   It's similar to the patch 
(hack) you forwarded but a complete implementation.

Bob

*** gdb-6.1-orig/gdb/remote.c	2004-02-25 15:41:00.000000000 -0500
--- gdb-6.1/gdb/remote.c	2004-05-06 14:05:47.000000000 -0400
***************
*** 809,814 ****
--- 809,831 ----
    show_packet_config_cmd (&remote_protocol_E);
  }
  
+ static struct packet_config remote_protocol_p;
+ 
+ static void
+ set_remote_protocol_p_packet_cmd (char *args, int from_tty,
+ 				  struct cmd_list_element *c)
+ {
+   update_packet_config (&remote_protocol_p);
+ }
+ 
+ static void
+ show_remote_protocol_p_packet_cmd (char *args, int from_tty,
+ 				   struct cmd_list_element *c)
+ {
+   show_packet_config_cmd (&remote_protocol_p);
+ }
+ 
+ 
  
  /* Should we try the 'P' (set register) request?  */
  
***************
*** 2080,2085 ****
--- 2097,2103 ----
    update_packet_config (&remote_protocol_e);
    update_packet_config (&remote_protocol_E);
    update_packet_config (&remote_protocol_P);
+   update_packet_config (&remote_protocol_p);
    update_packet_config (&remote_protocol_qSymbol);
    update_packet_config (&remote_protocol_vcont);
    for (i = 0; i < NR_Z_PACKET_TYPES; i++)
***************
*** 3240,3245 ****
--- 3258,3293 ----
  /* Read the remote registers into the block REGS.  */
  /* Currently we just read all the registers, so we don't use regnum.  */
  
+ static int
+ fetch_register_using_p (int regnum)
+ {
+   struct remote_state *rs = get_remote_state ();
+   char *buf = alloca (rs->remote_packet_size), *p;
+   char regp[MAX_REGISTER_SIZE];
+   int i;
+ 
+   buf[0] = 'p';
+   bin2hex((char *) &regnum, &buf[1], sizeof(regnum));
+   buf[9] = 0;
+   remote_send (buf, rs->remote_packet_size);
+   if (buf[0] != 0 && buf[0] != 'E') {
+      p = buf;
+      i = 0;
+      while (p[0] != 0) {
+ 	if (p[1] == 0) {
+ 		error("fetch_register_using_p: early buf termination");
+ 		return 0;
+ 	}
+ 	regp[i++] = fromhex (p[0]) * 16 + fromhex (p[1]);
+         p += 2;
+     }
+     regcache_raw_supply (current_regcache, regnum, regp);
+     return 1;
+  }
+ 
+  return 0;
+ }
+ 
  static void
  remote_fetch_registers (int regnum)
  {
***************
*** 3260,3265 ****
--- 3308,3338 ----
  			"Attempt to fetch a non G-packet register when this "
  			"remote.c does not support the p-packet.");
      }
+       switch (remote_protocol_p.support)
+ 	{
+ 	case PACKET_DISABLE:
+ 	  break;
+ 	case PACKET_ENABLE:
+ 	  if (fetch_register_using_p (regnum))
+ 	    return;
+ 	  else
+ 	    error ("Protocol error: p packet not recognized by stub");
+ 	case PACKET_SUPPORT_UNKNOWN:
+ 	  if (fetch_register_using_p (regnum))
+ 	    {
+ 	      /* The stub recognized the 'p' packet.  Remember this.  */
+ 	      remote_protocol_p.support = PACKET_ENABLE;
+ 	      return;
+ 	    }
+ 	  else
+ 	    {
+ 	      /* The stub does not support the 'P' packet.  Use 'G'
+ 	         instead, and don't try using 'P' in the future (it
+ 	         will just waste our time).  */
+ 	      remote_protocol_p.support = PACKET_DISABLE;
+ 	      break;
+ 	    }
+ 	}
  
    sprintf (buf, "g");
    remote_send (buf, (rs->remote_packet_size));
***************
*** 5419,5424 ****
--- 5492,5498 ----
    show_remote_protocol_e_packet_cmd (args, from_tty, NULL);
    show_remote_protocol_E_packet_cmd (args, from_tty, NULL);
    show_remote_protocol_P_packet_cmd (args, from_tty, NULL);
+   show_remote_protocol_p_packet_cmd (args, from_tty, NULL);
    show_remote_protocol_qSymbol_packet_cmd (args, from_tty, NULL);
    show_remote_protocol_vcont_packet_cmd (args, from_tty, NULL);
    show_remote_protocol_binary_download_cmd (args, from_tty, NULL);
***************
*** 5631,5636 ****
--- 5705,5717 ----
  			 &remote_set_cmdlist, &remote_show_cmdlist,
  			 1);
  
+   add_packet_config_cmd (&remote_protocol_p,
+ 			 "p", "fetch-register",
+ 			 set_remote_protocol_p_packet_cmd,
+ 			 show_remote_protocol_p_packet_cmd,
+ 			 &remote_set_cmdlist, &remote_show_cmdlist,
+ 			 1);
+ 
    add_packet_config_cmd (&remote_protocol_Z[Z_PACKET_SOFTWARE_BP],
  			 "Z0", "software-breakpoint",
  			 set_remote_protocol_Z_software_bp_packet_cmd,

Re: new gdb remote packet type

[Robert Picco]

Andrew have you had an opportunity to review this?

thanks,

Bob
Robert Picco wrote:
Andrew Cagney wrote:

Anyway, it sounds like you might (dig dig) be more interested  in the 
attached patch+hack (fernando nasser gets credit for the patch, I 
deny responsibility for the hack bit :-).  It adds a P packet for 
fetching individual registers which is something the existing remote 
protocol spec doco hints at.

Andrew


Seems remote.c currently supports "P" packet for storing a single 
register would you find a patch supporting the "p" packet for fetching 
a single register to be acceptable?  This would put the minimum 
payload on the wire and complement the "P" packet.   It's similar to 
the patch (hack) you forwarded but a complete implementation.

Bob

*** gdb-6.1-orig/gdb/remote.c    2004-02-25 15:41:00.000000000 -0500
--- gdb-6.1/gdb/remote.c    2004-05-06 14:05:47.000000000 -0400
***************
*** 809,814 ****
--- 809,831 ----
   show_packet_config_cmd (&remote_protocol_E);
 }
 
+ static struct packet_config remote_protocol_p;
+ + static void
+ set_remote_protocol_p_packet_cmd (char *args, int from_tty,
+                   struct cmd_list_element *c)
+ {
+   update_packet_config (&remote_protocol_p);
+ }
+ + static void
+ show_remote_protocol_p_packet_cmd (char *args, int from_tty,
+                    struct cmd_list_element *c)
+ {
+   show_packet_config_cmd (&remote_protocol_p);
+ }
+ +  
 /* Should we try the 'P' (set register) request?  */
 
***************
*** 2080,2085 ****
--- 2097,2103 ----
   update_packet_config (&remote_protocol_e);
   update_packet_config (&remote_protocol_E);
   update_packet_config (&remote_protocol_P);
+   update_packet_config (&remote_protocol_p);
   update_packet_config (&remote_protocol_qSymbol);
   update_packet_config (&remote_protocol_vcont);
   for (i = 0; i < NR_Z_PACKET_TYPES; i++)
***************
*** 3240,3245 ****
--- 3258,3293 ----
 /* Read the remote registers into the block REGS.  */
 /* Currently we just read all the registers, so we don't use regnum.  */
 
+ static int
+ fetch_register_using_p (int regnum)
+ {
+   struct remote_state *rs = get_remote_state ();
+   char *buf = alloca (rs->remote_packet_size), *p;
+   char regp[MAX_REGISTER_SIZE];
+   int i;
+ +   buf[0] = 'p';
+   bin2hex((char *) &regnum, &buf[1], sizeof(regnum));
+   buf[9] = 0;
+   remote_send (buf, rs->remote_packet_size);
+   if (buf[0] != 0 && buf[0] != 'E') {
+      p = buf;
+      i = 0;
+      while (p[0] != 0) {
+     if (p[1] == 0) {
+         error("fetch_register_using_p: early buf termination");
+         return 0;
+     }
+     regp[i++] = fromhex (p[0]) * 16 + fromhex (p[1]);
+         p += 2;
+     }
+     regcache_raw_supply (current_regcache, regnum, regp);
+     return 1;
+  }
+ +  return 0;
+ }
+  static void
 remote_fetch_registers (int regnum)
 {
***************
*** 3260,3265 ****
--- 3308,3338 ----
             "Attempt to fetch a non G-packet register when this "
             "remote.c does not support the p-packet.");
     }
+       switch (remote_protocol_p.support)
+     {
+     case PACKET_DISABLE:
+       break;
+     case PACKET_ENABLE:
+       if (fetch_register_using_p (regnum))
+         return;
+       else
+         error ("Protocol error: p packet not recognized by stub");
+     case PACKET_SUPPORT_UNKNOWN:
+       if (fetch_register_using_p (regnum))
+         {
+           /* The stub recognized the 'p' packet.  Remember this.  */
+           remote_protocol_p.support = PACKET_ENABLE;
+           return;
+         }
+       else
+         {
+           /* The stub does not support the 'P' packet.  Use 'G'
+              instead, and don't try using 'P' in the future (it
+              will just waste our time).  */
+           remote_protocol_p.support = PACKET_DISABLE;
+           break;
+         }
+     }
 
   sprintf (buf, "g");
   remote_send (buf, (rs->remote_packet_size));
***************
*** 5419,5424 ****
--- 5492,5498 ----
   show_remote_protocol_e_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_E_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_P_packet_cmd (args, from_tty, NULL);
+   show_remote_protocol_p_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_qSymbol_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_vcont_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_binary_download_cmd (args, from_tty, NULL);
***************
*** 5631,5636 ****
--- 5705,5717 ----
              &remote_set_cmdlist, &remote_show_cmdlist,
              1);
 
+   add_packet_config_cmd (&remote_protocol_p,
+              "p", "fetch-register",
+              set_remote_protocol_p_packet_cmd,
+              show_remote_protocol_p_packet_cmd,
+              &remote_set_cmdlist, &remote_show_cmdlist,
+              1);
+    add_packet_config_cmd (&remote_protocol_Z[Z_PACKET_SOFTWARE_BP],
              "Z0", "software-breakpoint",
              set_remote_protocol_Z_software_bp_packet_cmd,

Re: new gdb remote packet type

[Andrew Cagney]

Yes, this is the key, use it conditionally, nice.

+       switch (remote_protocol_p.support)
+     {
+     case PACKET_DISABLE:
+       break;
+     case PACKET_ENABLE:
+       if (fetch_register_using_p (regnum))
+         return;
+       else
+         error ("Protocol error: p packet not recognized by stub");
+     case PACKET_SUPPORT_UNKNOWN:
+       if (fetch_register_using_p (regnum))
+         {
+           /* The stub recognized the 'p' packet.  Remember this.  */
+           remote_protocol_p.support = PACKET_ENABLE;
+           return;
+         }
+       else
+         {
+           /* The stub does not support the 'P' packet.  Use 'G'
+              instead, and don't try using 'P' in the future (it
+              will just waste our time).  */
+           remote_protocol_p.support = PACKET_DISABLE;
+           break;
+         }
+     }
 
   sprintf (buf, "g"); 
To get this in, there's some additional leg work:

- copyright assignment?

- documentation (see gdb/doc/gdb.texinfo near the end)

- a corresponding gdbserver patch

- not to forget, ChangeLogs :-)

Andrew

Re: new gdb remote packet type

[Daniel Jacobowitz]

On Wed, May 12, 2004 at 02:20:36PM -0400, Andrew Cagney wrote:
> Yes, this is the key, use it conditionally, nice.
> 
> >+       switch (remote_protocol_p.support)
> >+     {
> >+     case PACKET_DISABLE:
> >+       break;
> >+     case PACKET_ENABLE:
> >+       if (fetch_register_using_p (regnum))
> >+         return;
> >+       else
> >+         error ("Protocol error: p packet not recognized by stub");
> >+     case PACKET_SUPPORT_UNKNOWN:
> >+       if (fetch_register_using_p (regnum))
> >+         {
> >+           /* The stub recognized the 'p' packet.  Remember this.  */
> >+           remote_protocol_p.support = PACKET_ENABLE;
> >+           return;
> >+         }
> >+       else
> >+         {
> >+           /* The stub does not support the 'P' packet.  Use 'G'
> >+              instead, and don't try using 'P' in the future (it
> >+              will just waste our time).  */
> >+           remote_protocol_p.support = PACKET_DISABLE;
> >+           break;
> >+         }
> >+     }
> > 
> >   sprintf (buf, "g"); 

This patch (if 'p' were implemented for gdbserver; I have this lying
around, as it happens) would make register fetches default to using
individual 'p' packets for every register; this would hurt latency, a
lot.

Robert, wouldn't it be good enough for you to work with
!reg->in_g_packet?

-- 
Daniel Jacobowitz

Re: new gdb remote packet type

[Robert Picco]

Daniel Jacobowitz wrote:

On Wed, May 12, 2004 at 02:20:36PM -0400, Andrew Cagney wrote:
 

Yes, this is the key, use it conditionally, nice.

   

+       switch (remote_protocol_p.support)
+     {
+     case PACKET_DISABLE:
+       break;
+     case PACKET_ENABLE:
+       if (fetch_register_using_p (regnum))
+         return;
+       else
+         error ("Protocol error: p packet not recognized by stub");
+     case PACKET_SUPPORT_UNKNOWN:
+       if (fetch_register_using_p (regnum))
+         {
+           /* The stub recognized the 'p' packet.  Remember this.  */
+           remote_protocol_p.support = PACKET_ENABLE;
+           return;
+         }
+       else
+         {
+           /* The stub does not support the 'P' packet.  Use 'G'
+              instead, and don't try using 'P' in the future (it
+              will just waste our time).  */
+           remote_protocol_p.support = PACKET_DISABLE;
+           break;
+         }
+     }
 sprintf (buf, "g"); 
     

This patch (if 'p' were implemented for gdbserver; I have this lying
around, as it happens) would make register fetches default to using
individual 'p' packets for every register; this would hurt latency, a
lot.
Robert, wouldn't it be good enough for you to work with
!reg->in_g_packet?
 

On IA64 the g-packet is too large for kernel debugging.  How would you 
suggest identifying registers that are in g-packet.  Use another 
protocol extension to see whether target wants to exclude registers from 
g-packet?  Another way would be to optionally enable the p-packet with a 
"set-command".  This p-packet causes very little latency on IA64 because 
most register values retrieved come from RSE (register stack engine) 
backing store memory.

thanks,

Bob

Re: new gdb remote packet type

[Daniel Jacobowitz]

On Wed, May 12, 2004 at 03:05:40PM -0400, Robert Picco wrote:
> On IA64 the g-packet is too large for kernel debugging.  How would you 
> suggest identifying registers that are in g-packet.  Use another 
> protocol extension to see whether target wants to exclude registers from 
> g-packet?  Another way would be to optionally enable the p-packet with a 
> "set-command".  This p-packet causes very little latency on IA64 because 
> most register values retrieved come from RSE (register stack engine) 
> backing store memory.

The latency I was worried about was round trip time - if we have to
request a half-dozen out of a hundred registers, then this will
probably take longer than a whole g packet would, because of waiting
for each side to process the conversation.  I guess we'll ignore it for
now and investigate smaller regsets later with qPart.

-- 
Daniel Jacobowitz

Re: new gdb remote packet type

[Robert Picco]

Daniel Jacobowitz wrote:

On Wed, May 12, 2004 at 03:05:40PM -0400, Robert Picco wrote:
 

On IA64 the g-packet is too large for kernel debugging.  How would you 
suggest identifying registers that are in g-packet.  Use another 
protocol extension to see whether target wants to exclude registers from 
g-packet?  Another way would be to optionally enable the p-packet with a 
"set-command".  This p-packet causes very little latency on IA64 because 
most register values retrieved come from RSE (register stack engine) 
backing store memory.
   

The latency I was worried about was round trip time - if we have to
request a half-dozen out of a hundred registers, then this will
probably take longer than a whole g packet would, because of waiting
for each side to process the conversation.  I guess we'll ignore it for
now and investigate smaller regsets later with qPart.
 

Ah.  I see.

Would you like to share your gdbserver "p-packet" code with me?  There's 
no point in me reinventing what you have already done. 

I've looked at the gdbserver code some.  The 'P' packet isn't being 
recognized.  If the 'P' packet isn't supported, then do we want to 
support the 'p' packet in gdbserver?

thanks,

Bob

Re: new gdb remote packet type

[Daniel Jacobowitz]

On Wed, May 12, 2004 at 03:23:27PM -0400, Andrew Cagney wrote:
> 
> >This patch (if 'p' were implemented for gdbserver; I have this lying
> >around, as it happens) would make register fetches default to using
> >individual 'p' packets for every register; this would hurt latency, a
> >lot.
> 
> That isn't true.  The T packet should have previously returned all the 
> important registers (and is needed anyway to make single step fast). 
> This "p" would just fill in the gaps.
> 
> If after this we still have problems, we can investigate transfering 
> registers in bigger chunks using qPart:<regset> (it was concluded that, 
> for the moment, it is too bigger sledge hammer for this simple nut).

Sure enough, I'm mistaken.  There's no target_fetch_registers (-1)
in the core code any more, although I think there used to be; just in
various native and corefile code.  So "p" should work OK.

> >Robert, wouldn't it be good enough for you to work with
> >!reg->in_g_packet?
> 
> The original problem is that all registers are in the g-packet and that 
> it was too big.

Ah, I see what's going on (though not why it "doesn't work" - I can see
it would be hideously slow though.

-- 
Daniel Jacobowitz

Re: new gdb remote packet type

[Robert Picco]

Andrew Cagney wrote:

>
> To get this in, there's some additional leg work:
>
> - copyright assignment?
>
> - documentation (see gdb/doc/gdb.texinfo near the end)
>
> - a corresponding gdbserver patch
>
> - not to forget, ChangeLogs :-)
>
> Andrew
>
>
Hi Andrew,

FSF has a copyright assignment on file for me.  The other items you 
mentioned are contained in the patch below with the exception of 
gdbserver patch.  There wasn't any feedback on 'P' packet support not 
being in gdbserver.  So I'm assuming that not adding 'p' packet support 
is o.k.

thanks,

Bob

diff -ruN gdb-6.1.1-orig/gdb/ChangeLog gdb-6.1.1/gdb/ChangeLog
--- gdb-6.1.1-orig/gdb/ChangeLog	2004-06-14 18:39:49.000000000 -0400
+++ gdb-6.1.1/gdb/ChangeLog	2004-08-02 10:33:03.993382712 -0400
@@ -1,3 +1,9 @@
+2004-06-28  Robert Picco <Robert.Picco@hp.com>
+	* Add new 'p' packet to gdb/remote.c.  The 'p' is for fetching
+	the value of a single register.  It complements the 'P' which
+	writes a single register.  Should the remote gdbserver support
+	the 'p', then 'g' packets aren't used.
+
 2004-06-14  GDB Administrator  <gdbadmin@sourceware.org>
 
 	GDB 6.1.1 released.
diff -ruN gdb-6.1.1-orig/gdb/doc/gdb.texinfo gdb-6.1.1/gdb/doc/gdb.texinfo
--- gdb-6.1.1-orig/gdb/doc/gdb.texinfo	2004-06-14 18:25:29.000000000 -0400
+++ gdb-6.1.1/gdb/doc/gdb.texinfo	2004-08-02 10:33:28.011731368 -0400
@@ -19896,19 +19896,18 @@
 
 @item @code{O} --- reserved
 
-Reserved for future use.
-
-@item @code{p}@var{n@dots{}} --- read reg @strong{(reserved)}
+@item @code{p}@var{hex value of register} --- read register packet
 @cindex @code{p} packet
 
-@xref{write register packet}.
-
 Reply:
 @table @samp
 @item @var{r@dots{}.}
-The hex encoded value of the register in target byte order.
+Two hex values for each byte of register content in target byte order.
+@item E@var{NN}
+for an error
 @end table
 
+
 @item @code{P}@var{n@dots{}}@code{=}@var{r@dots{}} --- write register
 @anchor{write register packet}
 @cindex @code{P} packet
diff -ruN gdb-6.1.1-orig/gdb/remote.c gdb-6.1.1/gdb/remote.c
--- gdb-6.1.1-orig/gdb/remote.c	2004-02-25 15:41:00.000000000 -0500
+++ gdb-6.1.1/gdb/remote.c	2004-08-02 10:32:48.960668032 -0400
@@ -809,6 +809,23 @@
   show_packet_config_cmd (&remote_protocol_E);
 }
 
+static struct packet_config remote_protocol_p;
+
+static void
+set_remote_protocol_p_packet_cmd (char *args, int from_tty,
+				  struct cmd_list_element *c)
+{
+  update_packet_config (&remote_protocol_p);
+}
+
+static void
+show_remote_protocol_p_packet_cmd (char *args, int from_tty,
+				   struct cmd_list_element *c)
+{
+  show_packet_config_cmd (&remote_protocol_p);
+}
+
+
 
 /* Should we try the 'P' (set register) request?  */
 
@@ -2080,6 +2097,7 @@
   update_packet_config (&remote_protocol_e);
   update_packet_config (&remote_protocol_E);
   update_packet_config (&remote_protocol_P);
+  update_packet_config (&remote_protocol_p);
   update_packet_config (&remote_protocol_qSymbol);
   update_packet_config (&remote_protocol_vcont);
   for (i = 0; i < NR_Z_PACKET_TYPES; i++)
@@ -3240,6 +3258,36 @@
 /* Read the remote registers into the block REGS.  */
 /* Currently we just read all the registers, so we don't use regnum.  */
 
+static int
+fetch_register_using_p (int regnum)
+{
+  struct remote_state *rs = get_remote_state ();
+  char *buf = alloca (rs->remote_packet_size), *p;
+  char regp[MAX_REGISTER_SIZE];
+  int i;
+
+  buf[0] = 'p';
+  bin2hex((char *) &regnum, &buf[1], sizeof(regnum));
+  buf[9] = 0;
+  remote_send (buf, rs->remote_packet_size);
+  if (buf[0] != 0 && buf[0] != 'E') {
+     p = buf;
+     i = 0;
+     while (p[0] != 0) {
+	if (p[1] == 0) {
+		error("fetch_register_using_p: early buf termination");
+		return 0;
+	}
+	regp[i++] = fromhex (p[0]) * 16 + fromhex (p[1]);
+        p += 2;
+    }
+    regcache_raw_supply (current_regcache, regnum, regp);
+    return 1;
+ }
+
+ return 0;
+}
+
 static void
 remote_fetch_registers (int regnum)
 {
@@ -3260,6 +3308,31 @@
 			"Attempt to fetch a non G-packet register when this "
 			"remote.c does not support the p-packet.");
     }
+      switch (remote_protocol_p.support)
+	{
+	case PACKET_DISABLE:
+	  break;
+	case PACKET_ENABLE:
+	  if (fetch_register_using_p (regnum))
+	    return;
+	  else
+	    error ("Protocol error: p packet not recognized by stub");
+	case PACKET_SUPPORT_UNKNOWN:
+	  if (fetch_register_using_p (regnum))
+	    {
+	      /* The stub recognized the 'p' packet.  Remember this.  */
+	      remote_protocol_p.support = PACKET_ENABLE;
+	      return;
+	    }
+	  else
+	    {
+	      /* The stub does not support the 'P' packet.  Use 'G'
+	         instead, and don't try using 'P' in the future (it
+	         will just waste our time).  */
+	      remote_protocol_p.support = PACKET_DISABLE;
+	      break;
+	    }
+	}
 
   sprintf (buf, "g");
   remote_send (buf, (rs->remote_packet_size));
@@ -5419,6 +5492,7 @@
   show_remote_protocol_e_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_E_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_P_packet_cmd (args, from_tty, NULL);
+  show_remote_protocol_p_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_qSymbol_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_vcont_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_binary_download_cmd (args, from_tty, NULL);
@@ -5631,6 +5705,13 @@
 			 &remote_set_cmdlist, &remote_show_cmdlist,
 			 1);
 
+  add_packet_config_cmd (&remote_protocol_p,
+			 "p", "fetch-register",
+			 set_remote_protocol_p_packet_cmd,
+			 show_remote_protocol_p_packet_cmd,
+			 &remote_set_cmdlist, &remote_show_cmdlist,
+			 1);
+
   add_packet_config_cmd (&remote_protocol_Z[Z_PACKET_SOFTWARE_BP],
 			 "Z0", "software-breakpoint",
 			 set_remote_protocol_Z_software_bp_packet_cmd,

Re: new gdb remote packet type

[Andrew Cagney]

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

[draining my backlog]


> Andrew Cagney wrote:
> 
>>
>> To get this in, there's some additional leg work:
>>
>> - copyright assignment?
>>
>> - documentation (see gdb/doc/gdb.texinfo near the end)
>>
>> - a corresponding gdbserver patch
>>
>> - not to forget, ChangeLogs :-)
>>
>> Andrew

I cleaned up the ChangeLog and commited the remote.c change (adding a 
nod to Fernando Nasser who wrote a part of the original code).

Eli, How's the attached slightly revised doco tweak?

Andrew

> FSF has a copyright assignment on file for me.  The other items you mentioned are contained in the patch below with the exception of gdbserver patch.  There wasn't any feedback on 'P' packet support not being in gdbserver.  So I'm assuming that not adding 'p' packet support is o.k.
> 
> thanks,
> 
> Bob
> 
> diff -ruN gdb-6.1.1-orig/gdb/ChangeLog gdb-6.1.1/gdb/ChangeLog
> --- gdb-6.1.1-orig/gdb/ChangeLog    2004-06-14 18:39:49.000000000 -0400
> +++ gdb-6.1.1/gdb/ChangeLog    2004-08-02 10:33:03.993382712 -0400
> @@ -1,3 +1,9 @@
> +2004-06-28  Robert Picco <Robert.Picco@hp.com>
> +    * Add new 'p' packet to gdb/remote.c.  The 'p' is for fetching
> +    the value of a single register.  It complements the 'P' which
> +    writes a single register.  Should the remote gdbserver support
> +    the 'p', then 'g' packets aren't used.
> +
> 2004-06-14  GDB Administrator  <gdbadmin@sourceware.org>

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 1135 bytes --]

2004-09-24  Andrew Cagney  <cagney@gnu.org>
	    Robert Picco  <Robert.Picco@hp.com>

	* gdb.texinfo (Packets): Document the "p" packet.

Index: gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.214
diff -p -u -r1.214 gdb.texinfo
--- gdb.texinfo	20 Sep 2004 22:26:21 -0000	1.214
+++ gdb.texinfo	24 Sep 2004 20:04:27 -0000
@@ -19963,17 +19963,20 @@ Reserved for future use.
 
 @item @code{O} --- reserved
 
-Reserved for future use.
-
-@item @code{p}@var{n@dots{}} --- read reg @strong{(reserved)}
+@item @code{p}@var{hex number of register} --- read register packet
 @cindex @code{p} packet
 
-@xref{write register packet}.
+@xref{read registers packet}, for a description of how the returned
+register value is encoded.
 
 Reply:
 @table @samp
-@item @var{r@dots{}.}
-The hex encoded value of the register in target byte order.
+@item @var{XX@dots{}}
+the register's value
+@item E@var{NN}
+for an error
+@item
+Indicating an unrecognized @var{query}.
 @end table
 
 @item @code{P}@var{n@dots{}}@code{=}@var{r@dots{}} --- write register

Re: new gdb remote packet type

[Eli Zaretskii]

> Date: Fri, 24 Sep 2004 16:04:37 -0400
> From: Andrew Cagney <cagney@gnu.org>
> Cc: gdb-patches@sources.redhat.com
> 
> Eli, How's the attached slightly revised doco tweak?

Fine with me, thanks.

Re: new gdb remote packet type

[Andrew Cagney]

> 2004-09-24  Andrew Cagney  <cagney@gnu.org>
> 	    Robert Picco  <Robert.Picco@hp.com>
> 
> 	* gdb.texinfo (Packets): Document the "p" packet.

I've checked this in.

Andrew