[PATCH 0/3] PR gdb/21220: Fix quadratic runtime of memory writes into inferior on GNU/Linux

[PATCH 0/3] PR gdb/21220: Fix quadratic runtime of memory writes into inferior on GNU/Linux

[Andreas Arnez]

On GNU/Linux native targets, when writing into inferior memory, only a
single word is currently transferred at a time.  This causes trouble with
large transfers, because memory_xfer_partial wastes a lot of effort in
this case: allocate a buffer and copy all remaining bytes of the transfer
request into it, then fill the breakpoints in, but just transfer the first
word of this buffer, and drop the rest.  This quadratic behavior affects
large "restore" operations, as reported by PR 21220, as well as
reverse-stepping a large memory-copy instruction, such as in the
s390-specific test case s390-mvcle.exp.  The quadratic run-time of a large
restore operation was already addressed and circumvented by this patch:

  https://sourceware.org/ml/gdb-patches/2013-07/msg00611.html -- "Improve
  performance of large restore commands"

But then another patch broke the circumvention:

  https://sourceware.org/ml/gdb-patches/2016-06/msg00565.html -- "Optimize
  memory_xfer_partial for remote"

This series fixes the problem in two different ways:

* When using ptrace with PTRACE_POKEDATA, do not return to the caller
  after transferring a single word, but attempt to fulfill the whole
  transfer request in one invocation.

* If possible, do not even use PTRACE_POKEDATA, but write to
  /proc/<pid>/mem instead.

To avoid another regression, a new dump/restore test case is added as
well.

Andreas Arnez (3):
  inf-ptrace: Do not stop memory transfers after a single word
  Test case for dump/restore of large array
  linux-nat: Exploit /proc/<pid>/mem for writing

 gdb/inf-ptrace.c                | 115 ++++++++++++++++++----------------------
 gdb/linux-nat.c                 |  32 +++++------
 gdb/testsuite/gdb.base/dump.c   |  37 +++++++++++++
 gdb/testsuite/gdb.base/dump.exp |   8 +++
 4 files changed, 112 insertions(+), 80 deletions(-)

-- 
2.3.0

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

