Re: [PATCH] Removal of uses of MAX_REGISTER_SIZE

[Yao Qi <qiyaoltc at gmail dot com>]

On Fri, Feb 3, 2017 at 9:59 AM, Alan Hayward <Alan.Hayward@arm.com> wrote:
>
>> 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.

I don't think we have to replace all MAX_REGISTER_SIZE with std::vector.
MAX_REGISTER_SIZE is mostly used in arch-dependent code (*-tdep.c
and *-nat.c), where the register size or max register size is known.  For
example, MAX_REGISTER_SIZE is used only once in arm-tdep.c, and
it can be replaced with FP_REGISTER_SIZE, because 'buf' is to get the
contents for FPA register.  Similarly, MAX_REGISTER_SIZE is used three
times in aarch64-tdep.c, all of them can be repalced by V_REGISTER_SIZE.
Also, MAX_REGISTER_SIZE can be replaced by
I386_MAX_REGISTER_SIZE in i386-tdep.c.  I would like to examine the
usages of MAX_REGISTER_SIZE in each target-dependent code, and
replace MAX_REGISTER_SIZE with known constants as much as we can.
I don't think anyone has objections on replacing one constant
MAX_REGISTER_SIZE with other smaller constants :)

Then, let us discuss how to remove MAX_REGISTER_SIZE from
arch-independent code after all above is done.

-- 
Yao (齐尧)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 801c03d..1f82187 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1982,7 +1982,7 @@ aarch64_store_return_value (struct type *type, struct regcache *regs,
       for (i = 0; i < elements; i++)
 	{
 	  int regno = AARCH64_V0_REGNUM + i;
-	  bfd_byte tmpbuf[MAX_REGISTER_SIZE];
+	  bfd_byte tmpbuf[V_REGISTER_SIZE];
 
 	  if (aarch64_debug)
 	    {
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 0ae311f..42a39dc 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -69,6 +69,10 @@
 #include "features/arm/arm-with-vfpv3.c"
 #include "features/arm/arm-with-neon.c"
 
+#if GDB_SELF_TEST
+#include "selftest.h"
+#endif
+
 static int arm_debug;
 
 /* Macros for setting and testing a bit in a minimal symbol that marks
@@ -4237,6 +4241,23 @@ convert_to_extended (const struct floatformat *fmt, void *dbl, const void *ptr,
 			       &d, dbl);
 }
 
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+static void
+arm_floatformat_test (void)
+{
+  SELF_CHECK (floatformat_totalsize_bytes (&floatformat_arm_ext_big)
+	      == FP_REGISTER_SIZE);
+  SELF_CHECK (floatformat_totalsize_bytes (&floatformat_arm_ext_littlebyte_bigword)
+	      == FP_REGISTER_SIZE);
+}
+
+} // namespace selftests
+
+#endif /* GDB_SELF_TEST */
+
 /* Given BUF, which is OLD_LEN bytes ending at ENDADDR, expand
    the buffer to be NEW_LEN bytes ending at ENDADDR.  Return
    NULL if an error occurs.  BUF is freed.  */
@@ -8153,11 +8174,10 @@ arm_store_return_value (struct type *type, struct regcache *regs,
 
   if (TYPE_CODE (type) == TYPE_CODE_FLT)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-
       switch (gdbarch_tdep (gdbarch)->fp_model)
 	{
 	case ARM_FLOAT_FPA:
+	  gdb_byte buf[FP_REGISTER_SIZE];
 
 	  convert_to_extended (floatformat_from_type (type), buf, valbuf,
 			       gdbarch_byte_order (gdbarch));
@@ -9717,6 +9737,10 @@ vfp - VFP co-processor."),
 			   NULL,
 			   NULL, /* FIXME: i18n: "ARM debugging is %s.  */
 			   &setdebuglist, &showdebuglist);
+
+#if GDB_SELF_TEST
+  register_self_test (selftests::arm_floatformat_test);
+#endif
 }
 
 /* ARM-reversible process record data structures.  */