[PATCH v2] inf-ptrace: Do not stop memory transfers after a single word

[PATCH v2] inf-ptrace: Do not stop memory transfers after a single word

[Andreas Arnez]

Version 1 is here:

  https://sourceware.org/ml/gdb-patches/2017-03/msg00043.html

Changes from version 1:

* "unsigned" -> "unsigned int"

* Compare pointers with NULL explicitly.

* Move the ptrace peek/poke loop to a separate routine for better
  readability.

OK to apply?

--
Andreas

-- >8 --
Subject: [PATCH v2] inf-ptrace: Do not stop memory transfers after a single
 word

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.

gdb/ChangeLog:

	PR gdb/21220
	* inf-ptrace.c (inf_ptrace_xfer_partial): In "case
	TARGET_OBJECT_MEMORY", extract the logic for ptrace peek/poke...
	(inf_ptrace_peek_poke): ...here.  New function.  Now also loop
	over ptrace peek/poke until end of buffer or error.
---
 gdb/inf-ptrace.c | 142 +++++++++++++++++++++++++++----------------------------
 1 file changed, 69 insertions(+), 73 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 21578742..ce6796b 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -446,6 +446,72 @@ inf_ptrace_wait (struct target_ops *ops,
   return pid_to_ptid (pid);
 }
 
