Re: [PATCH] Removal of uses of MAX_REGISTER_SIZE

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

> On 2 Feb 2017, at 09:40, Joel Brobecker <brobecker@adacore.com> wrote:
> 
>> #2 - Switch to heap allocation
>> 
>> Or std::vector or something like that that hides it.  It's not clear
>> whether that would have a noticeable performance impact.
> 
> I would try that. I think using one of the standard C++ classes
> looks a little more attractive to me, and would only consider
> the lambda functions if we can show a noticeable performance
> impact. Those two are not exclusive, by the way, but in the past,
> we've always frowned on calls to alloca in a loop, and using
> a xmalloc+cleanup combination has never been an issue in my cases.
> I'd imagine that a standard C++ memory management class would be
> fast enough for those same situations.
> 
> -- 
> Joel

We're not allocating much space, so I'd hope std::vector didn't cause much
of a slowdown. Also, the allocas with lamdba's approach feels a little
overcomplicated.

Reworking my patch that adds max_register_size (), I've replaced the allocas
with std::vector.

This patch ok? If people are happy, I'll then rework the larger patch.

Please let me know if there's a particular test you want me to run to
test performance (although I suspect it would need the larger patch
to get meaningful results). 

Thanks,
Alan.

2017-02-03  Alan Hayward  <alan.hayward@arm.com>

	* regcache.c (struct regcache_descr): Add max_register_size.
	(max_register_size): New.
	(init_regcache_descr): Find max register size.
	(regcache_save): Use std::vector.
	(regcache_restore): Likewise.
	(regcache_dump): Likewise.
	* regcache.h (max_register_size): New.
	* remote.c (remote_prepare_to_store): Use std::vector.
	* frame.c (frame_unwind_register_signed): Likewise.
	(frame_unwind_register_unsigned): Likewise.
	(get_frame_register_bytes): Likewise.
	(put_frame_register_bytes): Likewise.


diff --git a/gdb/frame.c b/gdb/frame.c
index d98003dee7c34a63bd25356e6674721664a4b2f3..135ca2b753cf4d64d0322331199d008548839c8d 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1252,10 +1252,10 @@ frame_unwind_register_signed (struct frame_info *frame, int regnum)
   struct gdbarch *gdbarch = frame_unwind_arch (frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int size = register_size (gdbarch, regnum);
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (size);

-  frame_unwind_register (frame, regnum, buf);
-  return extract_signed_integer (buf, size, byte_order);
+  frame_unwind_register (frame, regnum, buf.data ());
+  return extract_signed_integer (buf.data (), size, byte_order);
 }

 LONGEST
@@ -1270,10 +1270,10 @@ frame_unwind_register_unsigned (struct frame_info *frame, int regnum)
   struct gdbarch *gdbarch = frame_unwind_arch (frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int size = register_size (gdbarch, regnum);
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (size);

-  frame_unwind_register (frame, regnum, buf);
-  return extract_unsigned_integer (buf, size, byte_order);
+  frame_unwind_register (frame, regnum, buf.data ());
+  return extract_unsigned_integer (buf.data (), size, byte_order);
 }

 ULONGEST
@@ -1410,16 +1410,16 @@ get_frame_register_bytes (struct frame_info *frame, int regnum,
 	}
       else
 	{
-	  gdb_byte buf[MAX_REGISTER_SIZE];
+	  std::vector<gdb_byte> buf (max_register_size (gdbarch));
 	  enum lval_type lval;
 	  CORE_ADDR addr;
 	  int realnum;

 	  frame_register (frame, regnum, optimizedp, unavailablep,
-			  &lval, &addr, &realnum, buf);
+			  &lval, &addr, &realnum, buf.data ());
 	  if (*optimizedp || *unavailablep)
 	    return 0;
-	  memcpy (myaddr, buf + offset, curr_len);
+	  memcpy (myaddr, buf.data () + offset, curr_len);
 	}

       myaddr += curr_len;
@@ -1460,11 +1460,11 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,
 	}
       else
 	{
-	  gdb_byte buf[MAX_REGISTER_SIZE];
+	  std::vector<gdb_byte> buf (max_register_size (gdbarch));

-	  deprecated_frame_register_read (frame, regnum, buf);
-	  memcpy (buf + offset, myaddr, curr_len);
-	  put_frame_register (frame, regnum, buf);
+	  deprecated_frame_register_read (frame, regnum, buf.data ());
+	  memcpy (buf.data () + offset, myaddr, curr_len);
+	  put_frame_register (frame, regnum, buf.data ());
 	}

       myaddr += curr_len;
diff --git a/gdb/regcache.h b/gdb/regcache.h
index e5a7cf553279b8cc0d546ec1b8274cbf97e246d5..5bc99f5c1ef87318edf4e934ec60c7f1225e7561 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -202,6 +202,8 @@ extern struct type *register_type (struct gdbarch *gdbarch, int regnum);

 extern int register_size (struct gdbarch *gdbarch, int regnum);

