[v4,02/10] Make gdbserver reg_defs a vector of objects
Commit Message
From: Alan Hayward <alan.hayward@arm.com>
Following a suggestion from Philipp, this patch changes reg_defs in
gdbserver tdesc so that it is a vector of objects instead of a vector
of pointers. Updated uses of reg_defs to use references.
This change reduces the number of small hunks allocated when building
reg_defs.
find_register_by_number() is only used by regcache, therefore I made
it a static function and moved it earlier in the file so that find_regno()
could use it. The eventual plan is to replace this with accessor functions
in a common tdesc class.
Alan.
2018-03-21 Alan Hayward <alan.hayward@arm.com>
gdb/gdbserver/
* regcache.c (find_register_by_number): Make static and return a ref.
(find_regno): Use find_register_by_number
(register_size): Use references.
(register_data): Likewise.
* regcache.h (struct reg): Remove declaration.
* tdesc.c (target_desc::~target_desc): Remove free calls.
(target_desc::operator==): Use references.
(init_target_desc): Likewise.
(tdesc_create_reg): Use references and resize in a single call.
* tdesc.h (struct target_desc): Replace pointer with object.
---
gdb/gdbserver/regcache.c | 27 +++++++++++----------------
gdb/gdbserver/regcache.h | 4 ----
gdb/gdbserver/tdesc.c | 30 ++++++++++++++----------------
gdb/gdbserver/tdesc.h | 2 +-
4 files changed, 26 insertions(+), 37 deletions(-)
Comments
On 2018-03-22 04:44 AM, alan.hayward@arm.com wrote:
> From: Alan Hayward <alan.hayward@arm.com>
>
> Following a suggestion from Philipp, this patch changes reg_defs in
> gdbserver tdesc so that it is a vector of objects instead of a vector
> of pointers. Updated uses of reg_defs to use references.
>
> This change reduces the number of small hunks allocated when building
> reg_defs.
>
> find_register_by_number() is only used by regcache, therefore I made
> it a static function and moved it earlier in the file so that find_regno()
> could use it. The eventual plan is to replace this with accessor functions
> in a common tdesc class.
>
> Alan.
>
> 2018-03-21 Alan Hayward <alan.hayward@arm.com>
>
> gdb/gdbserver/
> * regcache.c (find_register_by_number): Make static and return a ref.
> (find_regno): Use find_register_by_number
> (register_size): Use references.
> (register_data): Likewise.
> * regcache.h (struct reg): Remove declaration.
> * tdesc.c (target_desc::~target_desc): Remove free calls.
> (target_desc::operator==): Use references.
> (init_target_desc): Likewise.
> (tdesc_create_reg): Use references and resize in a single call.
> * tdesc.h (struct target_desc): Replace pointer with object.
> ---
> gdb/gdbserver/regcache.c | 27 +++++++++++----------------
> gdb/gdbserver/regcache.h | 4 ----
> gdb/gdbserver/tdesc.c | 30 ++++++++++++++----------------
> gdb/gdbserver/tdesc.h | 2 +-
> 4 files changed, 26 insertions(+), 37 deletions(-)
>
> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
> index 1bb15900dd..df5b9e9dd2 100644
> --- a/gdb/gdbserver/regcache.c
> +++ b/gdb/gdbserver/regcache.c
> @@ -196,6 +196,13 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
> dst->registers_valid = src->registers_valid;
> }
>
> +/* Return a reference to the description of register ``n''. */
> +
> +static const struct reg &
> +find_register_by_number (const struct target_desc *tdesc, int n)
> +{
> + return tdesc->reg_defs[n];
> +}
>
> #ifndef IN_PROCESS_AGENT
>
> @@ -243,25 +250,13 @@ int
> find_regno (const struct target_desc *tdesc, const char *name)
> {
> for (int i = 0; i < tdesc->reg_defs.size (); ++i)
> - {
> - struct reg *reg = tdesc->reg_defs[i];
> + if (strcmp (name, find_register_by_number (tdesc, i).name) == 0)
> + return i;
>
> - if (strcmp (name, reg->name) == 0)
> - return i;
> - }
> internal_error (__FILE__, __LINE__, "Unknown register %s requested",
> name);
Please keep the curly braces for the for, as shown here:
https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#index-multiple-variables-in-a-line
Can you put the moving of this function/making it static/removing the declaration
in its own patch? It's pre-approved, with that fixed.
> }
>
> -#endif
> -
> -struct reg *
> -find_register_by_number (const struct target_desc *tdesc, int n)
> -{
> - return tdesc->reg_defs[n];
> -}
> -
> -#ifndef IN_PROCESS_AGENT
> static void
> free_register_cache_thread (struct thread_info *thread)
> {
> @@ -292,7 +287,7 @@ register_cache_size (const struct target_desc *tdesc)
> int
> register_size (const struct target_desc *tdesc, int n)
> {
> - return find_register_by_number (tdesc, n)->size / 8;
> + return find_register_by_number (tdesc, n).size / 8;
> }
>
> /* See common/common-regcache.h. */
> @@ -307,7 +302,7 @@ static unsigned char *
> register_data (struct regcache *regcache, int n, int fetch)
> {
> return (regcache->registers
> - + find_register_by_number (regcache->tdesc, n)->offset / 8);
> + + find_register_by_number (regcache->tdesc, n).offset / 8);
> }
>
> /* Supply register N, whose contents are stored in BUF, to REGCACHE.
> diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
> index 3a75ce3fe1..6ff13084b0 100644
> --- a/gdb/gdbserver/regcache.h
> +++ b/gdb/gdbserver/regcache.h
> @@ -94,10 +94,6 @@ void registers_from_string (struct regcache *regcache, char *buf);
>
> void regcache_write_pc (struct regcache *regcache, CORE_ADDR pc);
>
> -/* Return a pointer to the description of register ``n''. */
> -
> -struct reg *find_register_by_number (const struct target_desc *tdesc, int n);
> -
> int register_cache_size (const struct target_desc *tdesc);
>
> int register_size (const struct target_desc *tdesc, int n);
> diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
> index e50a848e2f..8e68a27c7c 100644
> --- a/gdb/gdbserver/tdesc.c
> +++ b/gdb/gdbserver/tdesc.c
> @@ -25,9 +25,6 @@ target_desc::~target_desc ()
> {
> int i;
>
> - for (reg *reg : reg_defs)
> - xfree (reg);
> -
> xfree ((char *) arch);
> xfree ((char *) osabi);
>
> @@ -45,10 +42,10 @@ bool target_desc::operator== (const target_desc &other) const
>
> for (int i = 0; i < reg_defs.size (); ++i)
> {
> - struct reg *reg = reg_defs[i];
> - struct reg *reg2 = other.reg_defs[i];
> + struct reg reg = reg_defs[i];
> + struct reg reg2 = other.reg_defs[i];
>
> - if (reg != reg2 && *reg != *reg2)
> + if (® != ®2 && reg != reg2)
I don't think the first part, "® != ®", makes sense. It's comparing
the addresses of the local variables. Also, that comparison only made
sense as a shortcut when the vector held pointers to dynamically allocated
memory. So I think it should just be removed.
Actually, now that the vector elements are the objects directly, I think that
this whole for loop could be replaced with:
if (reg_defs != other.reg_defs)
return false;
The if that compares the vector sizes above that can also be removed, since
it's also done by std::vector::operator==:
template<typename _Tp, typename _Alloc>
inline bool
operator==(const vector<_Tp, _Alloc>& __x, const vector<_Tp, _Alloc>& __y)
{ return (__x.size() == __y.size()
&& std::equal(__x.begin(), __x.end(), __y.begin())); }
> return false;
> }
>
> @@ -72,10 +69,10 @@ init_target_desc (struct target_desc *tdesc)
> {
> int offset = 0;
>
> - for (reg *reg : tdesc->reg_defs)
> + for (reg ® : tdesc->reg_defs)
> {
> - reg->offset = offset;
> - offset += reg->size;
> + reg.offset = offset;
> + offset += reg.size;
> }
>
> tdesc->registers_size = offset / 8;
> @@ -240,24 +237,25 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,
> int bitsize, const char *type)
> {
> struct target_desc *tdesc = (struct target_desc *) feature;
> + int current_size = tdesc->reg_defs.size ();
>
> - while (tdesc->reg_defs.size () < regnum)
> - {
> - struct reg *reg = XCNEW (struct reg);
> + tdesc->reg_defs.resize (regnum != 0 ? regnum + 1 : current_size + 1);
>
> + while (current_size < regnum)
> + {
> + struct reg *reg = &tdesc->reg_defs[current_size];
> reg->name = "";
> reg->size = 0;
> - tdesc->reg_defs.push_back (reg);
> + current_size++;
> }
>
> gdb_assert (regnum == 0
> - || regnum == tdesc->reg_defs.size ());
> + || regnum + 1 == tdesc->reg_defs.size ());
>
> - struct reg *reg = XCNEW (struct reg);
> + struct reg *reg = &tdesc->reg_defs.back ();
>
> reg->name = name;
> reg->size = bitsize;
> - tdesc->reg_defs.push_back (reg);
> }
To simplify this further, I would suggest adding a default constructor to reg
that initializes everything to nullptr/0, and adding a constructor that takes
parameters. The .resize() will only be necessary if regnum != 0 and will
automatically initialize all the unused register slots properly, so the for
loop will be unnecessary:
if (regnum != 0)
tdesc->reg_defs.resize (regnum);
The register can then be added with
tdesc->reg_defs.emplace_back (name, bitsize);
One behavior that might change compared to the previous code is when trying to
add a register with a regnum smaller than the current size of the vector. I
think that the old code would have asserted, but the new code would just
accept it and resize the vector to a smaller size. Could you verify? In that
case, maybe we should move the gdb_assert to the beginning of the function:
gdb_assert (regnum == 0 || regnum >= tdesc->reg_defs.size ())
something like that. It can also be moved under the "if (regnum != 0)" shown
above.
>
> /* See common/tdesc.h. */
> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index 4513ea7423..a62544341c 100644
> --- a/gdb/gdbserver/tdesc.h
> +++ b/gdb/gdbserver/tdesc.h
> @@ -34,7 +34,7 @@ struct target_desc : tdesc_feature
> {
> /* A vector of elements of register definitions that
> describe the inferior's register set. */
> - std::vector<struct reg *> reg_defs;
> + std::vector<struct reg> reg_defs;
>
> /* The register cache size, in bytes. */
> int registers_size;
>
Thanks,
Simon
@@ -196,6 +196,13 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
dst->registers_valid = src->registers_valid;
}
+/* Return a reference to the description of register ``n''. */
+
+static const struct reg &
+find_register_by_number (const struct target_desc *tdesc, int n)
+{
+ return tdesc->reg_defs[n];
+}
#ifndef IN_PROCESS_AGENT
@@ -243,25 +250,13 @@ int
find_regno (const struct target_desc *tdesc, const char *name)
{
for (int i = 0; i < tdesc->reg_defs.size (); ++i)
- {
- struct reg *reg = tdesc->reg_defs[i];
+ if (strcmp (name, find_register_by_number (tdesc, i).name) == 0)
+ return i;
- if (strcmp (name, reg->name) == 0)
- return i;
- }
internal_error (__FILE__, __LINE__, "Unknown register %s requested",
name);
}
-#endif
-
-struct reg *
-find_register_by_number (const struct target_desc *tdesc, int n)
-{
- return tdesc->reg_defs[n];
-}
-
-#ifndef IN_PROCESS_AGENT
static void
free_register_cache_thread (struct thread_info *thread)
{
@@ -292,7 +287,7 @@ register_cache_size (const struct target_desc *tdesc)
int
register_size (const struct target_desc *tdesc, int n)
{
- return find_register_by_number (tdesc, n)->size / 8;
+ return find_register_by_number (tdesc, n).size / 8;
}
/* See common/common-regcache.h. */
@@ -307,7 +302,7 @@ static unsigned char *
register_data (struct regcache *regcache, int n, int fetch)
{
return (regcache->registers
- + find_register_by_number (regcache->tdesc, n)->offset / 8);
+ + find_register_by_number (regcache->tdesc, n).offset / 8);
}
/* Supply register N, whose contents are stored in BUF, to REGCACHE.
@@ -94,10 +94,6 @@ void registers_from_string (struct regcache *regcache, char *buf);
void regcache_write_pc (struct regcache *regcache, CORE_ADDR pc);
-/* Return a pointer to the description of register ``n''. */
-
-struct reg *find_register_by_number (const struct target_desc *tdesc, int n);
-
int register_cache_size (const struct target_desc *tdesc);
int register_size (const struct target_desc *tdesc, int n);
@@ -25,9 +25,6 @@ target_desc::~target_desc ()
{
int i;
- for (reg *reg : reg_defs)
- xfree (reg);
-
xfree ((char *) arch);
xfree ((char *) osabi);
@@ -45,10 +42,10 @@ bool target_desc::operator== (const target_desc &other) const
for (int i = 0; i < reg_defs.size (); ++i)
{
- struct reg *reg = reg_defs[i];
- struct reg *reg2 = other.reg_defs[i];
+ struct reg reg = reg_defs[i];
+ struct reg reg2 = other.reg_defs[i];
- if (reg != reg2 && *reg != *reg2)
+ if (® != ®2 && reg != reg2)
return false;
}
@@ -72,10 +69,10 @@ init_target_desc (struct target_desc *tdesc)
{
int offset = 0;
- for (reg *reg : tdesc->reg_defs)
+ for (reg ® : tdesc->reg_defs)
{
- reg->offset = offset;
- offset += reg->size;
+ reg.offset = offset;
+ offset += reg.size;
}
tdesc->registers_size = offset / 8;
@@ -240,24 +237,25 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,
int bitsize, const char *type)
{
struct target_desc *tdesc = (struct target_desc *) feature;
+ int current_size = tdesc->reg_defs.size ();
- while (tdesc->reg_defs.size () < regnum)
- {
- struct reg *reg = XCNEW (struct reg);
+ tdesc->reg_defs.resize (regnum != 0 ? regnum + 1 : current_size + 1);
+ while (current_size < regnum)
+ {
+ struct reg *reg = &tdesc->reg_defs[current_size];
reg->name = "";
reg->size = 0;
- tdesc->reg_defs.push_back (reg);
+ current_size++;
}
gdb_assert (regnum == 0
- || regnum == tdesc->reg_defs.size ());
+ || regnum + 1 == tdesc->reg_defs.size ());
- struct reg *reg = XCNEW (struct reg);
+ struct reg *reg = &tdesc->reg_defs.back ();
reg->name = name;
reg->size = bitsize;
- tdesc->reg_defs.push_back (reg);
}
/* See common/tdesc.h. */
@@ -34,7 +34,7 @@ struct target_desc : tdesc_feature
{
/* A vector of elements of register definitions that
describe the inferior's register set. */
- std::vector<struct reg *> reg_defs;
+ std::vector<struct reg> reg_defs;
/* The register cache size, in bytes. */
int registers_size;