Re: [PATCH 1/3] inf-ptrace: Do not stop memory transfers after a single word

Simon Marchi
2017-03-08T19:10:01Z

On 17-03-06 11:00 AM, Andreas Arnez wrote:
> When inf_ptrace_xfer_partial performs a memory transfer via ptrace with
> PT_READ_I, PT_WRITE_I (aka PTRACE_PEEKTEXT, PTRACE_POKETEXT), etc., then
> it currently transfers at most one word. This behavior yields degraded
> performance, particularly if the caller has significant preparation work
> for each invocation. And indeed it has for writing, in
> memory_xfer_partial in target.c, where all of the remaining data to be
> transferred is copied to a temporary buffer each time, for breakpoint
> shadow handling. Thus large writes have quadratic runtime and can take
> hours.
>
> Note: On GNU/Linux targets GDB usually does not use
> inf_ptrace_xfer_partial for large memory *reads* from the inferior, but
> attempts a single read from /proc/<pid>/mem instead. However, this is not
> currently true for memory *writes*. So the problem occurs on GNU/Linux
> when transferring large amounts of data *into* the inferior.
>
> This patch fixes the performance issue by attempting to fulfill the whole
> transfer request in inf_ptrace_xfer_partial, using a loop around the
> ptrace call.

I think the idea is good. The xfer partial interface says that the target should
transfer up to len bytes. Transferring 1 word at the time respects the contract,
but we should try to be more efficient when possible.

> gdb/ChangeLog:
>
> PR gdb/21220
> * inf-ptrace.c (inf_ptrace_xfer_partial): For memory transfers,
> loop over ptrace peek/poke until end of buffer or error.
> ---
> gdb/inf-ptrace.c | 115 ++++++++++++++++++++++++-------------------------------
> 1 file changed, 51 insertions(+), 64 deletions(-)
>
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index 21578742..2989259 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -492,77 +492,64 @@ inf_ptrace_xfer_partial (struct target_ops *ops, enum target_object object,
> }
> #endif
> {
> - union
> - {
> - PTRACE_TYPE_RET word;
> - gdb_byte byte[sizeof (PTRACE_TYPE_RET)];
> - } buffer;
> - ULONGEST rounded_offset;
> - ULONGEST partial_len;
> -
> - /* Round the start offset down to the next long word
> - boundary. */
> - rounded_offset = offset & -(ULONGEST) sizeof (PTRACE_TYPE_RET);
> -
> - /* Since ptrace will transfer a single word starting at that
> - rounded_offset the partial_len needs to be adjusted down to
> - that (remember this function only does a single transfer).
> - Should the required length be even less, adjust it down
> - again. */
> - partial_len = (rounded_offset + sizeof (PTRACE_TYPE_RET)) - offset;
> - if (partial_len > len)
> - partial_len = len;
> -
> - if (writebuf)
> + ULONGEST n;
> + unsigned chunk;

"unsigned" -> "unsigned int"?

> +
> + /* We transfer aligned words. Thus align OFFSET down to a word
> + boundary and determine how many bytes to skip at the
> + beginning. */
> + unsigned skip = offset & (sizeof (PTRACE_TYPE_RET) - 1);
> + offset -= skip;
> +
> + for (n = 0;
> + n < len;
> + n += chunk, offset += sizeof (PTRACE_TYPE_RET), skip = 0)
> {
> - /* If OFFSET:PARTIAL_LEN is smaller than
> - ROUNDED_OFFSET:WORDSIZE then a read/modify write will
> - be needed. Read in the entire word. */
> - if (rounded_offset < offset
> - || (offset + partial_len
> - < rounded_offset + sizeof (PTRACE_TYPE_RET)))
> - /* Need part of initial word -- fetch it. */
> - buffer.word = ptrace (PT_READ_I, pid,
> - (PTRACE_TYPE_ARG3)(uintptr_t)
> - rounded_offset, 0);
> -
> - /* Copy data to be written over corresponding part of
> - buffer. */
> - memcpy (buffer.byte + (offset - rounded_offset),
> - writebuf, partial_len);
> -
> - errno = 0;
> - ptrace (PT_WRITE_D, pid,
> - (PTRACE_TYPE_ARG3)(uintptr_t)rounded_offset,
> - buffer.word);
> - if (errno)
> + /* Restrict to a chunk that fits in the current word. */
> + chunk = std::min (sizeof (PTRACE_TYPE_RET) - skip, len - n);
> +
> + /* Use a union for type punning. */
> + union
> + {
> + PTRACE_TYPE_RET word;
> + gdb_byte byte[sizeof (PTRACE_TYPE_RET)];
> + } buf;
> +
> + /* Read the word, also when doing a partial word write. */
> + if (readbuf || chunk < sizeof (PTRACE_TYPE_RET))

Use != NULL or == NULL when checking pointers.

> {
> - /* Using the appropriate one (I or D) is necessary for
> - Gould NP1, at least. */
> errno = 0;
> - ptrace (PT_WRITE_I, pid,
> - (PTRACE_TYPE_ARG3)(uintptr_t)rounded_offset,
> - buffer.word);
> + buf.word = ptrace (PT_READ_I, pid,
> + (PTRACE_TYPE_ARG3)(uintptr_t) offset,
> + 0);
> if (errno)
> - return TARGET_XFER_EOF;
> + break;
> + if (readbuf)
> + memcpy (readbuf + n, buf.byte + skip, chunk);
> + }
> + if (writebuf)
> + {
> + memcpy (buf.byte + skip, writebuf + n, chunk);
> + errno = 0;
> + ptrace (PT_WRITE_D, pid,
> + (PTRACE_TYPE_ARG3)(uintptr_t) offset,
> + buf.word);
> + if (errno)
> + {
> + /* Using the appropriate one (I or D) is necessary for
> + Gould NP1, at least. */
> + errno = 0;
> + ptrace (PT_WRITE_I, pid,
> + (PTRACE_TYPE_ARG3)(uintptr_t) offset,
> + buf.word);
> + if (errno)
> + break;
> + }
> }
> }
>
> - if (readbuf)
> - {
> - errno = 0;
> - buffer.word = ptrace (PT_READ_I, pid,
> - (PTRACE_TYPE_ARG3)(uintptr_t)rounded_offset,
> - 0);
> - if (errno)
> - return TARGET_XFER_EOF;
> - /* Copy appropriate bytes out of the buffer. */
> - memcpy (readbuf, buffer.byte + (offset - rounded_offset),
> - partial_len);
> - }
> -
> - *xfered_len = partial_len;
> - return TARGET_XFER_OK;
> + *xfered_len = n;
> + return n ? TARGET_XFER_OK : TARGET_XFER_EOF;

This is not a comment specifically about your patch, since that's how it was already, but
maybe it would be a good time to address this. I understand there's some level of overlap
between the read and write (like the offset/skip computation), but I don't think that
handling reading and writing in the same loop is very readable. It just adds a bunch of
branches and makes it hard to follow. If that code was split in two functions (one for
read, one for write), it would be way more straightforward.

Simon