From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9599 invoked by alias); 25 Jan 2011 11:56:10 -0000 Received: (qmail 9505 invoked by uid 22791); 25 Jan 2011 11:56:08 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_SX,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 Jan 2011 11:56:01 +0000 Received: (qmail 25030 invoked from network); 25 Jan 2011 11:55:59 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 25 Jan 2011 11:55:59 -0000 To: gdb-patches@sourceware.org Subject: stop gdb/remote.c from handling partial memory reads itself From: Pedro Alves Date: Tue, 25 Jan 2011 12:21:00 -0000 MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201101251156.01581.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-01/txt/msg00489.txt.bz2 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 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 #include @@ -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 = 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"","" */ - /* 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 *);