Re: [PATCH] Support large registers in regcache transfer_regset

[Simon Marchi <simark at simark dot ca>]

On 2018-06-12 04:03 AM, Alan Hayward wrote:
> Regcache transfer_regset is used to copy registers to/from a regset.
> If the size of a regcache register is greater than it's slot in the regset
> then the writes will overflow into the next slot(s), causing general
> overflow corruption with the latter slots.

Hi Alan,

Can you please remind me when this happens?  When reading cores, we don't
write to regsets, if I understand correctly, so it would not be a problem
tben.

> Add raw_collect_part and raw_supply_part, which are simplified versions of
> raw_read_part and raw_write_part. Use these in accordingly in transfer_regset.
> 
> This is required to support the .reg2 core section on Aarch64 SVE, which
> only has enough space to store the first 128bits of each vector register.
> 
> This patch replaces [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores
> 
> Tested by generating cores on aarch64 and aarch64 sve, both via gdb and the
> kernel, then ensuring the cores load back on the both systems.
> Checked cores on x86 still look ok.
> Ran make check on x86 and aarch64.
> 
> 2018-06-12  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* regcache.c (reg_buffer::raw_collect_part): New function.
> 	(reg_buffer::raw_supply_part): Likewise.
> 	(regcache::transfer_regset): Call new functions.
> 	* regcache.h (reg_buffer::raw_collect_part): New declaration.
> 	(reg_buffer::raw_supply_part): Likewise.
> ---
>  gdb/regcache.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  gdb/regcache.h |  8 +++++
>  2 files changed, 94 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 750ea2ad30..babc0e1d43 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -812,6 +812,27 @@ readable_regcache::read_part (int regnum, int offset, int len, void *in,
>    return REG_VALID;
>  }
>  
> +/* See regcache.h.  */
> +
> +void
> +reg_buffer::raw_collect_part (int regnum, int offset, int len, void *in) const

I know this comes from raw_read_part/raw_write_part, but I find the naming of the
in/out parameters confusing.  I think it would make more sense if this one was
called "out" and the one in raw_supply_part "in".

Also, can they be "gdb_byte *" instead of "void *"?

> +{
> +  struct gdbarch *gdbarch = arch ();
> +  gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, 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]);

The "&& offset <= m_descr->sizeof_register[regnum]" is redundant, given the
following line.  Other than mimicking raw_read_part, is there a reason why
these are signed integers?  Having them unsigned would avoid having to assert
they are >= 0.

> +  /* Something to do?  */
> +  if (offset + len == 0)
> +    return;
> +  /* Read (when needed) ...  */
> +  raw_collect (regnum, reg);
> +
> +  /* ... modify ...  */
> +  memcpy (in, reg + offset, len);
> +}
> +
>  enum register_status
>  regcache::write_part (int regnum, int offset, int len,
>  		     const void *out, bool is_raw)
> @@ -849,6 +870,33 @@ regcache::write_part (int regnum, int offset, int len,
>    return REG_VALID;
>  }
>  
> +/* See regcache.h.  */
> +
> +void
> +reg_buffer::raw_supply_part (int regnum, int offset, int len,
> +			     const gdb_byte *out)
> +{
> +  struct gdbarch *gdbarch = arch ();
> +  gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
> +
> +  gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
> +  gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);

"&& offset <= m_descr->sizeof_register[regnum]" is redundant here too.

> +  /* Something to do?  */
> +  if (offset + len == 0)
> +    return;
> +  /* Read (when needed) ...  */
> +  if (offset > 0
> +      || offset + len < m_descr->sizeof_register[regnum])
> +    raw_collect (regnum, reg);
> +
> +  if (out == nullptr)
> +    memset (reg + offset, 0, len);
> +  else
> +    memcpy (reg + offset, out, len);
> +  /* ... write (when needed).  */
> +  raw_supply (regnum, reg);
> +}
> +
>  enum register_status
>  readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf)
>  {
> @@ -1016,12 +1064,26 @@ regcache::transfer_regset (const struct regset *regset,
>  	    if (offs + slot_size > size)
>  	      break;
>  
> -	    if (out_buf)
> -	      raw_collect (regno, (gdb_byte *) out_buf + offs);
> +	    gdb_byte *out_loc = (gdb_byte *) out_buf + offs;
> +	    const gdb_byte *in_loc = in_buf ? (const gdb_byte *) in_buf + offs
> +					    : NULL;
> +
> +	    if (slot_size < m_descr->sizeof_register[regno])
> +	      {
> +		/* Register is bigger than the size of the slot.  Prevent
> +		   possible overflow.  */
> +		if (out_buf)
> +		  raw_collect_part (regno, 0, slot_size, out_loc);
> +		else
> +		  out_regcache->raw_supply_part (regno, 0, slot_size, in_loc);
> +	      }
>  	    else
> -	      out_regcache->raw_supply (regno, in_buf
> -					? (const gdb_byte *) in_buf + offs
> -					: NULL);
> +	      {
> +		if (out_buf)
> +		  raw_collect (regno, out_loc);
> +		else
> +		  out_regcache->raw_supply (regno, in_loc);
> +	      }

I think the code here could stay relatively simple if we always went
through raw_collect_part/raw_supply_part.  To avoid the unnecessary copy
that would happen in raw_collect_part/raw_supply_part in the typical case
where the full register is transferred, there could be a shortcut path
that calls raw_collect/raw_supply when collecting/supplying the full
reguster.

Simon