This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Re: Regression for gdbserver [Re: [PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues]
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;
}