Re: Patch to speed up remote protocol

[Andrew Cagney <ac131313@cygnus.com>]

Ian Lance Taylor wrote:
> 
> The remote protocol code always retrieves all the register values
> before it stores a register value.  That is required for older stubs,
> but it is a waste of time if the stub supports the 'P' request, which
> permits the protocol to set a specific register.
> 
> This is particularly unfortunate because gdb likes to write the PC
> every time it continues, which means that when stepping through a
> program, gdb is continually and uselessly fetching all the register
> values, with horrible results over a slow serial line.
> 
> This patch fixes this problem by skipping the fetch of all the
> register values when the 'P' request is known to work.

Hmm, good idea.

Attached is a patch that integrates your change into a general cleanup
of ``P'' packet support code.
The patch adds the commands:

	set stub-supports-P-packet {supported,unsupported,auto}
	show stub-supports-P-packet

so explicit examination/control is also possible.  (I'm very interested
in suggestions for a better variable name).  The bulk of the code is
also generic so that it can be re-used by code detecting other optional
packets.

One important difference between this and Ian's original patch is that
in the below refuses to allow a target to change its mind over ``P''
support.

Ian, can you give this a wirl?

	enjoy,
		Andrew

Wed Jul  7 19:29:14 1999  Andrew Cagney  <cagney@b1.cygnus.com>

        * remote.c (enum packet_support, enum packet_detect, struct
        packet_config): Define.
        (set_packet_config_cmd, show_packet_config_cmd,
        add_packet_config_cmd, init_packet_config): New functions.
        Generic support for optional packets.
        (stub_supports_P): Change type to ``struct packet_config''.
        (set_stub_supports_P_packet_cmd,
show_stub_supports_P_packet_cmd):
        New functions.
        (_initialize_remote): Add ``set stub-supports-P-packet''
command.
        (remote_open_1, remote_async_open_1, remote_cisco_open):
        Initialize ``stub_supports_P''.
        (remote_store_registers): Re-write ``P'' probe logic.
        (store_register_using_P): New function.

        From Ian Lance Taylor <ian@airs.com>:
        (remote_prepare_to_store): Only read registers when ``P'' packet
        is in state unsupported or support-unknown.