[Andreas Arnez]

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): 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;
+
+	/* 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))
 	      {
-		/* 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;
       }
 
     case TARGET_OBJECT_UNWIND_TABLE:
-- 
2.3.0

[PATCH 2/3] Test case for dump/restore of large array

[Andreas Arnez]

This adds a test case to dump.exp that dumps and restores a large array.
In particular it verifies that the dump and restore operations are
completed in reasonable time.

gdb/testsuite/ChangeLog:

	PR gdb/21220
	* gdb.base/dump.c (bigarray): New variable.
	(zero_all): Clear bigarray as well.
	(func, fill_bigarray, cmp_bigarray): New functions.
	(main): Call fill_bigarray.
	* gdb.base/dump.exp: Add test for dump/restore of a large array.
---
 gdb/testsuite/gdb.base/dump.c   | 37 +++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/dump.exp |  8 ++++++++
 2 files changed, 45 insertions(+)

diff --git a/gdb/testsuite/gdb.base/dump.c b/gdb/testsuite/gdb.base/dump.c
index bdcafbf..fdeefa8 100644
--- a/gdb/testsuite/gdb.base/dump.c
+++ b/gdb/testsuite/gdb.base/dump.c
@@ -3,6 +3,9 @@
 #define ARRSIZE 32
 int intarray[ARRSIZE], intarray2[ARRSIZE];
 
+#define BIGSIZE 16777216
+unsigned char bigarray[BIGSIZE];
+
 struct teststruct {
   int a;
   int b;
@@ -25,6 +28,38 @@ zero_all ()
   memset ((char *) &intarray2,  0, sizeof (intarray2));
   memset ((char *) &intstruct,  0, sizeof (intstruct));
   memset ((char *) &intstruct2, 0, sizeof (intstruct2));
+  memset ((char *) &bigarray,	0, sizeof (bigarray));
+}
+
+static unsigned char
+func (unsigned u)
+{
+  unsigned char a = u;
+  unsigned char b = (u >> 8);
+  unsigned char c = (u >> 16);
+  unsigned char d = (u >> 24);
+
+  return (a ^ b ^ c ^ d);
+}
+
+int
+fill_bigarray ()
+{
+  unsigned i;
+
+  for (i = 0; i < sizeof (bigarray); i++)
+    bigarray[i] = func (i);
+}
+
+int
+cmp_bigarray ()
+{
+  unsigned i;
+
+  for (i = 0; i < sizeof (bigarray); i++)
+    if (bigarray[i] != func (i))
+      return 0;
+  return 1;
 }
 
 int
@@ -43,6 +78,8 @@ main()
   intstruct.f = 12 * 6;
   intstruct.g = 12 * 7;
 
+  fill_bigarray ();
+
   checkpoint1 ();
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/dump.exp b/gdb/testsuite/gdb.base/dump.exp
index e67e1aa..170a09f 100644
--- a/gdb/testsuite/gdb.base/dump.exp
+++ b/gdb/testsuite/gdb.base/dump.exp
@@ -74,6 +74,7 @@ set all_files {
     intstr2.bin intstr2b.bin intstr2.ihex
     intstr2.srec intstr2.tekhex intstr2.verilog
     intarr3.srec
+    bigarr1.bin
 }
 
 # This loop sets variables dynamically -- each name listed in
@@ -263,6 +264,9 @@ make_dump_file \
     "dump srec mem [set intarr3.srec] &intarray \(char *\) &intarray + sizeof intarray" \
 	"dump array as mem, srec, expressions"
 
+make_dump_file "dump bin mem [set bigarr1.bin] &bigarray &bigarray\[sizeof bigarray\]" \
+	"dump big array as memory, default"
+
 proc test_restore_saved_value { restore_args msg oldval newval } {
     global gdb_prompt
     
@@ -348,6 +352,10 @@ test_restore_saved_value "[set intstr2.bin] binary $struct_start" \
 	"struct as memory, binary" \
 	$struct_val "intstruct"
 
+test_restore_saved_value "[set bigarr1.bin] binary &bigarray" \
+        "reload big array as memory, binary" \
+	1 "cmp_bigarray ()"
+
 # test restore with offset.
 
 set array2_start   [capture_value "/x &intarray2\[0\]"]
-- 
2.3.0

[PATCH 3/3] linux-nat: Exploit /proc/<pid>/mem for writing

[Andreas Arnez]

So far linux_proc_xfer_partial refused to handle write requests.  This is
still based on the assumption that the Linux kernel does not support
writes to /proc/<pid>/mem.  That used to be true, but has changed with
Linux 2.6.39 released in May 2011.

This patch lifts this restriction and now exploits /proc/<pid>/mem for
writing to inferior memory as well, if possible.

gdb/ChangeLog:

	* linux-nat.c (linux_proc_xfer_partial): Handle write operations
	as well.
---
 gdb/linux-nat.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index c58ed83..73ef2d4 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3978,10 +3978,9 @@ linux_child_pid_to_exec_file (struct target_ops *self, int pid)
   return linux_proc_pid_to_exec_file (pid);
 }
 
-/* Implement the to_xfer_partial interface for memory reads using the /proc
-   filesystem.  Because we can use a single read() call for /proc, this
-   can be much more efficient than banging away at PTRACE_PEEKTEXT,
-   but it doesn't support writes.  */
+/* Implement the to_xfer_partial target method using /proc/<pid>/mem.
+   Because we can use a single read/write call, this can be much more
+   efficient than banging away at PTRACE_PEEKTEXT.  */
 
 static enum target_xfer_status
 linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
@@ -3993,7 +3992,7 @@ linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
   int fd;
   char filename[64];
 
-  if (object != TARGET_OBJECT_MEMORY || !readbuf)
+  if (object != TARGET_OBJECT_MEMORY)
     return TARGET_XFER_EOF;
 
   /* Don't bother for one word.  */