+/* Transfer data via ptrace into process PID's memory from WRITEBUF, or
+   from process PID's memory into READBUF.  Start at target address ADDR
+   and transfer up to LEN bytes.  Exactly one of READBUF and WRITEBUF must
+   be non-null.  Return the number of transferred bytes.  */
+
+static ULONGEST
+inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
+		      const gdb_byte *writebuf,
+		      ULONGEST addr, ULONGEST len)
+{
+  ULONGEST n;
+  unsigned int chunk;
+
+  /* We transfer aligned words.  Thus align ADDR down to a word
+     boundary and determine how many bytes to skip at the
+     beginning.  */
+  unsigned int skip = addr & (sizeof (PTRACE_TYPE_RET) - 1);
+  addr -= skip;
+
+  for (n = 0;
+       n < len;
+       n += chunk, addr += sizeof (PTRACE_TYPE_RET), skip = 0)
+    {
+      /* 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 != NULL || chunk < sizeof (PTRACE_TYPE_RET))
+	{
+	  errno = 0;
+	  buf.word = ptrace (PT_READ_I, pid,
+			     (PTRACE_TYPE_ARG3)(uintptr_t) addr, 0);
+	  if (errno)
+	    break;
+	  if (readbuf != NULL)
+	    memcpy (readbuf + n, buf.byte + skip, chunk);
+	}
+      if (writebuf != NULL)
+	{
+	  memcpy (buf.byte + skip, writebuf + n, chunk);
+	  errno = 0;
+	  ptrace (PT_WRITE_D, pid, (PTRACE_TYPE_ARG3)(uintptr_t) addr,
+		  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) addr,
+		      buf.word);
+	      if (errno)
+		break;
+	    }
+	}
+    }
+
+  return n;
+}
+
 /* Implement the to_xfer_partial target_ops method.  */
 
 static enum target_xfer_status
@@ -491,79 +557,9 @@ inf_ptrace_xfer_partial (struct target_ops *ops, enum target_object object,
 	  return TARGET_XFER_EOF;
       }
 #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)
-	  {
-	    /* 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)
-	      {
-		/* 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);
-		if (errno)
-		  return TARGET_XFER_EOF;
-	      }
-	  }
-
-	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 = inf_ptrace_peek_poke (pid, readbuf, writebuf,
+					  offset, len);
+      return *xfered_len ? TARGET_XFER_OK : TARGET_XFER_EOF;
 
     case TARGET_OBJECT_UNWIND_TABLE:
       return TARGET_XFER_E_IO;
-- 
2.5.0

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

[Pedro Alves]

On 03/14/2017 03:44 PM, Andreas Arnez wrote:
> Version 1 is here:
> 
>   https://sourceware.org/ml/gdb-patches/2017-03/msg00043.html
> 
> Changes from version 1:
> 
> * "unsigned" -> "unsigned int"
> 
> * Compare pointers with NULL explicitly.

Pedantically, it should be "if (errno != 0)" and
"*xfered_len != 0 ? ...."  as well.

> 
> * Move the ptrace peek/poke loop to a separate routine for better
>   readability.
> 
> OK to apply?

LGTM.

Thanks,
Pedro Alves

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

[Andreas Arnez]

On Tue, Mar 14 2017, Pedro Alves wrote:

> On 03/14/2017 03:44 PM, Andreas Arnez wrote:
>> Version 1 is here:
>> 
>>   https://sourceware.org/ml/gdb-patches/2017-03/msg00043.html
>> 
>> Changes from version 1:
>> 
>> * "unsigned" -> "unsigned int"
>> 
>> * Compare pointers with NULL explicitly.
>
> Pedantically, it should be "if (errno != 0)" and
> "*xfered_len != 0 ? ...."  as well.

OK, changed those as well.

>
>> 
>> * Move the ptrace peek/poke loop to a separate routine for better
>>   readability.
>> 
>> OK to apply?
>
> LGTM.

Thanks!

I noticed that the commit message is now slightly wrong, because GDB now
*does* exploit writing to /proc/<pid>/mem.  Fixed the commit message
accordingly.

Pushed with the above changes.

--
Andreas

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

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

On 14/03/17 15:44, Andreas Arnez wrote:
> +      /* Restrict to a chunk that fits in the current word.  */
> +      chunk = std::min (sizeof (PTRACE_TYPE_RET) - skip, len - n);
>
On 14/03/17 15:44, Andreas Arnez wrote:
>
> +      /* Restrict to a chunk that fits in the current word.  */
> +      chunk = std::min (sizeof (PTRACE_TYPE_RET) - skip, len - n);

Hi Andres,

I am seeing the following build failure on my local AArch32 native build:

...binutils-gdb/gdb/inf-ptrace.c:473:65: error: no matching function for call to 'min(unsigned int, ULONGEST)'

There are a few ways to fix this, this patch simply change the type of "skip" from "unsigned int" to "ULONGEST" so operands for std::min can have the same type.

OK for master?

gdb/
2017-03-16  Jiong Wang  <jiong.wang@arm.com>

         * inf-ptrace.c (inf_ptrace_peek_poke): Change the type to "ULONGEST" for
         "skip".


[-- Attachment #2: fix.patch --]
[-- Type: text/x-diff, Size: 562 bytes --]

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 32794ec132f72c261debd329196c73a4e3cff75b..431a36b8c726f8475650de00bf895b8d5f48fa83 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -462,7 +462,7 @@ inf_ptrace_peek_poke (pid_t pid, gdb_byte *readbuf,
   /* We transfer aligned words.  Thus align ADDR down to a word
      boundary and determine how many bytes to skip at the
      beginning.  */
-  unsigned int skip = addr & (sizeof (PTRACE_TYPE_RET) - 1);
+  ULONGEST skip = addr & (sizeof (PTRACE_TYPE_RET) - 1);
   addr -= skip;
 
   for (n = 0;

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

[Yao Qi]

Jiong Wang <jiong.wang@foss.arm.com> writes:

> I am seeing the following build failure on my local AArch32 native build:
>
> ...binutils-gdb/gdb/inf-ptrace.c:473:65: error: no matching function
> for call to 'min(unsigned int, ULONGEST)'
>
> There are a few ways to fix this, this patch simply change the type of
> "skip" from "unsigned int" to "ULONGEST" so operands for std::min can
> have the same type.
>
> OK for master?
>

Yes, pleas apply, a nit below,

> gdb/
> 2017-03-16  Jiong Wang  <jiong.wang@arm.com>
>
>         * inf-ptrace.c (inf_ptrace_peek_poke): Change the type to "ULONGEST" for

This line is too long.  The max line length of ChangeLog is 74.

>         "skip".


-- 
Yao (齐尧)

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

[Andreas Arnez]

On Thu, Mar 16 2017, Jiong Wang wrote:

> I am seeing the following build failure on my local AArch32 native build:
>
> ...binutils-gdb/gdb/inf-ptrace.c:473:65: error: no matching function for
> call to 'min(unsigned int, ULONGEST)'

Hm, sorry for the breakage.

> There are a few ways to fix this, this patch simply change the type of
> "skip" from "unsigned int" to "ULONGEST" so operands for std::min can have
> the same type.

Right, that's a possibility, although the reader may be confused why a
variable whose value is known to be less than 8 needs to be declared as
ULONGEST...

--
Andreas