Index: remote.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/remote.c,v
retrieving revision 1.201
diff -p -r1.201 remote.c
*** remote.c	1999/07/06 21:45:23	1.201
--- remote.c	1999/07/07 11:11:45
*************** void remote_console_output PARAMS ((char
*** 370,375 ****
--- 370,402 ----
  
  static void check_binary_download PARAMS ((CORE_ADDR addr));
  
+ #if __STDC__
+ struct packet_config;
+ #endif
+ 
+ static void show_packet_config_cmd PARAMS ((struct packet_config *config));
+ 
+ static void set_packet_config_cmd PARAMS ((struct packet_config *config,
+ 					   struct cmd_list_element *c));
+ 
+ static void add_packet_config_cmd PARAMS ((struct packet_config *config,
+ 					   char *name,
+ 					   void (*set_func) (char *args, int from_tty, struct cmd_list_element *c),
+ 					   void (*show_func) (char *name, int from_tty),
+ 					   char *doc));
+ 
+ static void init_packet_config PARAMS ((struct packet_config *config));
+ 
+ static void set_stub_supports_P_packet_cmd PARAMS ((char *args,
+ 						    int from_tty,
+ 						    struct cmd_list_element *c));
+ 
+ static void show_stub_supports_P_packet_cmd PARAMS ((char *args,
+ 						     int from_tty));
+ 
+ 
+ 
+ 
  /* Define the target subroutine names */
  
  void open_remote_target PARAMS ((char *, int, struct target_ops *, int));
*************** static int remote_address_size;
*** 465,475 ****
     trailers, it is only the payload size. */
  
  static int remote_register_buf_size = 0;
  
! /* Should we try the 'P' request?  If this is set to one when the stub
!    doesn't support 'P', the only consequence is some unnecessary traffic.  */
! static int stub_supports_P = 1;
  
  /* Tokens for use by the asynchronous signal handlers for SIGINT */
  PTR sigint_remote_twice_token;
  PTR sigint_remote_token;
--- 492,637 ----
     trailers, it is only the payload size. */
  
  static int remote_register_buf_size = 0;
+ 
+ /* Generic configuration support for packets the stub optionally
+    supports. Allows the user to specify the use of the packet as well
+    as allowing GDB to auto-detect support in the remote stub. */
+ 
+ enum packet_support {
+   PACKET_SUPPORT_UNKNOWN = 0,
+   PACKET_SUPPORTED,
+   PACKET_UNSUPPORTED
+ };
+ 
+ enum packet_detect {
+   PACKET_AUTO_DETECT = 0,
+   PACKET_MANUAL_DETECT
+ };
+ 
+ struct packet_config
+ {
+   char *state;
+   char *name;
+   enum packet_detect detect;
+   enum packet_support support;
+ };
+ 
+ static char packet_config_auto[] = "auto";
+ static char packet_config_supported[] = "supported";
+ static char packet_config_unsupported[] = "unsupported";
+ static char *packet_config_enums[] = {
+   packet_config_auto,
+   packet_config_supported,
+   packet_config_unsupported,
+   0,
+ };
+ 
+ static void
+ set_packet_config_cmd (config, c)
+      struct packet_config *config;
+      struct cmd_list_element *c;
+ {
+   if (config->state == packet_config_supported)
+     {
+       config->detect = PACKET_MANUAL_DETECT;
+       config->support = PACKET_SUPPORTED;
+     }
+   else if (config->state == packet_config_unsupported)
+     {
+       config->detect = PACKET_MANUAL_DETECT;
+       config->support = PACKET_UNSUPPORTED;
+     }
+   else if (config->state == packet_config_auto)
+     {
+       config->detect = PACKET_AUTO_DETECT;
+       config->support = PACKET_SUPPORT_UNKNOWN;
+     }
+   else
+     fatal ("Bad enum value");
+ }
  
! static void
! show_packet_config_cmd (config)
!      struct packet_config *config;
! {
!   char *support = "internal-error";
!   switch (config->support)
!     {
!     case PACKET_SUPPORTED:
!       support = "supported";
!       break;
!     case PACKET_UNSUPPORTED:
!       support = "unsupported";
!       break;
!     case PACKET_SUPPORT_UNKNOWN:
!       support = "unknown";
!       break;
!     }
!   switch (config->detect)
!     {
!     case PACKET_AUTO_DETECT:
!       printf_filtered ("%s is auto-detected, currently %s\n",
! 		       config->name, support);
!       break;
!     case PACKET_MANUAL_DETECT:
!       printf_filtered ("%s is currently %s\n",
! 		       config->name, support);
!     }
! }
  
+ static void
+ add_packet_config_cmd (config, name, set_func, show_func, doc)
+      struct packet_config *config;
+      char *name;
+      void (*set_func) PARAMS ((char *args, int from_tty,
+ 			       struct cmd_list_element *c));
+      void (*show_func) PARAMS ((char *name, int from_tty));
+      char *doc;
+ {
+   struct cmd_list_element *c;
+   config->name = name;
+   c = add_set_enum_cmd (name, class_obscure, packet_config_enums,
+ 			(char *) &config->state,
+ 			doc, &setlist);
+   c->function.sfunc = set_func;
+   add_cmd (name, class_obscure, show_func, doc, &showlist);
+ }
+ 
+ static void
+ init_packet_config (config)
+      struct packet_config *config;
+ {
+   switch (config->detect)
+     {
+     case PACKET_AUTO_DETECT:
+       config->support = PACKET_SUPPORT_UNKNOWN;
+       break;
+     case PACKET_MANUAL_DETECT:
+       /* let the user beware */
+       break;
+     }
+ }
+ 
+ /* Should we try the 'P' (set register) request?  */
+ 
+ static struct packet_config stub_supports_P;
+ 
+ static void
+ set_stub_supports_P_packet_cmd (char *args,
+ 				int from_tty,
+ 				struct cmd_list_element *c)
+ {
+   set_packet_config_cmd (&stub_supports_P, c);
+ }
+ 
+ static void
+ show_stub_supports_P_packet_cmd (char *args,
+ 				 int from_tty)
+ {
+   show_packet_config_cmd (&stub_supports_P);
+ }
+ 
+ 
  /* Tokens for use by the asynchronous signal handlers for SIGINT */
  PTR sigint_remote_twice_token;
  PTR sigint_remote_token;
*************** serial device is attached to the remote 
*** 1711,1721 ****
      }
    push_target (target);	/* Switch to using remote target now */
  
!   /* Start out by trying the 'P' request to set registers.  We set
!      this each time that we open a new target so that if the user
!      switches from one stub to another, we can (if the target is
!      closed and reopened) cope.  */
!   stub_supports_P = 1;
  
    general_thread  = -2;
    continue_thread = -2;
--- 1873,1879 ----
      }
    push_target (target);	/* Switch to using remote target now */
  
!   init_packet_config (&stub_supports_P);
  
    general_thread  = -2;
    continue_thread = -2;
*************** serial device is attached to the remote 
*** 1805,1815 ****
  
    push_target (target);	/* Switch to using remote target now */
  
!   /* Start out by trying the 'P' request to set registers.  We set
!      this each time that we open a new target so that if the user
!      switches from one stub to another, we can (if the target is
!      closed and reopened) cope.  */
!   stub_supports_P = 1;
  
    general_thread  = -2;
    continue_thread = -2;
--- 1963,1969 ----
  
    push_target (target);	/* Switch to using remote target now */
  
!   init_packet_config (&stub_supports_P);
  
    general_thread  = -2;
    continue_thread = -2;