@@ -4004,26 +4003,27 @@ linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
      thread.  That requires some juggling, but is even faster.  */
   xsnprintf (filename, sizeof filename, "/proc/%d/mem",
 	     ptid_get_pid (inferior_ptid));
-  fd = gdb_open_cloexec (filename, O_RDONLY | O_LARGEFILE, 0);
+  fd = gdb_open_cloexec (filename, ((readbuf ? O_RDONLY : O_WRONLY)
+				    | O_LARGEFILE), 0);
   if (fd == -1)
     return TARGET_XFER_EOF;
 
-  /* If pread64 is available, use it.  It's faster if the kernel
-     supports it (only one syscall), and it's 64-bit safe even on
-     32-bit platforms (for instance, SPARC debugging a SPARC64
-     application).  */
+  /* Use pread64/pwrite64 if available, since they save a syscall and can
+     handle 64-bit offsets even on 32-bit platforms (for instance, SPARC
+     debugging a SPARC64 application).  */
 #ifdef HAVE_PREAD64
-  if (pread64 (fd, readbuf, len, offset) != len)
+  ret = (readbuf ? pread64 (fd, readbuf, len, offset)
+	 : pwrite64 (fd, writebuf, len, offset));
 #else
-  if (lseek (fd, offset, SEEK_SET) == -1 || read (fd, readbuf, len) != len)
+  ret = lseek (fd, offset, SEEK_SET);
+  if (ret != -1)
+    ret = (readbuf ? read (fd, readbuf, len)
+	   : write (fd, writebuf, len));
 #endif
-    ret = 0;
-  else
-    ret = len;
 
   close (fd);
 
