[PATCH] Support large registers in regcache transfer_regset

[PATCH] Support large registers in regcache transfer_regset

[Alan Hayward]

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.

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
+{
+  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]);
+  /* 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]);
+  /* 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);
+	      }
 	  }
       else
 	{
@@ -1030,12 +1092,26 @@ regcache::transfer_regset (const struct regset *regset,
 	  if (offs + slot_size > size)
 	    return;
 
-	  if (out_buf)
-	    raw_collect (regnum, (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 (regnum, 0, slot_size, out_loc);
+	      else
+		out_regcache->raw_supply_part (regnum, 0, slot_size, in_loc);
+	    }
 	  else
-	    out_regcache->raw_supply (regnum, in_buf
-				      ? (const gdb_byte *) in_buf + offs
-				      : NULL);
+	    {
+	      if (out_buf)
+		raw_collect (regnum, out_loc);
+	      else
+		out_regcache->raw_supply (regnum, in_loc);
+	    }
 	  return;
 	}
     }
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 41465fb20d..3e2c5a9067 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -163,6 +163,10 @@ public:
   void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
 			    bool is_signed) const;
 
+  /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
+     reading only LEN.  */
+  void raw_collect_part (int regnum, int offset, int len, void *in) const;
+
   /* See common/common-regcache.h.  */
   void raw_supply (int regnum, const void *buf) override;
 
@@ -184,6 +188,10 @@ public:
      unavailable).  */
   void raw_supply_zeroed (int regnum);
 
+  /* Supply register REGNUM to REGCACHE, starting at offset in REGCACHE, writing
+     only LEN.  */
+  void raw_supply_part (int regnum, int offset, int len, const gdb_byte *out);
+
   void invalidate (int regnum);
 
   virtual ~reg_buffer () = default;
-- 
2.15.1 (Apple Git-101)

Re: [PATCH] Support large registers in regcache transfer_regset

[Simon Marchi]

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

Re: [PATCH] Support large registers in regcache transfer_regset

[Alan Hayward]

> On 17 Jun 2018, at 03:52, Simon Marchi <simark@simark.ca> wrote:
> 
> 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.

Type “generate-core-file” on the command line.
Not sure if there are other ways of triggering it.

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

Agreed. I’ll also update the original code too.

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

Done.

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

Looking at regcache, int is used for regnum throughout. I’d rather not have a
mismatch, and wouldn’t want to update everything else either (at least not
in this patch). In addition, if this code is going to now call down to
raw_collect/raw_supply, they should match.

I’ve updated the asserts in both here and the original code too.

> 
>> +  /* 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.

Done.

> 
>> +  /* 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.
> 

Ok, added this too.

However, this uncovered an issue. If the slot size is larger than the size of
the register then this causes raw_collect_part/raw_supply_part to assert.
This is triggered on aarch64 with CPSR, because the register is 32bits but
the core file reserves 64bits.
In the current code there is a bug here as the existing code doesn’t ensure
this extra space in the slot size is cleared when writing out cores.
(In reality, probably not much of a problem as we ignore that space when
reading the core back in. But it could be an issue if another program was
Reading gdb’s core files).

To fix this I allowed raw_collect_part/raw_supply_part to handle cases where
the length overflows the register, either filling with zeros or truncating
accordingly.
Also, because the existing transfer_register code would result in register
being set to invalid when writing null to the regcache, I also ensured
that was kept.


I’ve also made a few more changes to write_part/read_part to make them
consistent with the changes to the new functions. There is no functionality
change to them.


Is this ok?


2018-06-19  Alan Hayward  <alan.hayward@arm.com>

	* regcache.c (readable_regcache::read_part): Add full read
	shortcut.
	(reg_buffer::raw_collect_part): New function.
	(regcache::write_part): Add full write shortcut.
	(reg_buffer::raw_supply_part): New function.
	(regcache::transfer_regset): Use new functions.
	* regcache.h (reg_buffer::raw_collect_part): New declaration.
	(reg_buffer::raw_supply_part): Likewise.


diff --git a/gdb/regcache.h b/gdb/regcache.h
index 41465fb20d0dcdddaf39e47c1fc79f1e8eadc260..297f04b5b44690562602a2b5ce15b9dd1a92b18b 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -163,6 +163,11 @@ public:
   void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
 			    bool is_signed) const;

+  /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
+     reading only LEN.  If this runs off the end of the register, then fill the
+     additional space with zeros.  */
+  void raw_collect_part (int regnum, int offset, int len, gdb_byte *out) const;
+
   /* See common/common-regcache.h.  */
   void raw_supply (int regnum, const void *buf) override;

@@ -184,6 +189,11 @@ public:
      unavailable).  */
   void raw_supply_zeroed (int regnum);

