diff mbox

Support large registers in regcache transfer_regset

Message ID 85CCB6E0-63F0-4298-B328-D43D3207A91E@arm.com
State New
Headers show

Commit Message

Alan Hayward June 19, 2018, 11:27 a.m. UTC
> 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.

Comments

Simon Marchi June 19, 2018, 2:52 p.m. UTC | #1
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
Alan Hayward June 19, 2018, 3:46 p.m. UTC | #2
> 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.
Simon Marchi June 19, 2018, 9:11 p.m. UTC | #3
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
diff mbox

Patch

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