-  if (ret == 0)
+  if (ret == -1 || ret == 0)
     return TARGET_XFER_EOF;
   else
     {
-- 
2.3.0

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

[Simon Marchi]

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

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

[Andreas Arnez]

Simon,

Thanks for your comments!

On Wed, Mar 08 2017, Simon Marchi wrote:

> On 17-03-06 11:00 AM, Andreas Arnez wrote:

[...]

>> 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.

Right, and I think the function now behaves more like you would expect
(https://en.wikipedia.org/wiki/Principle_of_least_astonishment).  Maybe
at some point we should also fix the discrepancy between fulfilling the
contract and still not working correctly, but that is another story.

[...]

>> +	unsigned chunk;
>
> "unsigned" -> "unsigned int"?

OK.

[...]

>> +	    /* Read the word, also when doing a partial word write.  */
>> +	    if (readbuf || chunk < sizeof (PTRACE_TYPE_RET))
>
> Use != NULL or == NULL when checking pointers.

OK.  (I thought I've seen patches that stopped following this rule after
the C++ transition.) (1)

[...]

> 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.

That's probably a matter of taste.  Note that we do have separate
routines in gdbserver/linux-low.c that fulfill the equivalent function:
linux_read_memory() and linux_write_memory().  IMO they have even worse
readability *plus* suffer from heavy code duplication.  Maybe that's
just me, though.

Another thought that crossed my mind is whether we should extract the
whole peek/poke loop into a separate function instead of packing all the
logic under a case statement.  So far I didn't, because I wanted to keep
the bug fix small.

--
Andreas


(1) The GDB C/C++ coding standards provide a dubious explanation for the
"NULL Is Not Zero" rule
(https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#NULL_Is_Not_Zero):

  "Zero constant (0) is not interchangeable with a null pointer constant
  (NULL) anywhere. GCC does not give a warning for such interchange."

To me this seems to imply that the language does not support the
interchangeability.  But according to the C standard, it does:

  "An integer constant expression with the value 0, or such an
  expression cast to type void *, is called a null pointer constant."

C++ has a similar definition and specifies boolean conversion from
pointer types as well.  See also Stroustrup's style FAQ "should I use
NULL or 0?": http://www.stroustrup.com/bs_faq2.html#null

So maybe we want to support non-conforming compilers?  Or is this in
fact a GDB-specific style rule?  In either case we should adjust the
explanation, I think.

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

[Simon Marchi]

On 17-03-09 12:22 PM, Andreas Arnez wrote:
>> 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.
> 
> That's probably a matter of taste.  Note that we do have separate
> routines in gdbserver/linux-low.c that fulfill the equivalent function:
> linux_read_memory() and linux_write_memory().  IMO they have even worse
> readability *plus* suffer from heavy code duplication.  Maybe that's
> just me, though.

It's very possible, I just had this thought while reading the patch, I haven't actually tried.

> Another thought that crossed my mind is whether we should extract the
> whole peek/poke loop into a separate function instead of packing all the
> logic under a case statement.  So far I didn't, because I wanted to keep
> the bug fix small.

IMO it never hurt to split up big functions in smaller, more manageable parts...

> (1) The GDB C/C++ coding standards provide a dubious explanation for the
> "NULL Is Not Zero" rule
> (https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#NULL_Is_Not_Zero):
> 
>   "Zero constant (0) is not interchangeable with a null pointer constant
>   (NULL) anywhere. GCC does not give a warning for such interchange."
> 
> To me this seems to imply that the language does not support the
> interchangeability.  But according to the C standard, it does:
> 
>   "An integer constant expression with the value 0, or such an
>   expression cast to type void *, is called a null pointer constant."
> 
> C++ has a similar definition and specifies boolean conversion from
> pointer types as well.  See also Stroustrup's style FAQ "should I use
> NULL or 0?": http://www.stroustrup.com/bs_faq2.html#null
> 
> So maybe we want to support non-conforming compilers?  Or is this in
> fact a GDB-specific style rule?  In either case we should adjust the
> explanation, I think.
> 

I don't the idea behind that rule.  I thought it was just for readability,
to make it clear that the variable is a pointer without having to refer to the
declaration.  Perhaps some older timers could shed some light on that :).

Simon

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

[Pedro Alves]

On 03/10/2017 03:48 PM, Simon Marchi wrote:

> I don't the idea behind that rule.  I thought it was just for readability,
> to make it clear that the variable is a pointer without having to refer to the
> declaration.  Perhaps some older timers could shed some light on that :).

I don't know the original rationale, but I agree that nowadays the
justification can only be in terms of readability.  The same reason we
do
 if (integer_that_is_not_a_boolean != 0)
instead of
 if (integer_that_is_not_a_boolean)
.

Thanks,
Pedro Alves

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

[Andreas Arnez]

On Mon, Mar 13 2017, Pedro Alves wrote:

> On 03/10/2017 03:48 PM, Simon Marchi wrote:
>
>> I don't the idea behind that rule.  I thought it was just for readability,
>> to make it clear that the variable is a pointer without having to refer to the
>> declaration.  Perhaps some older timers could shed some light on that :).
>
> I don't know the original rationale, but I agree that nowadays the
> justification can only be in terms of readability.  The same reason we
> do
>  if (integer_that_is_not_a_boolean != 0)
> instead of
>  if (integer_that_is_not_a_boolean)
> .

Right, it's a GDB-specific style rule.  So who fixes the explanation on
the Wiki page?  :-)

--
Andreas

Re: [PATCH 2/3] Test case for dump/restore of large array

[Pedro Alves]

On 03/06/2017 04:00 PM, Andreas Arnez wrote:
> This adds a test case to dump.exp that dumps and restores a large array.
> In particular it verifies that the dump and restore operations are
> completed in reasonable time.

It sounds like this will make the testcase unusable with many
small embedded targets, while it would be before, because the
target simply may not have enough memory for such a big array.
Does that sound right?  If so, then this calls for something like
either splitting this particular test to a separate testcase, or
do condition compilation attempting with and without the big buffer.