+  /* Supply register REGNUM to REGCACHE, starting at offset in REGCACHE, writing
+     only LEN, without editing the rest of the register.  If the length of the
+     supplied value would overflow the register, then truncate.  */
+  void raw_supply_part (int regnum, int offset, int len, const gdb_byte *in);
+
   void invalidate (int regnum);

   virtual ~reg_buffer () = default;
@@ -254,8 +264,11 @@ public:
   struct value *cooked_read_value (int regnum);

 protected:
-  enum register_status read_part (int regnum, int offset, int len, void *in,
-				  bool is_raw);
+
+  /* Perform a partial register transfer using a read, modify, write
+     operation.  */
+  enum register_status read_part (int regnum, int offset, int len,
+				  gdb_byte *out, bool is_raw);
 };

 /* Buffer of registers, can be read and written.  */
@@ -356,8 +369,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);


   /* The address space of this register cache (for registers where it
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 750ea2ad30f60b03dd76fc30cb72f87d5a531406..5c672e6928101710904c2e70fc829036214f64b6 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -780,75 +780,138 @@ 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,
+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);
+
+  /* Read to buffer, then write out.  */
   enum register_status status;
+  gdb_byte *reg = (gdb_byte *) alloca (reg_size);

-  if (is_raw)
-    status = raw_read (regnum, reg);
-  else
-    status = cooked_read (regnum, reg);
+  status = (is_raw) ? raw_read (regnum, reg) : cooked_read (regnum, reg);
   if (status != REG_VALID)
     return status;

-  /* ... modify ...  */
-  memcpy (in, reg + offset, len);
-
+  memcpy (out, reg + offset, len);
   return REG_VALID;
 }

+/* See regcache.h.  */
+
+void
+reg_buffer::raw_collect_part (int regnum, int offset, int len,
+			      gdb_byte *out) const
+{
+  int reg_size = register_size (arch (), regnum);
+
+  gdb_assert (out != nullptr);
+  gdb_assert (offset >= 0 && len >= 0 && offset <= reg_size);
+
+  if (offset == 0 && len == 0)
+    return;
+
+  if (offset == 0 && len == reg_size)
+    return raw_collect (regnum, out);
+
+  /* Read to buffer, then write out.  */
+  gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+  raw_collect (regnum, reg);
+
+  if (offset + len <= reg_size)
+    memcpy (out, reg + offset, len);
+  else
+    {
+      /* Requested region runs off the end of the register.  Clear the
+	 additional space.  */
+      memcpy (out, reg + offset, reg_size - offset);
+      memset (out + reg_size, 0, offset + len - reg_size);
+    }
+}
+
+/* See regcache.h.  */
+
 enum register_status
