Re: [PATCH] Removal of uses of MAX_REGISTER_SIZE

Alan Hayward
2017-02-03T09:59:28Z

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