[PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues

[PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues

[Maciej W. Rozycki]

Hi,

 While trying to investigate suspicious MIPS16/Linux test suite results 
noted with the recent MIPS ISA bit solution proposal I have come across 
this problem in gdbserver.  It contributed to about 2550 failures per 
multilib.

 In linux_read_memory we make a two-way choice as to how we read 
debuggee's memory -- we prefer pread64 or lseek/read of /proc/<pid>/mem 
and failing that we resort to a long boring series of PEEKTEXT ptrace 
requests.  We use this in particular in linux_qxfer_libraries_svr4 to 
access names of shared libraries loaded.  There we rely on 
linux_read_memory to return data in the buffer provided even if the 
function wasn't able to read the whole number of bytes requested.

 This has two problems as I revealed.  If any call in the pread64 fails 
then the supplied buffer is obviously not filled.  Then if the ptrace 
fallback path is not able to retrieve the whole number of bytes requested, 
then it does not supply partially retrieved data to the buffer.  This is 
the scenario that I observed.

 Gdbserver tried to retrieved the name of the dynamic linker that is 
normally linked to ld.so's internal link map from the PT_INTERP segment of 
the executable debugged.  This segment is typically very short, it can 
take a single page only.  Gdbserver wants to read 4095 (PATH_MAX) bytes 
and the offset of the name into the page PT_INTERP is mapped into is 
already 0x134.  That plus 4095 goes beyond the page:

$ mips-linux-gnu-readelf -l func-ptrs

Elf file type is EXEC (Executable file)
Entry point 0x4005c1
There are 8 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x00400034 0x00400034 0x00100 0x00100 R E 0x4
  INTERP         0x000134 0x00400134 0x00400134 0x0008c 0x0008c R   0x1
      [Requesting program interpreter: .../mips16/lib/ld.so.1]
  REGINFO        0x0001e0 0x004001e0 0x004001e0 0x00018 0x00018 R   0x4
  LOAD           0x000000 0x00400000 0x00400000 0x007f4 0x007f4 R E 0x10000
  LOAD           0x0007f4 0x004107f4 0x004107f4 0x00078 0x0008c RW  0x10000
  DYNAMIC        0x0001f8 0x004001f8 0x004001f8 0x00108 0x00108 RWE 0x4
  NOTE           0x0001c0 0x004001c0 0x004001c0 0x00020 0x00020 R   0x4
  NULL           0x000000 0x00000000 0x00000000 0x00000 0x00000     0x4
[...]
$ cat /proc/1234/maps
00400000-00401000 r-xp 00000000 00:15 6838968    .../gdb.base/func-ptrs
00410000-00411000 rw-p 00000000 00:15 6838968    .../gdb.base/func-ptrs
2aaa8000-2aac2000 r-xp 00000000 00:15 11153755   .../mips16/lib/ld-2.15.so
2aac2000-2aac5000 rw-p 00000000 00:00 0
2aad1000-2aad2000 r--p 00019000 00:15 11153755   .../mips16/lib/ld-2.15.so
2aad2000-2aad3000 rw-p 0001a000 00:15 11153755   .../mips16/lib/ld-2.15.so
[...]
$

so linux_read_memory can at most read 0x401000 - 0x400134 = 3788 bytes.  
If the pread64 path fails, e.g. /proc not mounted, then it falls back to 
the ptrace loop that eventually fails too, because it can't read beyond 
offset 3788.  The temporary buffer used by the ptrace loop is then not 
copied to the user buffer and linux_qxfer_libraries_svr4 receives nothing.

 The second problem is if /proc is indeed mounted and pread64 succeeds, 
then linux_read_memory will fall back to ptrace anyway, because in this 
case pread64 will return 3788 bytes that is different to the number 
requested.  Then the ptrace loop will go back to retrieving data from the 
beginning, again fail after 3788 have been read, ignore its temporary 
buffer and linux_qxfer_libraries_svr4 will get what pread64 has already 
retrieved -- waste of time.

 The change below addresses these two problems.  Any data successfully 
retrieved by the ptrace loop is copied over to the user buffer even if 
linux_read_memory returns unsuccessfully.  If pread64 successfully 
retrieves any data, then ptrace is not used to read that data again, the 
attempt to read data is resumed at the point where pread64 stopped.  This 
will normally not retrieve any further data as pread64 would have provided 
that (internally, in the Linux kernel, the read handlers for 
/proc/<pid>/maps special files are implemented as a wrapper around the 
very same worker function that the PEEKTEXT ptrace request uses.

 I have successfully regression-tested this change with the i686-linux-gnu 
and mips-linux-gnu targets.  OK to apply?

2012-05-16  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/gdbserver/
	* linux-low.c (linux_store_registers): Don't re-retrieve data
	with ptrace that has already been obtained from /proc.  Always
	copy any data retrieved with ptrace to the buffer supplied.

  Maciej

gdb-gdbserver-linux-read-memory-ptrace.diff
Index: gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/gdbserver/linux-low.c	2012-05-16 17:12:48.000000000 +0100
+++ gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c	2012-05-16 20:26:34.415575387 +0100
@@ -4356,23 +4356,19 @@ linux_store_registers (struct regcache *
 static int
 linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 {
+  int pid = lwpid_of (get_thread_lwp (current_inferior));
+  register PTRACE_XFER_TYPE *buffer;
+  register CORE_ADDR addr;
+  register int count;
+  char filename[64];
   register int i;
-  /* Round starting address down to longword boundary.  */
-  register CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
-  /* Round ending address up; get number of longwords that makes.  */
-  register int count
-    = (((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
-      / sizeof (PTRACE_XFER_TYPE);
-  /* Allocate buffer of that many longwords.  */
-  register PTRACE_XFER_TYPE *buffer
-    = (PTRACE_XFER_TYPE *) alloca (count * sizeof (PTRACE_XFER_TYPE));
   int fd;
-  char filename[64];
-  int pid = lwpid_of (get_thread_lwp (current_inferior));
 
   /* Try using /proc.  Don't bother for one word.  */
   if (len >= 3 * sizeof (long))
     {
+      int bytes;
+
       /* We could keep this file open and cache it - possibly one per
 	 thread.  That requires some juggling, but is even faster.  */
       sprintf (filename, "/proc/%d/mem", pid);
@@ -4385,38 +4381,55 @@ linux_read_memory (CORE_ADDR memaddr, un
 	 32-bit platforms (for instance, SPARC debugging a SPARC64
 	 application).  */
 #ifdef HAVE_PREAD64
-      if (pread64 (fd, myaddr, len, memaddr) != len)
+      bytes = pread64 (fd, myaddr, len, memaddr);
 #else
-      if (lseek (fd, memaddr, SEEK_SET) == -1 || read (fd, myaddr, len) != len)
+      bytes = -1;
+      if (lseek (fd, memaddr, SEEK_SET) != -1)
+	bytes = read (fd, myaddr, len);
 #endif
-	{
-	  close (fd);
-	  goto no_proc;
-	}
 
       close (fd);
-      return 0;
+      if (bytes == len)
+	return 0;
+
+      /* Some data was read, we'll try to get the rest with ptrace.  */
+      if (bytes > 0)
+	{
+	  memaddr += bytes;
+	  myaddr += bytes;
+	  len -= bytes;
+	}
     }
 
  no_proc:
+  /* Round starting address down to longword boundary.  */
+  addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
+  /* Round ending address up; get number of longwords that makes.  */
+  count = ((((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
+	   / sizeof (PTRACE_XFER_TYPE));
+  /* Allocate buffer of that many longwords.  */
+  buffer = (PTRACE_XFER_TYPE *) alloca (count * sizeof (PTRACE_XFER_TYPE));
+
   /* Read all the longwords */
+  errno = 0;
   for (i = 0; i < count; i++, addr += sizeof (PTRACE_XFER_TYPE))
     {
-      errno = 0;
       /* Coerce the 3rd arg to a uintptr_t first to avoid potential gcc warning
 	 about coercing an 8 byte integer to a 4 byte pointer.  */
       buffer[i] = ptrace (PTRACE_PEEKTEXT, pid,
 			  (PTRACE_ARG3_TYPE) (uintptr_t) addr, 0);
       if (errno)
-	return errno;
+	break;
     }
 
   /* Copy appropriate bytes out of the buffer.  */
+  i *= sizeof (PTRACE_XFER_TYPE);
+  i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
   memcpy (myaddr,
 	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
-	  len);
+	  i < len ? i : len);
 
-  return 0;
+  return errno;
 }
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's

Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues

[Pedro Alves]

Hi Maciej,

On 05/16/2012 11:56 PM, Maciej W. Rozycki wrote:

>  While trying to investigate suspicious MIPS16/Linux test suite results 
> noted with the recent MIPS ISA bit solution proposal I have come across 
> this problem in gdbserver.  It contributed to about 2550 failures per 
> multilib.
> 
>  In linux_read_memory we make a two-way choice as to how we read 
> debuggee's memory -- we prefer pread64 or lseek/read of /proc/<pid>/mem 
> and failing that we resort to a long boring series of PEEKTEXT ptrace 
> requests.  We use this in particular in linux_qxfer_libraries_svr4 to 
> access names of shared libraries loaded.  There we rely on 
> linux_read_memory to return data in the buffer provided even if the 
> function wasn't able to read the whole number of bytes requested.
> 
>  This has two problems as I revealed.  If any call in the pread64 fails 
> then the supplied buffer is obviously not filled.


It looks like the pread64/lseek/read path actually reads directly
into MYADDR (the supplied buffer).

> Then if the ptrace

> fallback path is not able to retrieve the whole number of bytes requested, 
> then it does not supply partially retrieved data to the buffer.  This is 
> the scenario that I observed.


Right.

>  The change below addresses these two problems.  Any data successfully 
> retrieved by the ptrace loop is copied over to the user buffer even if 
> linux_read_memory returns unsuccessfully.  If pread64 successfully 
> retrieves any data, then ptrace is not used to read that data again, the 
> attempt to read data is resumed at the point where pread64 stopped.  This 
> will normally not retrieve any further data as pread64 would have provided 
> that (internally, in the Linux kernel, the read handlers for 
> /proc/<pid>/maps special files are implemented as a wrapper around the 
> very same worker function that the PEEKTEXT ptrace request uses.


So how about just returning immediately if reading from /proc manages to
read something?  From what you say, the PEEKTEXT fallback then won't just
normally fail; it'll _always_ fail.  I suspect that'd make the code a bit
simpler.

>    /* Copy appropriate bytes out of the buffer.  */
> +  i *= sizeof (PTRACE_XFER_TYPE);
> +  i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
>    memcpy (myaddr,
>  	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
> -	  len);
> +	  i < len ? i : len);
>  
> -  return 0;
> +  return errno;


You shouldn't assume that "errno" is preserved across library
calls (memcpy in this case).

>  }
>  



-- 
Pedro Alves

Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues

[Maciej W. Rozycki]

Hi Pedro,

> >  In linux_read_memory we make a two-way choice as to how we read 
> > debuggee's memory -- we prefer pread64 or lseek/read of /proc/<pid>/mem 
> > and failing that we resort to a long boring series of PEEKTEXT ptrace 
> > requests.  We use this in particular in linux_qxfer_libraries_svr4 to 
> > access names of shared libraries loaded.  There we rely on 
> > linux_read_memory to return data in the buffer provided even if the 
> > function wasn't able to read the whole number of bytes requested.
> > 
> >  This has two problems as I revealed.  If any call in the pread64 fails 
> > then the supplied buffer is obviously not filled.
> 
> 
> It looks like the pread64/lseek/read path actually reads directly
> into MYADDR (the supplied buffer).

 Yes, that's true, but for this to happen all the calls up to and 
including pread64/read need to succeed.

> > Then if the ptrace
> 
> > fallback path is not able to retrieve the whole number of bytes requested, 
> > then it does not supply partially retrieved data to the buffer.  This is 
> > the scenario that I observed.
> 
> 
> Right.
> 
> >  The change below addresses these two problems.  Any data successfully 
> > retrieved by the ptrace loop is copied over to the user buffer even if 
> > linux_read_memory returns unsuccessfully.  If pread64 successfully 
> > retrieves any data, then ptrace is not used to read that data again, the 
> > attempt to read data is resumed at the point where pread64 stopped.  This 
> > will normally not retrieve any further data as pread64 would have provided 
> > that (internally, in the Linux kernel, the read handlers for 
> > /proc/<pid>/maps special files are implemented as a wrapper around the 
> > very same worker function that the PEEKTEXT ptrace request uses.
> 
> 
> So how about just returning immediately if reading from /proc manages to
> read something?  From what you say, the PEEKTEXT fallback then won't just
> normally fail; it'll _always_ fail.  I suspect that'd make the code a bit
> simpler.

 Maybe.  Question is, actually why it was written like this in the first 
place.  And the only answer I could come up with was: to retrieve the 
right error code for the caller to investigate.  You won't get that with 
pread64/read if they succeed reading any data at all, even if the amount 
is less than requested.  Hence I decided to preserve the original flow.  
I do not feel comfortable with cooking up an artificial value of errno.

 Alternatively we could revamp the whole API to make 
the_target->read_memory return the number of bytes actually read, just 
like read and friends do.  That would of course ask for a complementing 
change to the_target->write_memory too.  I even thought about it when I 
finally tracked down what the cause of odd behaviour was, but I decided I 
was too tired debugging this issue already to turn gdbserver upside down 
at that stage.

 I think my fix will serve its purpose and if we decide to turn overhaul 
this part of gdbserver indeed, then perhaps it would be best done after 
7.5 has been branched.

> >    /* Copy appropriate bytes out of the buffer.  */
> > +  i *= sizeof (PTRACE_XFER_TYPE);
> > +  i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
> >    memcpy (myaddr,
> >  	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
> > -	  len);
> > +	  i < len ? i : len);
> >  
> > -  return 0;
> > +  return errno;
> 
> 
> You shouldn't assume that "errno" is preserved across library
> calls (memcpy in this case).

 Oh, that was an oversight rather than a deliberate assumption, sorry 
about that.  Here's a trivial update that I have applied to my fix.

  Maciej

Index: gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/gdbserver/linux-low.c	2012-05-18 19:28:38.255559523 +0100
+++ gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c	2012-05-18 19:28:19.795559309 +0100
@@ -4384,6 +4384,7 @@ linux_read_memory (CORE_ADDR memaddr, un
   register int count;
   char filename[64];
   register int i;
+  int ret;
   int fd;
 
   /* Try using /proc.  Don't bother for one word.  */
@@ -4443,6 +4444,7 @@ linux_read_memory (CORE_ADDR memaddr, un
       if (errno)
 	break;
     }
+  ret = errno;
 
   /* Copy appropriate bytes out of the buffer.  */
   i *= sizeof (PTRACE_XFER_TYPE);
@@ -4451,7 +4453,7 @@ linux_read_memory (CORE_ADDR memaddr, un
 	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
 	  i < len ? i : len);
 
-  return errno;
+  return ret;
 }
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's

Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues

[Pedro Alves]

On 05/18/2012 07:45 PM, Maciej W. Rozycki wrote:

>> > So how about just returning immediately if reading from /proc manages to
>> > read something?  From what you say, the PEEKTEXT fallback then won't just
>> > normally fail; it'll _always_ fail.  I suspect that'd make the code a bit
>> > simpler.
>  Maybe.  Question is, actually why it was written like this in the first 
> place.  And the only answer I could come up with was: to retrieve the 
> right error code for the caller to investigate.  You won't get that with 
> pread64/read if they succeed reading any data at all, even if the amount 
> is less than requested.  Hence I decided to preserve the original flow.  
> I do not feel comfortable with cooking up an artificial value of errno.


Ah, right, we can't return immediately --- but if we loop pread64/read,
handling partial reads (and EINTR, while at it), until it returns error
would get us the errno we want, I'd think.  I think the code would be
more straightforward that way if it works, but anyway, just a suggestion;
I'm super fine with your version.

> 
>  Alternatively we could revamp the whole API to make 
> the_target->read_memory return the number of bytes actually read, just 
> like read and friends do.  That would of course ask for a complementing 
> change to the_target->write_memory too.  I even thought about it when I 
> finally tracked down what the cause of odd behaviour was, but I decided I 
> was too tired debugging this issue already to turn gdbserver upside down 
> at that stage.


Yeah, certainly not worth the effort at this stage.

>> > You shouldn't assume that "errno" is preserved across library
>> > calls (memcpy in this case).
>  Oh, that was an oversight rather than a deliberate assumption, sorry 
> about that.  Here's a trivial update that I have applied to my fix.


Thanks.  The whole patch is fine with me with this fix.

-- 
Pedro Alves

Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues

[Maciej W. Rozycki]

On Fri, 18 May 2012, Pedro Alves wrote:

> Ah, right, we can't return immediately --- but if we loop pread64/read,
> handling partial reads (and EINTR, while at it), until it returns error
> would get us the errno we want, I'd think.  I think the code would be
> more straightforward that way if it works, but anyway, just a suggestion;
> I'm super fine with your version.

 You're correct, especially EINTR is worth getting right, interrupted 
syscalls are always rare and unpredictable enough to make debugging their 
consequences a nightmare.

> >  Alternatively we could revamp the whole API to make 
> > the_target->read_memory return the number of bytes actually read, just 
> > like read and friends do.  That would of course ask for a complementing 
> > change to the_target->write_memory too.  I even thought about it when I 
> > finally tracked down what the cause of odd behaviour was, but I decided I 
> > was too tired debugging this issue already to turn gdbserver upside down 
> > at that stage.
> 
> 
> Yeah, certainly not worth the effort at this stage.

 I think ultimately we should do it though.  The /proc path is surely a 
later addition and can explain the resulting unfit API that was originally 
just fine with ptrace, but I think we should straighten it out and give an 
advantage to the common case.  Especially as it's not like the ptrace 
fallback or platforms where it's the only choice are going to suffer from 
such a read/write-like API.

 I think the two choices in the Linux handlers should be factored out into 
separate functions too.

> Thanks.  The whole patch is fine with me with this fix.

 Applied now, thanks for the review.

  Maciej

Regression for gdbserver [Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues]

[Jan Kratochvil]

On Tue, 22 May 2012 02:05:28 +0200, Maciej W. Rozycki wrote:
>  Applied now, thanks for the review.

272cb31d810a541dcc44f942fabb3167580b838e is the first bad commit
commit 272cb31d810a541dcc44f942fabb3167580b838e
Author: Maciej W. Rozycki <macro@linux-mips.org>
Date:   Mon May 21 23:50:25 2012 +0000

    	* linux-low.c (linux_store_registers): Don't re-retrieve data
    	with ptrace that has already been obtained from /proc.  Always
    	copy any data retrieved with ptrace to the buffer supplied.

-PASS: gdb.mi/gdb701.exp: list children of fooPtr
+FAIL: gdb.mi/gdb701.exp: list children of fooPtr
-PASS: gdb.mi/mi2-var-child.exp: VT: list children of ptr2.*ptr.1_anonymous
+FAIL: gdb.mi/mi2-var-child.exp: VT: list children of ptr2.*ptr.1_anonymous
-PASS: gdb.mi/mi2-var-child.exp: VT: create root varobj for v
-PASS: gdb.mi/mi2-var-child.exp: VT: list children of v3
-PASS: gdb.mi/mi2-var-child.exp: path expression for v3
-PASS: gdb.mi/mi2-var-child.exp: expression for v3
-PASS: gdb.mi/mi2-var-child.exp: path expression for v3.x
-PASS: gdb.mi/mi2-var-child.exp: expression for v3.x
-PASS: gdb.mi/mi2-var-child.exp: VT: list children of v3.1_anonymous
-PASS: gdb.mi/mi2-var-child.exp: path expression for v3.1_anonymous
-PASS: gdb.mi/mi2-var-child.exp: expression for v3.1_anonymous
-PASS: gdb.mi/mi2-var-child.exp: VT: list children of v3.2_anonymous
-PASS: gdb.mi/mi2-var-child.exp: path expression for v3.2_anonymous
-PASS: gdb.mi/mi2-var-child.exp: expression for v3.2_anonymous
-PASS: gdb.mi/mi2-var-child.exp: path expression for v3.1_anonymous.a
-PASS: gdb.mi/mi2-var-child.exp: expression for v3.1_anonymous.a
-PASS: gdb.mi/mi2-var-child.exp: path expression for v3.2_anonymous.b
-PASS: gdb.mi/mi2-var-child.exp: expression for v3.2_anonymous.b
+FAIL: gdb.mi/mi2-var-child.exp: VT: create root varobj for v
+FAIL: gdb.mi/mi2-var-child.exp: VT: list children of v3
+FAIL: gdb.mi/mi2-var-child.exp: path expression for v3
+FAIL: gdb.mi/mi2-var-child.exp: expression for v3
+FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.x
+FAIL: gdb.mi/mi2-var-child.exp: expression for v3.x
+FAIL: gdb.mi/mi2-var-child.exp: VT: list children of v3.1_anonymous
+FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.1_anonymous
+FAIL: gdb.mi/mi2-var-child.exp: expression for v3.1_anonymous
+FAIL: gdb.mi/mi2-var-child.exp: VT: list children of v3.2_anonymous
+FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.2_anonymous
+FAIL: gdb.mi/mi2-var-child.exp: expression for v3.2_anonymous
+FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.1_anonymous.a
+FAIL: gdb.mi/mi2-var-child.exp: expression for v3.1_anonymous.a
+FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.2_anonymous.b
+FAIL: gdb.mi/mi2-var-child.exp: expression for v3.2_anonymous.b
(many more, I did not list them all)

 888-interpreter-exec console "set $pc=0x0"^M
-888^done^M
+=thread-group-exited,id="i1"^M
+&"Remote connection closed\n"^M
+888^error,msg="Remote connection closed"^M
 (gdb) ^M
-PASS: gdb.mi/mi-cli.exp: -interpreter-exec console "set $pc=0x0"
+FAIL: gdb.mi/mi-cli.exp: -interpreter-exec console "set $pc=0x0"

 -var-list-children fooPtr^M
+=thread-group-exited,id="i1"^M
 ^done,numchild="3",children=[child={name="fooPtr.x",exp="x",numchild="0",type="int",thread-id="1"},child={name="fooPtr.y",exp="y",numchild="0",type="int",thread-id="1"},child={name="fooPtr.z",exp="z",numchild="0",type="int",thread-id="1"}],has_more="0"^M
 (gdb) ^M
-PASS: gdb.mi/gdb701.exp: list children of fooPtr
+FAIL: gdb.mi/gdb701.exp: list children of fooPtr

 -var-create v3 * v^M
-^done,name="v3",numchild="3",value="{...}",type="struct {...}",thread-id="1",has_more="0"^M
+^error,msg="-var-create: unable to create variable object"^M
 (gdb) ^M
-PASS: gdb.mi/mi2-var-child.exp: VT: create root varobj for v
+FAIL: gdb.mi/mi2-var-child.exp: VT: create root varobj for v

etc. etc.

Re: Regression for gdbserver [Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues]

[Maciej W. Rozycki]

On Tue, 22 May 2012, Jan Kratochvil wrote:

> On Tue, 22 May 2012 02:05:28 +0200, Maciej W. Rozycki wrote:
> >  Applied now, thanks for the review.
> 
> 272cb31d810a541dcc44f942fabb3167580b838e is the first bad commit
> commit 272cb31d810a541dcc44f942fabb3167580b838e
> Author: Maciej W. Rozycki <macro@linux-mips.org>
> Date:   Mon May 21 23:50:25 2012 +0000
> 
>     	* linux-low.c (linux_store_registers): Don't re-retrieve data
>     	with ptrace that has already been obtained from /proc.  Always
>     	copy any data retrieved with ptrace to the buffer supplied.
> 
> -PASS: gdb.mi/gdb701.exp: list children of fooPtr
> +FAIL: gdb.mi/gdb701.exp: list children of fooPtr
> -PASS: gdb.mi/mi2-var-child.exp: VT: list children of ptr2.*ptr.1_anonymous
> +FAIL: gdb.mi/mi2-var-child.exp: VT: list children of ptr2.*ptr.1_anonymous
> -PASS: gdb.mi/mi2-var-child.exp: VT: create root varobj for v
> -PASS: gdb.mi/mi2-var-child.exp: VT: list children of v3
> -PASS: gdb.mi/mi2-var-child.exp: path expression for v3
> -PASS: gdb.mi/mi2-var-child.exp: expression for v3
> -PASS: gdb.mi/mi2-var-child.exp: path expression for v3.x
> -PASS: gdb.mi/mi2-var-child.exp: expression for v3.x
> -PASS: gdb.mi/mi2-var-child.exp: VT: list children of v3.1_anonymous
> -PASS: gdb.mi/mi2-var-child.exp: path expression for v3.1_anonymous
> -PASS: gdb.mi/mi2-var-child.exp: expression for v3.1_anonymous
> -PASS: gdb.mi/mi2-var-child.exp: VT: list children of v3.2_anonymous
> -PASS: gdb.mi/mi2-var-child.exp: path expression for v3.2_anonymous
> -PASS: gdb.mi/mi2-var-child.exp: expression for v3.2_anonymous
> -PASS: gdb.mi/mi2-var-child.exp: path expression for v3.1_anonymous.a
> -PASS: gdb.mi/mi2-var-child.exp: expression for v3.1_anonymous.a
> -PASS: gdb.mi/mi2-var-child.exp: path expression for v3.2_anonymous.b
> -PASS: gdb.mi/mi2-var-child.exp: expression for v3.2_anonymous.b
> +FAIL: gdb.mi/mi2-var-child.exp: VT: create root varobj for v
> +FAIL: gdb.mi/mi2-var-child.exp: VT: list children of v3
> +FAIL: gdb.mi/mi2-var-child.exp: path expression for v3
> +FAIL: gdb.mi/mi2-var-child.exp: expression for v3
> +FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.x
> +FAIL: gdb.mi/mi2-var-child.exp: expression for v3.x
> +FAIL: gdb.mi/mi2-var-child.exp: VT: list children of v3.1_anonymous
> +FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.1_anonymous
> +FAIL: gdb.mi/mi2-var-child.exp: expression for v3.1_anonymous
> +FAIL: gdb.mi/mi2-var-child.exp: VT: list children of v3.2_anonymous
> +FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.2_anonymous
> +FAIL: gdb.mi/mi2-var-child.exp: expression for v3.2_anonymous
> +FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.1_anonymous.a
> +FAIL: gdb.mi/mi2-var-child.exp: expression for v3.1_anonymous.a
> +FAIL: gdb.mi/mi2-var-child.exp: path expression for v3.2_anonymous.b
> +FAIL: gdb.mi/mi2-var-child.exp: expression for v3.2_anonymous.b
> (many more, I did not list them all)
> 
>  888-interpreter-exec console "set $pc=0x0"^M
> -888^done^M
> +=thread-group-exited,id="i1"^M
> +&"Remote connection closed\n"^M
> +888^error,msg="Remote connection closed"^M
>  (gdb) ^M
> -PASS: gdb.mi/mi-cli.exp: -interpreter-exec console "set $pc=0x0"
> +FAIL: gdb.mi/mi-cli.exp: -interpreter-exec console "set $pc=0x0"
> 
>  -var-list-children fooPtr^M
> +=thread-group-exited,id="i1"^M
>  ^done,numchild="3",children=[child={name="fooPtr.x",exp="x",numchild="0",type="int",thread-id="1"},child={name="fooPtr.y",exp="y",numchild="0",type="int",thread-id="1"},child={name="fooPtr.z",exp="z",numchild="0",type="int",thread-id="1"}],has_more="0"^M
>  (gdb) ^M
> -PASS: gdb.mi/gdb701.exp: list children of fooPtr
> +FAIL: gdb.mi/gdb701.exp: list children of fooPtr
> 
>  -var-create v3 * v^M
> -^done,name="v3",numchild="3",value="{...}",type="struct {...}",thread-id="1",has_more="0"^M
> +^error,msg="-var-create: unable to create variable object"^M
>  (gdb) ^M
> -PASS: gdb.mi/mi2-var-child.exp: VT: create root varobj for v
> +FAIL: gdb.mi/mi2-var-child.exp: VT: create root varobj for v

 I don't have these failures in my test run, which target is that?  Can 
you check that these are not intermittent failures -- I recall at least
gdb.mi/mi2-var-child.exp to be somewhat unreliable (I don't recall 
gdb.mi/gdb701.exp ever causing troubles though).

 I have e.g.:

(gdb)
PASS: gdb.mi/mi-cli.exp: -interpreter-exec console "help set args"
Expecting: ^(888-interpreter-exec console "set \$pc=0x0"[
]+)?(888\^done[
]+[(]gdb[)]
[ ]*)
888-interpreter-exec console "set $pc=0x0"
888^done
(gdb)
PASS: gdb.mi/mi-cli.exp: -interpreter-exec console "set $pc=0x0"
testcase .../gdb/testsuite/gdb.mi/mi-cli.exp completed in 2 seconds

in a full mips-linux-gnu o32/EB test run that has just completed.  I have 
just run gdb.mi/gdb701.exp for all the eight multilibs I've been testing 
recently and got 64 passes total and no failures too:

PASS: gdb.mi/gdb701.exp: breakpoint at main
PASS: gdb.mi/gdb701.exp: mi runto main
PASS: gdb.mi/gdb701.exp: step over "foo = 0"
PASS: gdb.mi/gdb701.exp: create fooPtr
PASS: gdb.mi/gdb701.exp: list children of fooPtr
PASS: gdb.mi/gdb701.exp: list children of fooPtr.x
PASS: gdb.mi/gdb701.exp: list children of fooPtr.y
PASS: gdb.mi/gdb701.exp: list children of fooPtr.z

Actually I checked the long runs too and all their eight multilibs have:

FAIL: gdb.mi/gdb701.exp: mi runto main (unknown output after running)

but all the other gdb.mi/gdb701.exp tests pass, so it looks to me this 
test case is prone to intermittent failures as well.

 The full log is like this:

(gdb)
220-exec-continue
220^running
*running,thread-id="all"
(gdb)
mi_expect_stop: expecting: \*stopped,reason="breakpoint-hit",disp="del",bkptno="[0-9]+",frame={addr="0x[0-9A-Fa-f]+",func="main",args=\[.*\],(?:file="[^
]*.*",fullname="(/[^\n]*/|\\\\[^\\]+\\[^\n]+\\|\\[^\\][^\n]*\\|[a-zA-Z]:[^\n]*\\).*",line="[0-9]+"|from=".*")},thread-id="[0-9]+",stopped-threads=[^
]*
(=thread-selected,id="[0-9]+"
|Breakpoint [0-9]+ at 0x[0-9A-Fa-f]+: file .*func-ptrs.c, line [0-9]+\.)*[(]gdb[)]
$
=library-loaded,id=".../el/lib/../lib64/libm.so.6",target-name=".../el/lib/../lib64/libm.so.6",host-name=".../el/lib/../lib64/libm.so.6",symbols-loaded="0",thread-group="i1"
=library-loaded,id=".../el/lib/../lib64/libc.so.6",target-name=".../el/lib/../lib64/libc.so.6",host-name=".../el/lib/../lib64/libc.so.6",symbols-loaded="0",thread-group="i1"
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x0000000120000a6c",func="main",file=".../gdb/testsuite/gdb.mi/gdb701.c",fullname=".../gdb/testsuite/gdb.mi/gdb701.c",line="13",times="1",original-location="main"}
*stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={addr="0x0000000120000a6c",func="main",args=[{name="argc",value="1"},{name="argv",value="0xffffffb8b8"}],file=".../gdb/testsuite/gdb.mi/gdb701.c",fullname=".../gdb/testsuite/gdb.mi/gdb701.c",line="13"},thread-id="1",stopped-threads="all",core="0"
=breakpoint-deleted,id="1"
(gdb)
got =library-loaded,id=".../el/lib/../lib64/libm.so.6",target-name=".../el/lib/../lib64/libm.so.6",host-name=".../el/lib/../lib64/libm.so.6",symbols-loaded="0",thread-group="i1"
=library-loaded,id=".../el/lib/../lib64/libc.so.6",target-name=".../el/lib/../lib64/libc.so.6",host-name=".../el/lib/../lib64/libc.so.6",symbols-loaded="0",thread-group="i1"
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x0000000120000a6c",func="main",file=".../gdb/testsuite/gdb.mi/gdb701.c",fullname=".../gdb/testsuite/gdb.mi/gdb701.c",line="13",times="1",original-location="main"}
*stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={addr="0x0000000120000a6c",func="main",args=[{name="argc",value="1"},{name="argv",value="0xffffffb8b8"}],file=".../gdb/testsuite/gdb.mi/gdb701.c",fullname=".../gdb/testsuite/gdb.mi/gdb701.c",line="13"},thread-id="1",stopped-threads="all",core="0"
=breakpoint-deleted,id="1"
(gdb)

FAIL: gdb.mi/gdb701.exp: mi runto main (unknown output after running)

which is weird as it looks that my newly-added gdb.base/func-ptrs.exp test 
case (posted here recently) has overwritten a lib/mi-support.exp variable.  
It shouldn't really happen, from it I infer the test cases are not run in 
a subprocedure context each, but as a flat execution flow.  That's 
certainly begging for troubles and could explain some failures like the 
above that only happen when the test suite is run as a whole, but not with 
individual test cases.

 I'll look into it.

  Maciej

[patch] Re: Regression for gdbserver [Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues]

[Jan Kratochvil]

On Tue, 22 May 2012 14:43:21 +0200, Maciej W. Rozycki wrote:
>  I don't have these failures in my test run, which target is that?

Normal Fedora 17 x86_64 with:
	--with-system-zlib --enable-64-bit-bfd --enable-targets=all --enable-static --disable-shared --enable-debug --disable-sim --enable-gold --enable-plugins --with-separate-debug-dir=/usr/lib/debug --disable-option-checking --build=x86_64-unknown-linux-gnu --host=x86_64-unknown-linux-gnu --target=x86_64-unknown-linux-gnu CFLAGS="-m64 -g3 -pipe -Wall -fexceptions -fstack-protector --param=ssp-buffer-size=4" LDFLAGS="-lmcheck"
that is from my
	http://git.jankratochvil.net/?p=nethome.git;a=blob_plain;f=bin/errs12;hb=master


> Can you check that these are not intermittent failures

It was very 0% vs. 100% reproducible and for the single testcase.

No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu for gdbserver
mode.

I will check it in yet today as the HEAD is broken now.


Regards,
Jan

gdb/gdbserver/
2012-05-22  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* linux-low.c (linux_read_memory): Fix final memcpy size.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 19f7be6..c33c976 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4450,10 +4450,12 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 
   /* Copy appropriate bytes out of the buffer.  */
   i *= sizeof (PTRACE_XFER_TYPE);
-  i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
-  memcpy (myaddr,
-	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
-	  i < len ? i : len);
+  if (i > 0)
+    memcpy (myaddr,
+	    (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
+	    (MIN ((memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE)) + i,
+		  memaddr + len)
+	     - memaddr));
 
   return ret;
 }

Re: [patch] Re: Regression for gdbserver [Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues]

[Maciej W. Rozycki]

On Tue, 22 May 2012, Jan Kratochvil wrote:

> > Can you check that these are not intermittent failures
> 
> It was very 0% vs. 100% reproducible and for the single testcase.

 Oh well, thanks for double-checking, it's difficult to chase something 
you can't reproduce.

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 19f7be6..c33c976 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4450,10 +4450,12 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>  
>    /* Copy appropriate bytes out of the buffer.  */
>    i *= sizeof (PTRACE_XFER_TYPE);
> -  i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
> -  memcpy (myaddr,
> -	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
> -	  i < len ? i : len);
> +  if (i > 0)
> +    memcpy (myaddr,
> +	    (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
> +	    (MIN ((memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE)) + i,
> +		  memaddr + len)
> +	     - memaddr));
>  
>    return ret;
>  }

 Doh, it looks like my final correction of the copy size (not to overrun 
the buffer supplied) missed the zero case, sorry and thanks for catching 
this.  Presumably your alignment is different or suchlike.

 I think however, that this memcpy call needs a rewrite now, I find your 
proposal unreadable.  Would you please make use of an auxiliary variable 
or suchlike to make this calculation?  Also can we rely on MIN?  It's not 
a standard macro AFAIK and we don't define our own copy, except from one 
place in gdb/target.c (I checked before making the original change 
already).

 Perhaps a piece like this would actually be enough, i.e. the original 
code, only guarded against zero, where we're not going to copy anything 
anyway, so we can skip the whole calculation altogether:

  /* Copy appropriate bytes out of the buffer.  */
  if (i > 0)
    {
      i *= sizeof (PTRACE_XFER_TYPE);
      i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
      memcpy (myaddr,
	      (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
	      i < len ? i : len);
    }

?

  Maciej

Re: [patch] Re: Regression for gdbserver [Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues]

[Jan Kratochvil]

On Tue, 22 May 2012 22:05:30 +0200, Maciej W. Rozycki wrote:
> it's difficult to chase something you can't reproduce.

I see that patch
	[RFC patch] non-release srctrees: --enable-targets=all & 64bit & -lmcheck
	http://sourceware.org/ml/gdb-patches/2012-05/msg00714.html

should include also better CFLAGS.  But the patch does not seem to go in so
far so we may continue with mail threads like this one.


>  I think however, that this memcpy call needs a rewrite now, I find your 
> proposal unreadable.

I find the whole function unreadable but this was true also before your patch.
But I did not try to change that in this fix up.  It also has 64-bit unsafe
bug using 'int' for memory sizes.

I find always more clear to calculate everything as START ADDRESS and ONE BYTE
AFTER THE LAST ADDRESS till the very last moment.


>   /* Copy appropriate bytes out of the buffer.  */
>   if (i > 0)
>     {
>       i *= sizeof (PTRACE_XFER_TYPE);
>       i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
>       memcpy (myaddr,
> 	      (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
> 	      i < len ? i : len);
>     }
> 
> ?

This code has equal functionality in my local testing.


Regards,
Jan

Re: [patch] Re: Regression for gdbserver [Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues]

[Maciej W. Rozycki]

On Tue, 22 May 2012, Jan Kratochvil wrote:

> > it's difficult to chase something you can't reproduce.
> 
> I see that patch
> 	[RFC patch] non-release srctrees: --enable-targets=all & 64bit & -lmcheck
> 	http://sourceware.org/ml/gdb-patches/2012-05/msg00714.html
> 
> should include also better CFLAGS.  But the patch does not seem to go in so
> far so we may continue with mail threads like this one.

 Hmm, it wouldn't have triggered at the compilation time probably anyway.  
I have switched to --enable-targets=all already, based on some past 
experience.

> >  I think however, that this memcpy call needs a rewrite now, I find your 
> > proposal unreadable.
> 
> I find the whole function unreadable but this was true also before your patch.
> But I did not try to change that in this fix up.  It also has 64-bit unsafe
> bug using 'int' for memory sizes.

 As noted in the original thread, this whole stuff asks for a rewrite and 
that will be a good opportunity to make it more readable too.

 The 'int' bug hardly ever triggers probably, as you'd have to request 
more than 2GB in one go that is I believe very rare, but I agree that 
should be size_t instead.  You'd have to check the callers if they don't 
cope with that limitation already somehow however -- though I think it's 
unlikely and I am too lazy to go chase it right now.  It could be that 
this is how the RSP has been specified too.
 
> I find always more clear to calculate everything as START ADDRESS and ONE BYTE
> AFTER THE LAST ADDRESS till the very last moment.

 That indeed, or START & SIZE in bytes.

> >   /* Copy appropriate bytes out of the buffer.  */
> >   if (i > 0)
> >     {
> >       i *= sizeof (PTRACE_XFER_TYPE);
> >       i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
> >       memcpy (myaddr,
> > 	      (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
> > 	      i < len ? i : len);
> >     }
> > 
> > ?
> 
> This code has equal functionality in my local testing.

 And has passed my regression testing on mips-linux-gnu and i686-linux-gnu 
as well.  It did fix a number of failures on the latter, sorry for not 
testing my change on another target before, I should have.  I have now 
checked it in, the actual diff follows.

2012-05-22  Maciej W. Rozycki  <macro@codesourcery.com>

	* linux-low.c (linux_store_registers): Avoid the copying sequence
	when no data has been retrieved by ptrace.

  Maciej

gdb-gdbserver-linux-read-memory-ptrace-fix.diff
Index: gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/gdbserver/linux-low.c	2012-05-22 19:20:59.000000000 +0100
+++ gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c	2012-05-22 21:15:36.545454255 +0100
@@ -4447,11 +4447,14 @@ linux_read_memory (CORE_ADDR memaddr, un
   ret = errno;
 
   /* Copy appropriate bytes out of the buffer.  */
-  i *= sizeof (PTRACE_XFER_TYPE);
-  i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
-  memcpy (myaddr,
-	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
-	  i < len ? i : len);
+  if (i > 0)
+    {
+      i *= sizeof (PTRACE_XFER_TYPE);
+      i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
+      memcpy (myaddr,
+	      (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
+	      i < len ? i : len);
+    }
 
   return ret;
 }

Re: [patch] Re: Regression for gdbserver [Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues]

[Jan Kratochvil]

On Wed, 23 May 2012 01:34:05 +0200, Maciej W. Rozycki wrote:
> On Tue, 22 May 2012, Jan Kratochvil wrote:
> > 	[RFC patch] non-release srctrees: --enable-targets=all & 64bit & -lmcheck
> > 	http://sourceware.org/ml/gdb-patches/2012-05/msg00714.html
[...]
>  Hmm, it wouldn't have triggered at the compilation time probably anyway.  
> I have switched to --enable-targets=all already, based on some past 
> experience.

I was trying to figure out why the problem is reproducible for me and not for
you.  I understand --enable-targets=all does not affect it but as the patch
already makes development vs. user build difference we could add even more
tweaks like special CFLAGS into the development build.  (Assuming special
CFLAGS can reproduce the bug.)


> > I find always more clear to calculate everything as START ADDRESS and ONE BYTE
> > AFTER THE LAST ADDRESS till the very last moment.
> 
>  That indeed, or START & SIZE in bytes.

The key is to never use any SIZE because then after you modify START
everything breaks down.


Regards,
Jan