stop gdb/remote.c from handling partial memory reads itself
Michael Snyder
msnyder@vmware.com
Tue Feb 8 20:43:00 GMT 2011
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 *);
More information about the Gdb-patches
mailing list