-regcache::write_part (int regnum, int offset, int len,
-		     const void *out, bool is_raw)
+regcache::write_part (int regnum, int offset, int len, const gdb_byte *in,
+		      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 (out != 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 (in != 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
-      || offset + len < m_descr->sizeof_register[regnum])
+
+  if (offset == 0 && len == reg_size)
     {
-      enum register_status status;
+      (is_raw) ? raw_write (regnum, in) : cooked_write (regnum, in);
+      return REG_VALID;
+    }

-      if (is_raw)
-	status = raw_read (regnum, reg);
-      else
-	status = cooked_read (regnum, reg);
+  gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+
+  /* Read when needed.  */
+  if (offset > 0 || offset + len < reg_size)
+    {
+      enum register_status status;
+      status = (is_raw) ? raw_read (regnum, reg) : cooked_read (regnum, reg);
       if (status != REG_VALID)
 	return status;
     }

-  memcpy (reg + offset, out, len);
-  /* ... write (when needed).  */
-  if (is_raw)
-    raw_write (regnum, reg);
-  else
-    cooked_write (regnum, reg);
-
+  /* Write to buffer, then write out.  */
+  memcpy (reg + offset, in, len);
+  is_raw ? raw_write (regnum, reg) : cooked_write (regnum, reg);
   return REG_VALID;
 }

+/* See regcache.h.  */
+
+void
+reg_buffer::raw_supply_part (int regnum, int offset, int len,
+			     const gdb_byte *in)
+{
+  int reg_size = register_size (arch (), regnum);
+
+  gdb_assert (in != nullptr);
+  gdb_assert (offset >= 0 && len >= 0 && offset <= reg_size);
+
+  if (offset == 0 && len == 0)
+    return;
+
+  if (offset + len > reg_size)
+    {
+      /* Truncate length to fit the size of the regcache register.  */
+      len = reg_size - offset;
+    }
+
+  if (offset == 0 && len == reg_size)
+    return raw_supply (regnum, in);
+
+  gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+
+  /* Read when needed.  */
+  if (offset > 0 || offset + len < reg_size)
+    raw_collect (regnum, reg);
+
+  /* Write to buffer, then write out.  */
+  memcpy (reg + offset, in, len);
+  raw_supply (regnum, reg);
+}
+
 enum register_status
 readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf)
 {
@@ -994,6 +1057,7 @@ regcache::transfer_regset (const struct regset *regset,
 {
   const struct regcache_map_entry *map;
   int offs = 0, count;
+  struct gdbarch *gdbarch = arch ();

   for (map = (const struct regcache_map_entry *) regset->regmap;
        (count = map->count) != 0;
@@ -1016,12 +1080,18 @@ regcache::transfer_regset (const struct regset *regset,
 	    if (offs + slot_size > size)
 	      break;

+	    /* Use part versions to prevent possible overflow.  */
 	    if (out_buf)
-	      raw_collect (regno, (gdb_byte *) out_buf + offs);
+	      raw_collect_part (regno, 0, slot_size,
+				(gdb_byte *) out_buf + offs);
+	    else if (in_buf)
+	      out_regcache->raw_supply_part (regno, 0, slot_size,
+					     (const gdb_byte *) in_buf + offs);
 	    else
-	      out_regcache->raw_supply (regno, in_buf
-					? (const gdb_byte *) in_buf + offs
-					: NULL);
+	      {
+	      	/* Invalidate the register.  */
+		out_regcache->raw_supply (regno, nullptr);
+	      }
 	  }
       else
 	{
@@ -1030,12 +1100,19 @@ regcache::transfer_regset (const struct regset *regset,
 	  if (offs + slot_size > size)
 	    return;

+	  /* Use part versions to prevent possible overflow.  */
 	  if (out_buf)
-	    raw_collect (regnum, (gdb_byte *) out_buf + offs);
+	    raw_collect_part (regnum, 0, slot_size,
+			      (gdb_byte *) out_buf + offs);
+	  else if (in_buf)
+	    out_regcache->raw_supply_part (regnum, 0, slot_size,
+					   (const gdb_byte *) in_buf + offs);
 	  else
-	    out_regcache->raw_supply (regnum, in_buf
-				      ? (const gdb_byte *) in_buf + offs
-				      : NULL);
+	    {
+	      /* Invalidate the register.  */
+	      out_regcache->raw_supply (regnum, nullptr);
+	    }
+
 	  return;
 	}
     }

Re: [PATCH] Support large registers in regcache transfer_regset

[Simon Marchi]

On 2018-06-19 07:27, Alan Hayward wrote:
>>> +/* See regcache.h.  */
>>> +
>>> +void
>>> +reg_buffer::raw_collect_part (int regnum, int offset, int len, void 
>>> *in) const
>>> +{
>>> +  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.
> 
> Looking at regcache, int is used for regnum throughout. I’d rather not 
> have a
> mismatch, and wouldn’t want to update everything else either (at least 
> not
> in this patch). In addition, if this code is going to now call down to
> raw_collect/raw_supply, they should match.

Sorry, I was talking about len and offset, not regnum.

Simon

Re: [PATCH] Support large registers in regcache transfer_regset

[Alan Hayward]

> On 19 Jun 2018, at 15:52, Simon Marchi <simark@simark.ca> wrote:
> 
> On 2018-06-19 07:27, Alan Hayward wrote:
>>>> +/* See regcache.h.  */
>>>> +
>>>> +void
>>>> +reg_buffer::raw_collect_part (int regnum, int offset, int len, void *in) const
>>>> +{
>>>> +  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.
>> Looking at regcache, int is used for regnum throughout. I’d rather not have a
>> mismatch, and wouldn’t want to update everything else either (at least not
>> in this patch). In addition, if this code is going to now call down to
>> raw_collect/raw_supply, they should match.
> 
> Sorry, I was talking about len and offset, not regnum.
> 
> Simon

Ah, ok. If doing that, then it’d make sense to update regcache_map_entry to use
unsigned ints for count and size.

struct regcache_map_entry
{
  int count;
  int regno;
  int size;
};

At that point it probably makes sense to repost the patch as v2 in smaller pieces?


Alan.

Re: [PATCH] Support large registers in regcache transfer_regset

[Simon Marchi]

On 2018-06-19 11:46, Alan Hayward wrote:
> Ah, ok. If doing that, then it’d make sense to update 
> regcache_map_entry to use
> unsigned ints for count and size.
> 
> struct regcache_map_entry
> {
>   int count;
>   int regno;
>   int size;
> };
> 
> At that point it probably makes sense to repost the patch as v2 in
> smaller pieces?

If you end up doing this (I did not really dig much to see if it works), 
then yes it might make sense to have a preparatory patch that changes 
some types to be unsigned.  As always, one logical change per patch is 
appreciated and is much easier to review.

If you're going to send a proper v2, I'll wait for this one to look at 
the new changes you sent as part of the "updated" v1.

Simon