This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]