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

["Maciej W. Rozycki" <macro@codesourcery.com>]

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;
 }