[1/3] 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): 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(-)
  

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
  

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.
  

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
  

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
  

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
  

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
  

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
  

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!