Re: stop gdb/remote.c from handling partial memory reads itself

[Michael Snyder <msnyder@vmware.com>]

Kudos for this.  You also fixed a problem whereby long remote reads 
could not be interrupted.

Pedro Alves wrote:
> gdb/remote.c is presently handling partial memory reads itself.
> It shouldn't. Higher layers should handle it.  Consider, for
> example target_read_string.  If the remote end gives us less than
> we wanted, we might as well look for the end \0 byte immediately
> in the data we already have, rather than having remote.c possibly
> forcing another unnecessary roundtrip until it has as much data
> as requested.
> 
> Tested against an x86_64-linux gdbserver and checked in.
> 
> --
> Pedro Alves
> 
> 2011-01-25  Pedro Alves  <pedro@codesourcery.com>
> 
>         Stop remote_read_bytes from handling partial reads itself.
> 
>         * remote-fileio.c: Include target.h.
>         (remote_fileio_write_bytes): Delete.
>         (remote_fileio_func_open, remote_fileio_func_write)
>         (remote_fileio_func_rename, remote_fileio_func_unlink): Use
>         target_read_memory.
>         (remote_fileio_func_stat): Use target_read_memory and
>         target_write_memory.
>         (remote_fileio_func_gettimeofday): Use target_write_memory.
>         (remote_fileio_func_system): Use target_read_memory.
>         * remote.c (remote_write_bytes): Make it static.
>         (remote_read_bytes): Don't handle partial reads here.
>         * remote.h (remote_read_bytes): Delete declaration.
> 
> ---
>  gdb/remote-fileio.c |   76 +++++++++++++-----------------------------
>  gdb/remote.c        |   92
> +++++++++++++++++++---------------------------------
>  gdb/remote.h        |    5 --
>  3 files changed, 58 insertions(+), 115 deletions(-)
> 
> Index: src/gdb/remote-fileio.c
> ===================================================================
> --- src.orig/gdb/remote-fileio.c        2011-01-24 12:42:54.126176998 +0000
> +++ src/gdb/remote-fileio.c     2011-01-25 11:40:48.997639995 +0000
> @@ -30,6 +30,7 @@
>  #include "exceptions.h"
>  #include "remote-fileio.h"
>  #include "event-loop.h"
> +#include "target.h"
> 
>  #include <fcntl.h>
>  #include <sys/time.h>
> @@ -588,29 +589,11 @@ remote_fileio_return_success (int retcod
>    remote_fileio_reply (retcode, 0);
>  }
> 
> -/* Wrapper function for remote_write_bytes() which has the disadvantage to
> -   write only one packet, regardless of the requested number of bytes to
> -   transfer.  This wrapper calls remote_write_bytes() as often as needed.  */
> -static int
> -remote_fileio_write_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
> -{
> -  int ret = 0, written;
> -
> -  while (len > 0 && (written = remote_write_bytes (memaddr, myaddr, len)) >
> 0)
> -    {
> -      len -= written;
> -      memaddr += written;
> -      myaddr += written;
> -      ret += written;
> -    }
> -  return ret;
> -}
> -
>  static void
>  remote_fileio_func_open (char *buf)
>  {
>    CORE_ADDR ptrval;
> -  int length, retlength;
> +  int length;
>    long num;
>    int flags, fd;
>    mode_t mode;
> @@ -638,10 +621,9 @@ remote_fileio_func_open (char *buf)
>      }
>    mode = remote_fileio_mode_to_host (num, 1);
> 
> -  /* Request pathname using 'm' packet.  */
> +  /* Request pathname.  */
>    pathname = alloca (length);
> -  retlength = remote_read_bytes (ptrval, (gdb_byte *) pathname, length);
> -  if (retlength != length)
> +  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
>      {
>        remote_fileio_ioerror ();
>        return;
> @@ -819,10 +801,9 @@ remote_fileio_func_read (char *buf)
> 
>    if (ret > 0)
>      {
> -      retlength = remote_fileio_write_bytes (ptrval, buffer, ret);
> -      if (retlength != ret)
> -       ret = -1; /* errno has been set to EIO in
> -                    remote_fileio_write_bytes().  */
> +      errno = target_write_memory (ptrval, buffer, ret);
> +      if (errno != 0)
> +       ret = -1;
>      }
> 
>    if (ret < 0)
> @@ -839,7 +820,7 @@ remote_fileio_func_write (char *buf)
>    long target_fd, num;
>    LONGEST lnum;
>    CORE_ADDR ptrval;
> -  int fd, ret, retlength;
> +  int fd, ret;
>    gdb_byte *buffer;
>    size_t length;
> 
> @@ -871,8 +852,7 @@ remote_fileio_func_write (char *buf)
>    length = (size_t) num;
> 
>    buffer = (gdb_byte *) xmalloc (length);
> -  retlength = remote_read_bytes (ptrval, buffer, length);
> -  if (retlength != length)
> +  if (target_read_memory (ptrval, buffer, length) != 0)
>      {
>        xfree (buffer);
>        remote_fileio_ioerror ();
> @@ -966,7 +946,7 @@ static void
>  remote_fileio_func_rename (char *buf)
>  {
>    CORE_ADDR old_ptr, new_ptr;
> -  int old_len, new_len, retlength;
> +  int old_len, new_len;
>    char *oldpath, *newpath;
>    int ret, of, nf;
>    struct stat ost, nst;
> @@ -987,8 +967,7 @@ remote_fileio_func_rename (char *buf)
> 
>    /* Request oldpath using 'm' packet */
>    oldpath = alloca (old_len);
> -  retlength = remote_read_bytes (old_ptr, (gdb_byte *) oldpath, old_len);
> -  if (retlength != old_len)
> +  if (target_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0)
>      {
>        remote_fileio_ioerror ();
>        return;
> @@ -996,8 +975,7 @@ remote_fileio_func_rename (char *buf)
> 
>    /* Request newpath using 'm' packet */
>    newpath = alloca (new_len);
> -  retlength = remote_read_bytes (new_ptr, (gdb_byte *) newpath, new_len);
> -  if (retlength != new_len)
> +  if (target_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0)
>      {
>        remote_fileio_ioerror ();
>        return;
> @@ -1062,7 +1040,7 @@ static void
>  remote_fileio_func_unlink (char *buf)
>  {
>    CORE_ADDR ptrval;
> -  int length, retlength;
> +  int length;
>    char *pathname;
>    int ret;
>    struct stat st;
> @@ -1075,8 +1053,7 @@ remote_fileio_func_unlink (char *buf)
>      }
>    /* Request pathname using 'm' packet */
>    pathname = alloca (length);
> -  retlength = remote_read_bytes (ptrval, (gdb_byte *) pathname, length);
> -  if (retlength != length)
> +  if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0)
>      {
>        remote_fileio_ioerror ();
>        return;
> @@ -1103,7 +1080,7 @@ static void
>  remote_fileio_func_stat (char *buf)
>  {
>    CORE_ADDR statptr, nameptr;
> -  int ret, namelength, retlength;
> +  int ret, namelength;
>    char *pathname;
>    LONGEST lnum;
>    struct stat st;
> @@ -1126,8 +1103,7 @@ remote_fileio_func_stat (char *buf)
> 
>    /* Request pathname using 'm' packet */
>    pathname = alloca (namelength);
> -  retlength = remote_read_bytes (nameptr, (gdb_byte *) pathname, namelength);
> -  if (retlength != namelength)
> +  if (target_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0)
>      {
>        remote_fileio_ioerror ();
>        return;
> @@ -1151,10 +1127,9 @@ remote_fileio_func_stat (char *buf)
>      {
>        remote_fileio_to_fio_stat (&st, &fst);
>        remote_fileio_to_fio_uint (0, fst.fst_dev);
> -
> -      retlength = remote_fileio_write_bytes (statptr,
> -                                            (gdb_byte *) &fst, sizeof fst);
> -      if (retlength != sizeof fst)
> +
> +      errno = target_write_memory (statptr, (gdb_byte *) &fst, sizeof fst);
> +      if (errno != 0)
>         {
>           remote_fileio_return_errno (-1);
>           return;
> @@ -1236,9 +1211,8 @@ remote_fileio_func_fstat (char *buf)
>      {
>        remote_fileio_to_fio_stat (&st, &fst);
> 
> -      retlength = remote_fileio_write_bytes (ptrval, (gdb_byte *) &fst,
> -                                            sizeof fst);
> -      if (retlength != sizeof fst)
> +      errno = target_write_memory (ptrval, (gdb_byte *) &fst, sizeof fst);
> +      if (errno != 0)
>         {
>           remote_fileio_return_errno (-1);
>           return;
> @@ -1289,9 +1263,8 @@ remote_fileio_func_gettimeofday (char *b
>      {
>        remote_fileio_to_fio_timeval (&tv, &ftv);
> 
> -      retlength = remote_fileio_write_bytes (ptrval, (gdb_byte *) &ftv,
> -                                            sizeof ftv);
> -      if (retlength != sizeof ftv)
> +      errno = target_write_memory (ptrval, (gdb_byte *) &ftv, sizeof ftv);
> +      if (errno != 0)
>         {
>           remote_fileio_return_errno (-1);
>           return;
> @@ -1336,8 +1309,7 @@ remote_fileio_func_system (char *buf)
>      {
>        /* Request commandline using 'm' packet */
>        cmdline = alloca (length);
> -      retlength = remote_read_bytes (ptrval, (gdb_byte *) cmdline, length);
> -      if (retlength != length)
> +      if (target_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0)
>         {
>           remote_fileio_ioerror ();
>           return;
> Index: src/gdb/remote.c
> ===================================================================
> --- src.orig/gdb/remote.c       2011-01-25 09:49:05.487639998 +0000
> +++ src/gdb/remote.c    2011-01-25 11:40:49.007639996 +0000
> @@ -6356,7 +6356,7 @@ remote_write_bytes_aux (const char *head
>     Returns number of bytes transferred, or 0 (setting errno) for
>     error.  Only transfer a single packet.  */
> 
> -int
> +static int
>  remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, int len)
>  {
>    char *packet_format = 0;
> @@ -6391,19 +6391,14 @@ remote_write_bytes (CORE_ADDR memaddr, c
> 
>     Returns number of bytes transferred, or 0 for error.  */
> 
> -/* NOTE: cagney/1999-10-18: This function (and its siblings in other
> -   remote targets) shouldn't attempt to read the entire buffer.
> -   Instead it should read a single packet worth of data and then
> -   return the byte size of that packet to the caller.  The caller (its
> -   caller and its callers caller ;-) already contains code for
> -   handling partial reads.  */
> -
> -int
> +static int
>  remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
>  {
>    struct remote_state *rs = get_remote_state ();
>    int max_buf_size;            /* Max size of packet output buffer.  */
> -  int origlen;
> +  char *p;
> +  int todo;
> +  int i;
> 
>    if (len <= 0)
>      return 0;
> @@ -6412,56 +6407,37 @@ remote_read_bytes (CORE_ADDR memaddr, gd
>    /* The packet buffer will be large enough for the payload;
>       get_memory_packet_size ensures this.  */
> 
> -  origlen = len;
> -  while (len > 0)
> +  /* Number if bytes that will fit.  */
> +  todo = min (len, max_buf_size / 2);
> +
> +  /* Construct "m"<memaddr>","<len>".  */
> +  memaddr = remote_address_masked (memaddr);
> +  p = rs->buf;
> +  *p++ = 'm';
> +  p += hexnumstr (p, (ULONGEST) memaddr);
> +  *p++ = ',';
> +  p += hexnumstr (p, (ULONGEST) todo);
> +  *p = '\0';
> +  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')
>      {
> -      char *p;
> -      int todo;
> -      int i;
> -
> -      todo = min (len, max_buf_size / 2);      /* num bytes that will fit.  */
> -
> -      /* construct "m"<memaddr>","<len>" */
> -      /* sprintf (rs->buf, "m%lx,%x", (unsigned long) memaddr, todo); */
> -      memaddr = remote_address_masked (memaddr);
> -      p = rs->buf;
> -      *p++ = 'm';
> -      p += hexnumstr (p, (ULONGEST) memaddr);
> -      *p++ = ',';
> -      p += hexnumstr (p, (ULONGEST) todo);
> -      *p = '\0';
> -
> -      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')
> -       {
> -         /* There is no correspondance between what the remote
> -            protocol uses for errors and errno codes.  We would like
> -            a cleaner way of representing errors (big enough to
> -            include errno codes, bfd_error codes, and others).  But
> -            for now just return EIO.  */
> -         errno = EIO;
> -         return 0;
> -       }
> -
> -      /* Reply describes memory byte by byte,
> -         each byte encoded as two hex characters.  */
> -
> -      p = rs->buf;
> -      if ((i = hex2bin (p, myaddr, todo)) < todo)
> -       {
> -         /* Reply is short.  This means that we were able to read
> -            only part of what we wanted to.  */
> -         return i + (origlen - len);
> -       }
> -      myaddr += todo;
> -      memaddr += todo;
> -      len -= todo;
> +      /* There is no correspondance between what the remote protocol
> +        uses for errors and errno codes.  We would like a cleaner way
> +        of representing errors (big enough to include errno codes,
> +        bfd_error codes, and others).  But for now just return
> +        EIO.  */
> +      errno = EIO;
> +      return 0;
>      }
> -  return origlen;
> +  /* Reply describes memory byte by byte, each byte encoded as two hex
> +     characters.  */
> +  p = rs->buf;
> +  i = hex2bin (p, myaddr, todo);
> +  /* Return what we have.  Let higher layers handle partial reads.  */
> +  return i;
>  }
> 
> 
> Index: src/gdb/remote.h
> ===================================================================
> --- src.orig/gdb/remote.h       2011-01-24 12:42:54.126176998 +0000
> +++ src/gdb/remote.h    2011-01-25 11:40:49.007639996 +0000
> @@ -42,11 +42,6 @@ extern char *unpack_varlen_hex (char *bu
> 
>  extern void async_remote_interrupt_twice (void *arg);
> 
> -extern int remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr,
> -                              int len);
> -
> -extern int remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
> -
>  void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
>                                      const struct target_desc *tdesc);
>  void register_remote_support_xml (const char *);