*************** static void 
*** 2728,2736 ****
  remote_prepare_to_store ()
  {
    /* Make sure the entire registers array is valid.  */
!   read_register_bytes (0, (char *)NULL, REGISTER_BYTES);
  }
  
  /* Store register REGNO, or all registers if REGNO == -1, from the contents
     of REGISTERS.  FIXME: ignores errors.  */
  
--- 2882,2926 ----
  remote_prepare_to_store ()
  {
    /* Make sure the entire registers array is valid.  */
!   switch (stub_supports_P.support)
!     {
!     case PACKET_UNSUPPORTED:
!     case PACKET_SUPPORT_UNKNOWN:
!       read_register_bytes (0, (char *)NULL, REGISTER_BYTES);
!       break;
!     case PACKET_SUPPORTED:
!       break;
!     }
  }
  
+ /* Helper: Attempt to store REGNO using the P packet.  Return fail IFF
+    packet was not recognized. */
+ 
+ static int
+ store_register_using_P (regno)
+      int regno;
+ {
+   /* Try storing a single register.  */
+   char *buf = alloca (PBUFSIZ);
+   char *regp;
+   char *p;
+   int i;
+   
+   sprintf (buf, "P%x=", regno);
+   p = buf + strlen (buf);
+   regp = &registers[REGISTER_BYTE (regno)];
+   for (i = 0; i < REGISTER_RAW_SIZE (regno); ++i)
+     {
+       *p++ = tohex ((regp[i] >> 4) & 0xf);
+       *p++ = tohex (regp[i] & 0xf);
+     }
+   *p = '\0';
+   remote_send (buf);
+   
+   return buf[0] != '\0';
+ }      
+ 
+ 
  /* Store register REGNO, or all registers if REGNO == -1, from the contents
     of REGISTERS.  FIXME: ignores errors.  */
  
*************** remote_store_registers (regno)
*** 2744,2774 ****
  
    set_thread (inferior_pid, 1);
  
!   if (regno >= 0 && stub_supports_P)
      {
!       /* Try storing a single register.  */
!       char *regp;
! 
!       sprintf (buf, "P%x=", regno);
!       p = buf + strlen (buf);
!       regp = &registers[REGISTER_BYTE (regno)];
!       for (i = 0; i < REGISTER_RAW_SIZE (regno); ++i)
! 	{
! 	  *p++ = tohex ((regp[i] >> 4) & 0xf);
! 	  *p++ = tohex (regp[i] & 0xf);
! 	}
!       *p = '\0';
!       remote_send (buf);
!       if (buf[0] != '\0')
  	{
! 	  /* The stub understands the 'P' request.  We are done.  */
! 	  return;
  	}
- 
-       /* The stub does not support the 'P' request.  Use 'G' instead,
- 	 and don't try using 'P' in the future (it will just waste our
- 	 time).  */
-       stub_supports_P = 0;
      }
  
    buf[0] = 'G';
--- 2934,2966 ----
  
    set_thread (inferior_pid, 1);
  
!   if (regno >= 0)
      {
!       switch (stub_supports_P.support)
  	{
! 	case PACKET_UNSUPPORTED:
! 	  break;
! 	case PACKET_SUPPORTED:
! 	  if (store_register_using_P (regno))
! 	    return;
! 	  else
! 	    error ("Protocol error: P packet not recognized by stub");
! 	case PACKET_SUPPORT_UNKNOWN:
! 	  if (store_register_using_P (regno))
! 	    {
! 	      /* The stub recognized the 'P' packet.  Remember this.  */
! 	      stub_supports_P.support = PACKET_SUPPORTED;
! 	      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).  */
! 	      stub_supports_P.support = PACKET_UNSUPPORTED;
! 	      break;
! 	    }
  	}
      }
  
    buf[0] = 'G';
*************** device is attached to the remote system 
*** 4392,4401 ****
  
    push_target (&remote_cisco_ops);	/* Switch to using cisco target now */
  
!   /* Start out by trying the 'P' request to set registers.  We set this each
!      time that we open a new target so that if the user switches from one
!      stub to another, we can (if the target is closed and reopened) cope.  */
!   stub_supports_P = 1;
  
    general_thread  = -2;
    continue_thread = -2;
--- 4584,4590 ----
  
    push_target (&remote_cisco_ops);	/* Switch to using cisco target now */
  
!   init_packet_config (&stub_supports_P);
  
    general_thread  = -2;
    continue_thread = -2;
*************** in a memory packet.\n",
*** 4841,4844 ****
--- 5030,5037 ----
    add_info ("remote-process", remote_info_process,
  	    "Query the remote system for process info.");
  
+   add_packet_config_cmd (&stub_supports_P, "stub-supports-P-packet",
+ 			 set_stub_supports_P_packet_cmd,
+ 			 show_stub_supports_P_packet_cmd,
+ 			 "Stub supports ``P'' (set register) packet");
  }