Re: [PATCH v2 2/3] Avoid memcpys in regcache read_part/write_part for full registers.

[Simon Marchi <simon.marchi@ericsson.com>]

On 2018-06-21 05:38 AM, Alan Hayward wrote:
> Additionally, tidy up the functions: Remove asserts, use gdb_byte,
> update comments.

Thanks, that's a nice cleanup.

Just two nits, the patch LGTM with those fixed.

> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 6276b149a2..fe4742c2ee 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -775,77 +775,75 @@ regcache::cooked_write (int regnum, const gdb_byte *buf)
>  				   regnum, buf);
>  }
>  
> -/* Perform a partial register transfer using a read, modify, write
> -   operation.  */
> +/* See regcache.h.  */
>  
>  enum register_status
> -readable_regcache::read_part (int regnum, int offset, int len, void *in,
> -			      bool is_raw)
> +readable_regcache::read_part (int regnum, int offset, int len,
> +			      gdb_byte *out, bool is_raw)
>  {
> -  struct gdbarch *gdbarch = arch ();
> -  gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
> +  int reg_size = register_size (arch (), regnum);
>  
> -  gdb_assert (in != NULL);
> -  gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
> -  gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
> -  /* Something to do?  */
> -  if (offset + len == 0)
> +  gdb_assert (out != NULL);
> +  gdb_assert (offset >= 0 && len >= 0 && offset + len <= reg_size);
> +
> +  if (offset == 0 && len == 0)
>      return REG_VALID;
> -  /* Read (when needed) ...  */
> +
> +  if (offset == 0 && len == reg_size)
> +    return (is_raw) ? raw_read (regnum, out) : cooked_read (regnum, out);

Could you add some comments to help the reader quickly know what's
going on?  Something like:

  if (offset == 0 && len == 0)
    {
      /* We don't read anything at all.  */
      return REG_VALID;
    }

  if (offset == 0 && len == reg_size)
    {
      /* We read the whole register.  */
      return (is_raw) ? raw_read (regnum, out) : cooked_read (regnum, out);
    }

  /* We read only part of the register.  */

Same for write_part.

> @@ -355,9 +358,10 @@ private:
>  			int regnum, const void *in_buf,
>  			void *out_buf, size_t size) const;
>  
> +  /* Perform a partial register transfer using a read, modify, write
> +     operation.  */
>    enum register_status write_part (int regnum, int offset, int len,
> -				   const void *out, bool is_raw);
> -
> +				   const gdb_byte *out, bool is_raw);

Rename out to in?

Thanks,

Simon