> 
> gdb/testsuite/ChangeLog:
> 
> 	PR gdb/21220
> 	* gdb.base/dump.c (bigarray): New variable.
> 	(zero_all): Clear bigarray as well.
> 	(func, fill_bigarray, cmp_bigarray): New functions.
> 	(main): Call fill_bigarray.
> 	* gdb.base/dump.exp: Add test for dump/restore of a large array.
> ---
>  gdb/testsuite/gdb.base/dump.c   | 37 +++++++++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/dump.exp |  8 ++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.base/dump.c b/gdb/testsuite/gdb.base/dump.c
> index bdcafbf..fdeefa8 100644
> --- a/gdb/testsuite/gdb.base/dump.c
> +++ b/gdb/testsuite/gdb.base/dump.c
> @@ -3,6 +3,9 @@
>  #define ARRSIZE 32
>  int intarray[ARRSIZE], intarray2[ARRSIZE];
>  
> +#define BIGSIZE 16777216
> +unsigned char bigarray[BIGSIZE];
> +
>  struct teststruct {
>    int a;
>    int b;
> @@ -25,6 +28,38 @@ zero_all ()
>    memset ((char *) &intarray2,  0, sizeof (intarray2));
>    memset ((char *) &intstruct,  0, sizeof (intstruct));
>    memset ((char *) &intstruct2, 0, sizeof (intstruct2));
> +  memset ((char *) &bigarray,	0, sizeof (bigarray));
> +}
> +
> +static unsigned char
> +func (unsigned u)
> +{
> +  unsigned char a = u;
> +  unsigned char b = (u >> 8);
> +  unsigned char c = (u >> 16);
> +  unsigned char d = (u >> 24);
> +
> +  return (a ^ b ^ c ^ d);
> +}
> +
> +int
> +fill_bigarray ()
> +{
> +  unsigned i;
> +
> +  for (i = 0; i < sizeof (bigarray); i++)
> +    bigarray[i] = func (i);
> +}
> +
> +int
> +cmp_bigarray ()
> +{
> +  unsigned i;
> +
> +  for (i = 0; i < sizeof (bigarray); i++)
> +    if (bigarray[i] != func (i))
> +      return 0;
> +  return 1;
>  }
>  
>  int
> @@ -43,6 +78,8 @@ main()
>    intstruct.f = 12 * 6;
>    intstruct.g = 12 * 7;
>  
> +  fill_bigarray ();
> +
>    checkpoint1 ();
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.base/dump.exp b/gdb/testsuite/gdb.base/dump.exp
> index e67e1aa..170a09f 100644
> --- a/gdb/testsuite/gdb.base/dump.exp
> +++ b/gdb/testsuite/gdb.base/dump.exp
> @@ -74,6 +74,7 @@ set all_files {
>      intstr2.bin intstr2b.bin intstr2.ihex
>      intstr2.srec intstr2.tekhex intstr2.verilog
>      intarr3.srec
> +    bigarr1.bin
>  }
>  
>  # This loop sets variables dynamically -- each name listed in
> @@ -263,6 +264,9 @@ make_dump_file \
>      "dump srec mem [set intarr3.srec] &intarray \(char *\) &intarray + sizeof intarray" \
>  	"dump array as mem, srec, expressions"
>  
> +make_dump_file "dump bin mem [set bigarr1.bin] &bigarray &bigarray\[sizeof bigarray\]" \
> +	"dump big array as memory, default"
> +
>  proc test_restore_saved_value { restore_args msg oldval newval } {
>      global gdb_prompt
>      
> @@ -348,6 +352,10 @@ test_restore_saved_value "[set intstr2.bin] binary $struct_start" \
>  	"struct as memory, binary" \
>  	$struct_val "intstruct"
>  
> +test_restore_saved_value "[set bigarr1.bin] binary &bigarray" \
> +        "reload big array as memory, binary" \
> +	1 "cmp_bigarray ()"
> +
>  # test restore with offset.
>  
>  set array2_start   [capture_value "/x &intarray2\[0\]"]

Thanks,
Pedro Alves

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

[Pedro Alves]

On 03/13/2017 07:50 PM, Andreas Arnez wrote:
> On Mon, Mar 13 2017, Pedro Alves wrote:
> 
>> On 03/10/2017 03:48 PM, Simon Marchi wrote:
>>
>>> I don't the idea behind that rule.  I thought it was just for readability,
>>> to make it clear that the variable is a pointer without having to refer to the
>>> declaration.  Perhaps some older timers could shed some light on that :).
>>
>> I don't know the original rationale, but I agree that nowadays the
>> justification can only be in terms of readability.  The same reason we
>> do
>>  if (integer_that_is_not_a_boolean != 0)
>> instead of
>>  if (integer_that_is_not_a_boolean)
>> .
> 
> Right, it's a GDB-specific style rule.  So who fixes the explanation on
> the Wiki page?  :-)

