From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26482 invoked by alias); 14 Jan 2002 02:41:09 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 26437 invoked from network); 14 Jan 2002 02:41:07 -0000 Received: from unknown (HELO cygnus.com) (205.180.230.5) by sources.redhat.com with SMTP; 14 Jan 2002 02:41:07 -0000 Received: from redhat.com (reddwarf.sfbay.redhat.com [205.180.231.12]) by runyon.cygnus.com (8.8.7-cygnus/8.8.7) with ESMTP id SAA11568; Sun, 13 Jan 2002 18:41:04 -0800 (PST) Message-ID: <3C4243FC.7B2832BA@redhat.com> Date: Sun, 13 Jan 2002 18:41:00 -0000 From: Michael Snyder Organization: Red Hat, Inc. X-Mailer: Mozilla 4.76 [en] (X11; U; Linux 2.4.2-2smp i686) X-Accept-Language: en MIME-Version: 1.0 To: Andrew Cagney CC: gdb-patches@sources.redhat.com Subject: Re: [RFA] Crasher bug in infptrace.c References: <200112310020.fBV0Kr119534@reddwarf.cygnus.com> <3C33F983.4030605@cygnus.com> <3C34AF7B.D6658DC0@redhat.com> <3C34B7FB.7060900@cygnus.com> <3C34BAB2.573376FB@redhat.com> <3C4238E2.4030605@cygnus.com> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-SW-Source: 2002-01/txt/msg00372.txt.bz2 Andrew Cagney wrote: > > > Anyway, looking at the code, I'm wondering if it would actually be > >> better to just eliminate that bounce buffer and, instead just transfer > >> the data directly. This might leave the buffer in an undefined state, I > >> think, however, that is ok. > > > > > > I spent an hour thinking about how to do that (without significantly > > uglifying the code), and decided it was more trouble than I wanted to > > go to. I agree with you -- the function doesn't require a buffer > > at all. Anyone who wants to rewrite the function to that extent > > is more than welcome to by me. ;-) > > How does the attached work-in-progress look as a starting point? Ran it > against the testsuite and that came up with a few regressions ..... > However, it isn't totally broken. Close to how I envisioned it, but see comments below. > One thing I didn't realize is that I can't trust the alignment of the > hosts buffer and consequently I can't actually avoid the double buffering. That's right... ;-) That's one reason why I bailed on it. > ------------------------------------------------------------------------ > Index: infptrace.c > =================================================================== > RCS file: /cvs/src/src/gdb/infptrace.c,v > retrieving revision 1.20 > diff -p -r1.20 infptrace.c > *** infptrace.c 2002/01/08 00:59:29 1.20 > --- infptrace.c 2002/01/14 01:40:42 > *************** > *** 26,31 **** > --- 26,32 ---- > #include "target.h" > #include "gdb_string.h" > #include "regcache.h" > + #include "gdb_assert.h" > > #include "gdb_wait.h" > > *************** child_xfer_memory (CORE_ADDR memaddr, ch > *** 505,510 **** > --- 506,603 ---- > struct mem_attrib *attrib ATTRIBUTE_UNUSED, > struct target_ops *target) > { > + #if 1 > + CORE_ADDR addr = memaddr; > + long nr_bytes = len; > + PTRACE_XFER_TYPE buffer[1000]; > + while (nr_bytes > 0) > + { > + CORE_ADDR bufaddr; > + long count; > + long sizeof_buffer; > + long addr_off; > + long addr_len; > + long addr_pad; > + long i; > + > + /* Round starting address down to longword boundary to pick up > + all of the first byte. */ > + bufaddr = addr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE); > + > + /* Round ending address up to ensure that all of the last word > + is copied up and then adjust that to fit in BUFFER. */ > + sizeof_buffer = (addr - bufaddr) + len + sizeof (PTRACE_XFER_TYPE) - 1; > + if (sizeof_buffer > sizeof (buffer)) > + sizeof_buffer = sizeof (buffer); Now you've thrown away your rounding on the ending address. > + count = sizeof_buffer / sizeof (PTRACE_XFER_TYPE); Now count will never be greater than 250, so all writes of greater than 1000 bytes will lose. > + sizeof_buffer = count * sizeof (PTRACE_XFER_TYPE); /* adjust */ > + > + /* Break the buffer down into three byte regions: ADDR_OFF > + specifies the offset into BUFFER that ADDR starts; > + ADDR_LEN specifies the number of ADDR bytes actually in > + the buffer; and ADDR_PAD is the number of left over bytes > + in the buffer. Either/or ADDR_OFF and ADDR_PAD can be > + zero. */ > + addr_off = addr - bufaddr; > + addr_len = sizeof_buffer - addr_off; OK, except that sizeof_buffer has been truncated to 1000, therefore addr_len is forced to be less than or equal to that. > + if (addr_len > len) And this can't evaluate to true if len > 1000. > + { > + addr_len = len; > + } > + addr_pad = sizeof_buffer - (addr_len + addr_off); > + gdb_assert (addr_off + addr_len + addr_pad == sizeof_buffer); > + > + /* For writes, pre-fill the buffer with data that has to go out. */ > + if (write) > + { > + /* If the start is misaligned, a read - modify - write is > + needed. */ > + if (addr_off > 0) > + buffer[0] = ptrace (PT_READ_I, PIDGET (inferior_ptid), > + (PTRACE_ARG3_TYPE) bufaddr, 0); > + /* If the end is misaligned, a read - modify - write is > + needed. */ > + if (addr_pad > 0 && count > 1) > + { > + buffer[count - 1] = > + ptrace (PT_READ_I, PIDGET (inferior_ptid), > + ((PTRACE_ARG3_TYPE) > + (bufaddr + (count - 1) * sizeof (PTRACE_XFER_TYPE))), 0); > + } > + /* Fill in LEN bytes in the middle. */ > + memcpy (buffer + addr_off, myaddr, addr_len); > + } > + > + /* Transfer COUNT words. */ > + for (i = 0; i < count; i++) > + { > + CORE_ADDR a = bufaddr + i * sizeof (PTRACE_XFER_TYPE); > + if (write) > + { > + ptrace (PT_WRITE_D, PIDGET (inferior_ptid), > + (PTRACE_ARG3_TYPE) a, buffer[i]); > + } > + else > + { > + buffer[i] = ptrace (PT_READ_I, PIDGET (inferior_ptid), > + (PTRACE_ARG3_TYPE) a, 0); > + } > + } > + > + /* Copy any read data into MYADDR. */ > + if (!write) > + memcpy (myaddr, buffer + addr_off, addr_len); > + > + /* Update all counters. */ > + addr += addr_len; > + nr_bytes -= addr_len; > + } > + return len; > + #ifdef CLEAR_INSN_CACHE > + if (write) > + CLEAR_INSN_CACHE (); > + #endif > + #else > int i; > /* Round starting address down to longword boundary. */ > CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE); > *************** child_xfer_memory (CORE_ADDR memaddr, ch > *** 592,597 **** > --- 685,691 ---- > if (old_chain != NULL) > do_cleanups (old_chain); > return len; > + #endif > } > >