Re: [PATCH] Support large registers in regcache transfer_regset

[Alan Hayward <Alan.Hayward@arm.com>]

> 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;
 	}
     }