It's a wiki, so anyone can do that.  Want to volunteer?  :-)

Thanks,
Pedro Alves

Re: [PATCH 3/3] linux-nat: Exploit /proc/<pid>/mem for writing

[Pedro Alves]

On 03/06/2017 04:00 PM, Andreas Arnez wrote:
> So far linux_proc_xfer_partial refused to handle write requests.  This is
> still based on the assumption that the Linux kernel does not support
> writes to /proc/<pid>/mem.  That used to be true, but has changed with
> Linux 2.6.39 released in May 2011.

Hey, I had not noticed that.  Awesome.

(There's also process_vm_readv / process_vm_writev.)

> This patch lifts this restriction and now exploits /proc/<pid>/mem for
> writing to inferior memory as well, if possible.
> 
> gdb/ChangeLog:
> 
> 	* linux-nat.c (linux_proc_xfer_partial): Handle write operations
> 	as well.
> ---
>  gdb/linux-nat.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index c58ed83..73ef2d4 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -3978,10 +3978,9 @@ linux_child_pid_to_exec_file (struct target_ops *self, int pid)
>    return linux_proc_pid_to_exec_file (pid);
>  }
>  
> -/* Implement the to_xfer_partial interface for memory reads using the /proc
> -   filesystem.  Because we can use a single read() call for /proc, this
> -   can be much more efficient than banging away at PTRACE_PEEKTEXT,
> -   but it doesn't support writes.  */
> +/* Implement the to_xfer_partial target method using /proc/<pid>/mem.
> +   Because we can use a single read/write call, this can be much more
> +   efficient than banging away at PTRACE_PEEKTEXT.  */
>  
>  static enum target_xfer_status
>  linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
> @@ -3993,7 +3992,7 @@ linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
>    int fd;
>    char filename[64];
>  
> -  if (object != TARGET_OBJECT_MEMORY || !readbuf)
> +  if (object != TARGET_OBJECT_MEMORY)
>      return TARGET_XFER_EOF;
>  
>    /* Don't bother for one word.  */
> @@ -4004,26 +4003,27 @@ linux_proc_xfer_partial (struct target_ops *ops, enum target_object object,
>       thread.  That requires some juggling, but is even faster.  */
>    xsnprintf (filename, sizeof filename, "/proc/%d/mem",
>  	     ptid_get_pid (inferior_ptid));
> -  fd = gdb_open_cloexec (filename, O_RDONLY | O_LARGEFILE, 0);
> +  fd = gdb_open_cloexec (filename, ((readbuf ? O_RDONLY : O_WRONLY)
> +				    | O_LARGEFILE), 0);
>    if (fd == -1)
>      return TARGET_XFER_EOF;
>  
> -  /* If pread64 is available, use it.  It's faster if the kernel
> -     supports it (only one syscall), and it's 64-bit safe even on
> -     32-bit platforms (for instance, SPARC debugging a SPARC64
> -     application).  */
> +  /* Use pread64/pwrite64 if available, since they save a syscall and can
> +     handle 64-bit offsets even on 32-bit platforms (for instance, SPARC
> +     debugging a SPARC64 application).  */
>  #ifdef HAVE_PREAD64
> -  if (pread64 (fd, readbuf, len, offset) != len)
> +  ret = (readbuf ? pread64 (fd, readbuf, len, offset)
> +	 : pwrite64 (fd, writebuf, len, offset));
>  #else
> -  if (lseek (fd, offset, SEEK_SET) == -1 || read (fd, readbuf, len) != len)
> +  ret = lseek (fd, offset, SEEK_SET);
> +  if (ret != -1)
> +    ret = (readbuf ? read (fd, readbuf, len)
> +	   : write (fd, writebuf, len));
>  #endif
> -    ret = 0;
> -  else
> -    ret = len;
>  
>    close (fd);
>  
> -  if (ret == 0)
> +  if (ret == -1 || ret == 0)
>      return TARGET_XFER_EOF;

Are we sure we can't see partial reads/writes here?
I.e., seems like we lose the "read/pread64 (fd, readbuf, len) != len" 
checks?

Thanks,
Pedro Alves

Re: [PATCH 3/3] linux-nat: Exploit /proc/<pid>/mem for writing

[Pedro Alves]

On 03/13/2017 08:05 PM, Pedro Alves wrote:

> Are we sure we can't see partial reads/writes here?
> I.e., seems like we lose the "read/pread64 (fd, readbuf, len) != len" 
> checks?

Gah, nevermind.  I had it the other way around.
What you have handles partial reads/writes better than
what was there.

So patch is OK.

Thanks,
Pedro Alves

Re: [PATCH 3/3] linux-nat: Exploit /proc/<pid>/mem for writing

[Andreas Arnez]

On Mon, Mar 13 2017, Pedro Alves wrote:

> On 03/06/2017 04:00 PM, Andreas Arnez wrote:
>> So far linux_proc_xfer_partial refused to handle write requests.  This is
>> still based on the assumption that the Linux kernel does not support
>> writes to /proc/<pid>/mem.  That used to be true, but has changed with
>> Linux 2.6.39 released in May 2011.
>
> Hey, I had not noticed that.  Awesome.
>
> (There's also process_vm_readv / process_vm_writev.)

Right.  This reminds me that I've started a patch for exploiting
process_vm_readv/writev two years ago, but then abandoned it.  There was
some problem with it, but I don't recall the details.  Maybe I can dig
it out and try again...

--
Andreas

Re: [PATCH 3/3] linux-nat: Exploit /proc/<pid>/mem for writing

[Andreas Arnez]

On Mon, Mar 13 2017, Pedro Alves wrote:

> On 03/13/2017 08:05 PM, Pedro Alves wrote:
>
>> Are we sure we can't see partial reads/writes here?
>> I.e., seems like we lose the "read/pread64 (fd, readbuf, len) != len" 
>> checks?
>
> Gah, nevermind.  I had it the other way around.
> What you have handles partial reads/writes better than
> what was there.
>
> So patch is OK.

Thanks, pushed.

--
Andreas

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

[Andreas Arnez]

On Mon, Mar 13 2017, Pedro Alves wrote:

> On 03/13/2017 07:50 PM, Andreas Arnez wrote:
>> On Mon, Mar 13 2017, Pedro Alves wrote:
>> 
>>> On 03/10/2017 03:48 PM, Simon Marchi wrote:
>>>
>>>> I don't the idea behind that rule.  I thought it was just for readability,
>>>> to make it clear that the variable is a pointer without having to refer to the
>>>> declaration.  Perhaps some older timers could shed some light on that :).
>>>
>>> I don't know the original rationale, but I agree that nowadays the
>>> justification can only be in terms of readability.  The same reason we
>>> do
>>>  if (integer_that_is_not_a_boolean != 0)
>>> instead of
>>>  if (integer_that_is_not_a_boolean)
>>> .
>> 
>> Right, it's a GDB-specific style rule.  So who fixes the explanation on
>> the Wiki page?  :-)
>
> It's a wiki, so anyone can do that.  Want to volunteer?  :-)

OK, I just changed it and added your example above:

  https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_NULL_And_Zero

Comments welcome!

--
Andreas

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

[Pedro Alves]

On 03/14/2017 03:12 PM, Andreas Arnez wrote:
> On Mon, Mar 13 2017, Pedro Alves wrote:

>> It's a wiki, so anyone can do that.  Want to volunteer?  :-)
> 
> OK, I just changed it and added your example above:
> 
>   https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_NULL_And_Zero
> 
> Comments welcome!

Thanks!