From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14571 invoked by alias); 8 Feb 2011 20:43:18 -0000 Received: (qmail 14558 invoked by uid 22791); 8 Feb 2011 20:43:16 -0000 X-SWARE-Spam-Status: No, hits=-4.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,TW_SX,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-outbound-1.vmware.com (HELO smtp-outbound-1.vmware.com) (65.115.85.69) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 08 Feb 2011 20:43:10 +0000 Received: from mailhost2.vmware.com (mailhost2.vmware.com [10.16.67.167]) by smtp-outbound-1.vmware.com (Postfix) with ESMTP id 58ECB56003; Tue, 8 Feb 2011 12:43:08 -0800 (PST) Received: from msnyder-server.eng.vmware.com (promd-2s-dhcp138.eng.vmware.com [10.20.124.138]) by mailhost2.vmware.com (Postfix) with ESMTP id 4E78D8EE13; Tue, 8 Feb 2011 12:43:08 -0800 (PST) Message-ID: <4D51AADB.9090700@vmware.com> Date: Tue, 08 Feb 2011 20:43:00 -0000 From: Michael Snyder User-Agent: Thunderbird 2.0.0.24 (X11/20101201) MIME-Version: 1.0 To: Pedro Alves CC: "gdb-patches@sourceware.org" Subject: Re: stop gdb/remote.c from handling partial memory reads itself References: <201101251156.01581.pedro@codesourcery.com> In-Reply-To: <201101251156.01581.pedro@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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-02/txt/msg00180.txt.bz2 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 > > 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 *);