Re: [PATCH] Removal of uses of MAX_REGISTER_SIZE

Yao Qi
2017-02-03T10:28:41Z

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