+/* Return the size of the largest register.  */
+extern long max_register_size (struct gdbarch *gdbarch);

 /* Save/restore a register cache.  The set of registers saved /
    restored into the DST regcache determined by the save_reggroup /
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 9d28aa2c2114e0f1c52758bb2fbe9669a329c13e..522633ae0fdf6d80508d725bc1d68d05567fd9ff 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -73,6 +73,9 @@ struct regcache_descr

   /* Cached table containing the type of each register.  */
   struct type **register_type;
+
+  /* Size of the largest register.  */
+  long max_register_size;
 };

 static void *
@@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
+	descr->max_register_size = std::max (descr->max_register_size,
+					     descr->sizeof_register[i]);
       }
     /* Set the real size of the raw register cache buffer.  */
     descr->sizeof_raw_registers = offset;
@@ -136,6 +141,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
+	descr->max_register_size = std::max (descr->max_register_size,
+					     descr->sizeof_register[i]);
       }
     /* Set the real size of the readonly register cache buffer.  */
     descr->sizeof_cooked_registers = offset;
@@ -187,6 +194,13 @@ regcache_register_size (const struct regcache *regcache, int n)
   return register_size (get_regcache_arch (regcache), n);
 }

+long
+max_register_size (struct gdbarch *gdbarch)
+{
+  struct regcache_descr *descr = regcache_descr (gdbarch);
+  return descr->max_register_size;
+}
+
 /* The register cache for storing raw register values.  */

 struct regcache
@@ -327,7 +341,7 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
 	       void *src)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (max_register_size (gdbarch));
   int regnum;

   /* The DST should be `read-only', if it wasn't then the save would
@@ -346,10 +360,10 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
     {
       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
 	{
-	  enum register_status status = cooked_read (src, regnum, buf);
+	  enum register_status status = cooked_read (src, regnum, buf.data ());

 	  if (status == REG_VALID)
-	    memcpy (register_buffer (dst, regnum), buf,
+	    memcpy (register_buffer (dst, regnum), buf.data (),
 		    register_size (gdbarch, regnum));
 	  else
 	    {
@@ -369,7 +383,7 @@ regcache_restore (struct regcache *dst,
 		  void *cooked_read_context)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (max_register_size (gdbarch));
   int regnum;

   /* The dst had better not be read-only.  If it is, the `restore'
@@ -385,9 +399,9 @@ regcache_restore (struct regcache *dst,
 	{
 	  enum register_status status;

-	  status = cooked_read (cooked_read_context, regnum, buf);
+	  status = cooked_read (cooked_read_context, regnum, buf.data ());
 	  if (status == REG_VALID)
-	    regcache_cooked_write (dst, regnum, buf);
+	    regcache_cooked_write (dst, regnum, buf.data ());
 	}
     }
 }
@@ -1279,7 +1293,7 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
   int footnote_register_offset = 0;
   int footnote_register_type_name_null = 0;
   long register_offset = 0;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (max_register_size (gdbarch));

 #if 0
   fprintf_unfiltered (file, "nr_raw_registers %d\n",
@@ -1406,8 +1420,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    fprintf_unfiltered (file, "<unavailable>");
 	  else
 	    {
-	      regcache_raw_read (regcache, regnum, buf);
-	      print_hex_chars (file, buf,
+	      regcache_raw_read (regcache, regnum, buf.data ());
+	      print_hex_chars (file, buf.data (),
 			       regcache->descr->sizeof_register[regnum],
 			       gdbarch_byte_order (gdbarch));
 	    }
@@ -1422,13 +1436,13 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    {
 	      enum register_status status;

-	      status = regcache_cooked_read (regcache, regnum, buf);
+	      status = regcache_cooked_read (regcache, regnum, buf.data ());
 	      if (status == REG_UNKNOWN)
 		fprintf_unfiltered (file, "<invalid>");
 	      else if (status == REG_UNAVAILABLE)
 		fprintf_unfiltered (file, "<unavailable>");
 	      else
-		print_hex_chars (file, buf,
+		print_hex_chars (file, buf.data (),
 				 regcache->descr->sizeof_register[regnum],
 				 gdbarch_byte_order (gdbarch));
 	    }
diff --git a/gdb/remote.c b/gdb/remote.c
index c4cec910c44cf91cc7f36b7f2d87cde3f46de41e..157a1b1789d2a248c11dcc4efebd8ce54da82045 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7757,9 +7757,10 @@ remote_fetch_registers (struct target_ops *ops,
 static void
 remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct remote_arch_state *rsa = get_remote_arch_state ();
   int i;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (max_register_size (gdbarch));

   /* Make sure the entire registers array is valid.  */
   switch (packet_support (PACKET_P))
@@ -7769,7 +7770,7 @@ remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)
       /* Make sure all the necessary registers are cached.  */
       for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++)
 	if (rsa->regs[i].in_g_packet)
-	  regcache_raw_read (regcache, rsa->regs[i].regnum, buf);
+	  regcache_raw_read (regcache, rsa->regs[i].regnum, buf.data ());
       break;
     case PACKET_ENABLE:
       break;