[v5,1/8] Commonise tdesc_reg
Commit Message
This patch commonises tdesc_reg and makes use of it in gdbserver tdesc.
gdbserver tdesc_create_reg is changed to create a tdesc_reg instead of
a reg_defs entry. The vector of tdesc_reg is held inside tdesc_feature.
However, other modules in gdbserver directly access the reg_defs structure.
To work around this, init_target_desc fills in reg_defs by parsing the
tdesc_reg vector.
The long term goal is to remove reg_defs, replacing with accessor funcs.
I wanted to make tdesc_create_reg common, but I cannot do that until
the next patch.
I also had to commonise tdesc_element_visitor and tdesc_element.
This patch only differs from the V4 version in init_target_desc() and
the changing of constructors for regdef.
2018-04-10 Alan Hayward <alan.hayward@arm.com>
gdb/
* Makefile.in: Add arch/tdesc.c
* common/tdesc.c: New file.
* common/tdesc.h (tdesc_element_visitor): Move to here.
(tdesc_element): Likewise.
(tdesc_reg): Likewise.
(tdesc_reg_up): Likewise.
* regformats/regdef.h (reg): Add offset to constructors.
* target-descriptions.c (tdesc_element_visitor): Move from here.
(tdesc_element): Likewise.
(tdesc_reg): Likewise.
(tdesc_reg_up): Likewise.
gdbserver/
* Makefile.in: Add common/tdesc.c
* tdesc.c (init_target_desc): init all reg_defs from register vector.
(tdesc_create_reg): Create tdesc_reg.
* tdesc.h (tdesc_feature): Add register vector.
---
gdb/Makefile.in | 2 +
gdb/common/tdesc.c | 35 ++++++++++++++
gdb/common/tdesc.h | 105 +++++++++++++++++++++++++++++++++++++++++
gdb/gdbserver/Makefile.in | 3 ++
gdb/gdbserver/tdesc.c | 23 +++++----
gdb/gdbserver/tdesc.h | 5 +-
gdb/regformats/regdef.h | 8 ++--
gdb/target-descriptions.c | 116 ----------------------------------------------
8 files changed, 168 insertions(+), 129 deletions(-)
create mode 100644 gdb/common/tdesc.c
Comments
On 2018-04-10 10:33 AM, Alan Hayward wrote:
> This patch commonises tdesc_reg and makes use of it in gdbserver tdesc.
>
> gdbserver tdesc_create_reg is changed to create a tdesc_reg instead of
> a reg_defs entry. The vector of tdesc_reg is held inside tdesc_feature.
>
> However, other modules in gdbserver directly access the reg_defs structure.
> To work around this, init_target_desc fills in reg_defs by parsing the
> tdesc_reg vector.
> The long term goal is to remove reg_defs, replacing with accessor funcs.
>
> I wanted to make tdesc_create_reg common, but I cannot do that until
> the next patch.
>
> I also had to commonise tdesc_element_visitor and tdesc_element.
>
> This patch only differs from the V4 version in init_target_desc() and
> the changing of constructors for regdef.
Hi Alan,
Just two small comment, but the patch LGTM with those answered or addressed.
> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index 85139d948c..8eb88eedce 100644
> --- a/gdb/gdbserver/tdesc.h
> +++ b/gdb/gdbserver/tdesc.h
> @@ -25,7 +25,10 @@
> #include <vector>
>
> struct tdesc_feature
> -{};
> +{
> + /* The registers associated with this feature. */
> + std::vector<tdesc_reg_up> registers;
> +};
>
> /* A target description. Inherit from tdesc_feature so that target_desc
> can be used as tdesc_feature. */
> diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
> index 4775e863e9..7c80d45d48 100644
> --- a/gdb/regformats/regdef.h
> +++ b/gdb/regformats/regdef.h
> @@ -21,15 +21,15 @@
>
> struct reg
> {
> - reg ()
> + reg (int _offset)
> : name (""),
> - offset (0),
> + offset (_offset),
> size (0)
> {}
If this constructor is only used for padding entries, shouldn't name be
NULL, as the documentation for the field states?
Also, did you notice something failing if the padding entries don't have
the offset field to the "current" offset at the time they are created?
If we could leave them at 0, I think it would keep things simpler. I
stopped for a few seconds, wondering why init_target_desc did:
tdesc->reg_defs.resize (regnum, reg (offset));
and not just:
tdesc->reg_defs.resize (regnum);
Simon
> On 18 Apr 2018, at 02:57, Simon Marchi <simark@simark.ca> wrote:
>
> On 2018-04-10 10:33 AM, Alan Hayward wrote:
>> This patch commonises tdesc_reg and makes use of it in gdbserver tdesc.
>>
>> gdbserver tdesc_create_reg is changed to create a tdesc_reg instead of
>> a reg_defs entry. The vector of tdesc_reg is held inside tdesc_feature.
>>
>> However, other modules in gdbserver directly access the reg_defs structure.
>> To work around this, init_target_desc fills in reg_defs by parsing the
>> tdesc_reg vector.
>> The long term goal is to remove reg_defs, replacing with accessor funcs.
>>
>> I wanted to make tdesc_create_reg common, but I cannot do that until
>> the next patch.
>>
>> I also had to commonise tdesc_element_visitor and tdesc_element.
>>
>> This patch only differs from the V4 version in init_target_desc() and
>> the changing of constructors for regdef.
>
> Hi Alan,
>
> Just two small comment, but the patch LGTM with those answered or addressed.
>
>> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
>> index 85139d948c..8eb88eedce 100644
>> --- a/gdb/gdbserver/tdesc.h
>> +++ b/gdb/gdbserver/tdesc.h
>> @@ -25,7 +25,10 @@
>> #include <vector>
>>
>> struct tdesc_feature
>> -{};
>> +{
>> + /* The registers associated with this feature. */
>> + std::vector<tdesc_reg_up> registers;
>> +};
>>
>> /* A target description. Inherit from tdesc_feature so that target_desc
>> can be used as tdesc_feature. */
>> diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
>> index 4775e863e9..7c80d45d48 100644
>> --- a/gdb/regformats/regdef.h
>> +++ b/gdb/regformats/regdef.h
>> @@ -21,15 +21,15 @@
>>
>> struct reg
>> {
>> - reg ()
>> + reg (int _offset)
>> : name (""),
>> - offset (0),
>> + offset (_offset),
>> size (0)
>> {}
>
> If this constructor is only used for padding entries, shouldn't name be
> NULL, as the documentation for the field states?
>
That creates two issues:
The reg:operator== segfaults in the strcmp due to the null.
Easily fixable, but a little ugly.
With that patched, the gdbserver unit tests will fail.
That is because the creation of target descriptions in the -generated.c files
in the build dir create the gaps using:
tdesc_create_reg (feature, “", 0, 0, NULL, 0, NULL);
Would fixing that cause more issues? Not sure.
Having “” rather than 0 does make sense when you format these to xml files.
We are explicitly setting to a blank string. Whereas null is more for uninitialised.
I was going to suggest fixing in a follow on patch, but the more I think about it,
The more I’m thinking “” is correct.
> Also, did you notice something failing if the padding entries don't have
> the offset field to the "current" offset at the time they are created?
> If we could leave them at 0, I think it would keep things simpler. I
> stopped for a few seconds, wondering why init_target_desc did:
>
> tdesc->reg_defs.resize (regnum, reg (offset));
>
> and not just:
>
> tdesc->reg_defs.resize (regnum);
Again, this creates unit test failures due to the same tdesc_create_reg above.
Here offset is a little more explicit, and setting to 0 says it’s at the start of the
description, which isn’t right.
Thanks again for the reviews.
Alan.
On 2018-04-18 05:03, Alan Hayward wrote:
>> On 18 Apr 2018, at 02:57, Simon Marchi <simark@simark.ca> wrote:
>>
>> On 2018-04-10 10:33 AM, Alan Hayward wrote:
>>> This patch commonises tdesc_reg and makes use of it in gdbserver
>>> tdesc.
>>>
>>> gdbserver tdesc_create_reg is changed to create a tdesc_reg instead
>>> of
>>> a reg_defs entry. The vector of tdesc_reg is held inside
>>> tdesc_feature.
>>>
>>> However, other modules in gdbserver directly access the reg_defs
>>> structure.
>>> To work around this, init_target_desc fills in reg_defs by parsing
>>> the
>>> tdesc_reg vector.
>>> The long term goal is to remove reg_defs, replacing with accessor
>>> funcs.
>>>
>>> I wanted to make tdesc_create_reg common, but I cannot do that until
>>> the next patch.
>>>
>>> I also had to commonise tdesc_element_visitor and tdesc_element.
>>>
>>> This patch only differs from the V4 version in init_target_desc() and
>>> the changing of constructors for regdef.
>>
>> Hi Alan,
>>
>> Just two small comment, but the patch LGTM with those answered or
>> addressed.
>>
>>> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
>>> index 85139d948c..8eb88eedce 100644
>>> --- a/gdb/gdbserver/tdesc.h
>>> +++ b/gdb/gdbserver/tdesc.h
>>> @@ -25,7 +25,10 @@
>>> #include <vector>
>>>
>>> struct tdesc_feature
>>> -{};
>>> +{
>>> + /* The registers associated with this feature. */
>>> + std::vector<tdesc_reg_up> registers;
>>> +};
>>>
>>> /* A target description. Inherit from tdesc_feature so that
>>> target_desc
>>> can be used as tdesc_feature. */
>>> diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
>>> index 4775e863e9..7c80d45d48 100644
>>> --- a/gdb/regformats/regdef.h
>>> +++ b/gdb/regformats/regdef.h
>>> @@ -21,15 +21,15 @@
>>>
>>> struct reg
>>> {
>>> - reg ()
>>> + reg (int _offset)
>>> : name (""),
>>> - offset (0),
>>> + offset (_offset),
>>> size (0)
>>> {}
>>
>> If this constructor is only used for padding entries, shouldn't name
>> be
>> NULL, as the documentation for the field states?
>>
>
> That creates two issues:
>
> The reg:operator== segfaults in the strcmp due to the null.
> Easily fixable, but a little ugly.
>
> With that patched, the gdbserver unit tests will fail.
> That is because the creation of target descriptions in the
> -generated.c files
> in the build dir create the gaps using:
> tdesc_create_reg (feature, “", 0, 0, NULL, 0, NULL);
>
> Would fixing that cause more issues? Not sure.
>
> Having “” rather than 0 does make sense when you format these to xml
> files.
> We are explicitly setting to a blank string. Whereas null is more for
> uninitialised.
>
> I was going to suggest fixing in a follow on patch, but the more I
> think about it,
> The more I’m thinking “” is correct.
The important thing is that documentation is accurate, so could you
adjust the doc of the name field accordingly?
>> Also, did you notice something failing if the padding entries don't
>> have
>> the offset field to the "current" offset at the time they are created?
>> If we could leave them at 0, I think it would keep things simpler. I
>> stopped for a few seconds, wondering why init_target_desc did:
>>
>> tdesc->reg_defs.resize (regnum, reg (offset));
>>
>> and not just:
>>
>> tdesc->reg_defs.resize (regnum);
>
> Again, this creates unit test failures due to the same tdesc_create_reg
> above.
> Here offset is a little more explicit, and setting to 0 says it’s at
> the start of the
> description, which isn’t right.
Ok.
Thanks,
Simon
@@ -968,6 +968,7 @@ COMMON_SFILES = \
common/run-time-clock.c \
common/signals.c \
common/signals-state-save-restore.c \
+ common/tdesc.c \
common/vec.c \
common/xml-utils.c \
complaints.c \
@@ -1449,6 +1450,7 @@ HFILES_NO_SRCDIR = \
common/run-time-clock.h \
common/signals-state-save-restore.h \
common/symbol.h \
+ common/tdesc.h \
common/vec.h \
common/version.h \
common/x86-xstate.h \
new file mode 100644
@@ -0,0 +1,35 @@
+/* Target description support for GDB.
+
+ Copyright (C) 2018 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "common-defs.h"
+#include "common/tdesc.h"
+
+tdesc_reg::tdesc_reg (struct tdesc_feature *feature, const std::string &name_,
+ int regnum, int save_restore_, const char *group_,
+ int bitsize_, const char *type_)
+ : name (name_), target_regnum (regnum),
+ save_restore (save_restore_),
+ group (group_ != NULL ? group_ : ""),
+ bitsize (bitsize_),
+ type (type_ != NULL ? type_ : "<unknown>")
+{
+ /* If the register's type is target-defined, look it up now. We may not
+ have easy access to the containing feature when we want it later. */
+ tdesc_type = tdesc_named_type (feature, type.c_str ());
+}
@@ -26,6 +26,111 @@ struct tdesc_type_with_fields;
struct tdesc_reg;
struct target_desc;
+/* The interface to visit different elements of target description. */
+
+class tdesc_element_visitor
+{
+public:
+ virtual void visit_pre (const target_desc *e)
+ {}
+
+ virtual void visit_post (const target_desc *e)
+ {}
+
+ virtual void visit_pre (const tdesc_feature *e)
+ {}
+
+ virtual void visit_post (const tdesc_feature *e)
+ {}
+
+ virtual void visit (const tdesc_type_builtin *e)
+ {}
+
+ virtual void visit (const tdesc_type_vector *e)
+ {}
+
+ virtual void visit (const tdesc_type_with_fields *e)
+ {}
+
+ virtual void visit (const tdesc_reg *e)
+ {}
+};
+
+class tdesc_element
+{
+public:
+ virtual void accept (tdesc_element_visitor &v) const = 0;
+};
+
+/* An individual register from a target description. */
+
+struct tdesc_reg : tdesc_element
+{
+ tdesc_reg (struct tdesc_feature *feature, const std::string &name_,
+ int regnum, int save_restore_, const char *group_,
+ int bitsize_, const char *type_);
+
+ virtual ~tdesc_reg () = default;
+
+ DISABLE_COPY_AND_ASSIGN (tdesc_reg);
+
+ /* The name of this register. In standard features, it may be
+ recognized by the architecture support code, or it may be purely
+ for the user. */
+ std::string name;
+
+ /* The register number used by this target to refer to this
+ register. This is used for remote p/P packets and to determine
+ the ordering of registers in the remote g/G packets. */
+ long target_regnum;
+
+ /* If this flag is set, GDB should save and restore this register
+ around calls to an inferior function. */
+ int save_restore;
+
+ /* The name of the register group containing this register, or empty
+ if the group should be automatically determined from the
+ register's type. If this is "general", "float", or "vector", the
+ corresponding "info" command should display this register's
+ value. It can be an arbitrary string, but should be limited to
+ alphanumeric characters and internal hyphens. Currently other
+ strings are ignored (treated as empty). */
+ std::string group;
+
+ /* The size of the register, in bits. */
+ int bitsize;
+
+ /* The type of the register. This string corresponds to either
+ a named type from the target description or a predefined
+ type from GDB. */
+ std::string type;
+
+ /* The target-described type corresponding to TYPE, if found. */
+ struct tdesc_type *tdesc_type;
+
+ void accept (tdesc_element_visitor &v) const override
+ {
+ v.visit (this);
+ }
+
+ bool operator== (const tdesc_reg &other) const
+ {
+ return (name == other.name
+ && target_regnum == other.target_regnum
+ && save_restore == other.save_restore
+ && bitsize == other.bitsize
+ && group == other.group
+ && type == other.type);
+ }
+
+ bool operator!= (const tdesc_reg &other) const
+ {
+ return !(*this == other);
+ }
+};
+
+typedef std::unique_ptr<tdesc_reg> tdesc_reg_up;
+
/* Allocate a new target_desc. */
target_desc *allocate_target_description (void);
@@ -215,6 +215,7 @@ SFILES = \
$(srcdir)/common/print-utils.c \
$(srcdir)/common/ptid.c \
$(srcdir)/common/rsp-low.c \
+ $(srcdir)/common/tdesc.c \
$(srcdir)/common/vec.c \
$(srcdir)/common/xml-utils.c \
$(srcdir)/nat/linux-btrace.c \
@@ -258,6 +259,7 @@ OBS = \
common/rsp-low.o \
common/signals.o \
common/signals-state-save-restore.o \
+ common/tdesc.o \
common/vec.o \
common/xml-utils.o \
debug.o \
@@ -403,6 +405,7 @@ IPA_OBJS = \
common/format-ipa.o \
common/print-utils-ipa.o \
common/rsp-low-ipa.o \
+ common/tdesc-ipa.o \
common/vec-ipa.o \
regcache-ipa.o \
remote-utils-ipa.o \
@@ -54,10 +54,19 @@ init_target_desc (struct target_desc *tdesc)
{
int offset = 0;
- for (reg ® : tdesc->reg_defs)
+ /* Go through all the features and populate reg_defs. */
+ for (const tdesc_reg_up &treg : tdesc->registers)
{
- reg.offset = offset;
- offset += reg.size;
+ int regnum = treg->target_regnum;
+
+ /* Register number will increase (possibly with gaps) or be zero. */
+ gdb_assert (regnum == 0 || regnum >= tdesc->reg_defs.size ());
+
+ if (regnum != 0)
+ tdesc->reg_defs.resize (regnum, reg (offset));
+
+ tdesc->reg_defs.emplace_back (treg->name.c_str (), offset, treg->bitsize);
+ offset += treg->bitsize;
}
tdesc->registers_size = offset / 8;
@@ -222,12 +231,10 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,
{
struct target_desc *tdesc = (struct target_desc *) feature;
- gdb_assert (regnum == 0 || regnum >= tdesc->reg_defs.size ());
-
- if (regnum != 0)
- tdesc->reg_defs.resize (regnum);
+ tdesc_reg *reg = new tdesc_reg (feature, name, regnum, save_restore,
+ group, bitsize, type);
- tdesc->reg_defs.emplace_back (name, bitsize);
+ tdesc->registers.emplace_back (reg);
}
/* See common/tdesc.h. */
@@ -25,7 +25,10 @@
#include <vector>
struct tdesc_feature
-{};
+{
+ /* The registers associated with this feature. */
+ std::vector<tdesc_reg_up> registers;
+};
/* A target description. Inherit from tdesc_feature so that target_desc
can be used as tdesc_feature. */
@@ -21,15 +21,15 @@
struct reg
{
- reg ()
+ reg (int _offset)
: name (""),
- offset (0),
+ offset (_offset),
size (0)
{}
- reg (const char *_name, int _size)
+ reg (const char *_name, int _offset, int _size)
: name (_name),
- offset (0),
+ offset (_offset),
size (_size)
{}
@@ -38,44 +38,6 @@
#include "completer.h"
#include "readline/tilde.h" /* tilde_expand */
-static type *make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype);
-
-/* The interface to visit different elements of target description. */
-
-class tdesc_element_visitor
-{
-public:
- virtual void visit_pre (const target_desc *e)
- {}
-
- virtual void visit_post (const target_desc *e)
- {}
-
- virtual void visit_pre (const tdesc_feature *e)
- {}
-
- virtual void visit_post (const tdesc_feature *e)
- {}
-
- virtual void visit (const tdesc_type_builtin *e)
- {}
-
- virtual void visit (const tdesc_type_vector *e)
- {}
-
- virtual void visit (const tdesc_type_with_fields *e)
- {}
-
- virtual void visit (const tdesc_reg *e)
- {}
-};
-
-class tdesc_element
-{
-public:
- virtual void accept (tdesc_element_visitor &v) const = 0;
-};
-
/* Types. */
struct property
@@ -88,84 +50,6 @@ struct property
std::string value;
};
-/* An individual register from a target description. */
-
-struct tdesc_reg : tdesc_element
-{
- tdesc_reg (struct tdesc_feature *feature, const std::string &name_,
- int regnum, int save_restore_, const char *group_,
- int bitsize_, const char *type_)
- : name (name_), target_regnum (regnum),
- save_restore (save_restore_),
- group (group_ != NULL ? group_ : ""),
- bitsize (bitsize_),
- type (type_ != NULL ? type_ : "<unknown>")
- {
- /* If the register's type is target-defined, look it up now. We may not
- have easy access to the containing feature when we want it later. */
- tdesc_type = tdesc_named_type (feature, type.c_str ());
- }
-
- virtual ~tdesc_reg () = default;
-
- DISABLE_COPY_AND_ASSIGN (tdesc_reg);
-
- /* The name of this register. In standard features, it may be
- recognized by the architecture support code, or it may be purely
- for the user. */
- std::string name;
-
- /* The register number used by this target to refer to this
- register. This is used for remote p/P packets and to determine
- the ordering of registers in the remote g/G packets. */
- long target_regnum;
-
- /* If this flag is set, GDB should save and restore this register
- around calls to an inferior function. */
- int save_restore;
-
- /* The name of the register group containing this register, or empty
- if the group should be automatically determined from the register's
- type. This is traditionally "general", "float", "vector" but can
- also be an arbitrary string. If defined the corresponding "info"
- command should display this register's value. The string should be
- limited to alphanumeric characters and internal hyphens. */
- std::string group;
-
- /* The size of the register, in bits. */
- int bitsize;
-
- /* The type of the register. This string corresponds to either
- a named type from the target description or a predefined
- type from GDB. */
- std::string type;
-
- /* The target-described type corresponding to TYPE, if found. */
- struct tdesc_type *tdesc_type;
-
- void accept (tdesc_element_visitor &v) const override
- {
- v.visit (this);
- }
-
- bool operator== (const tdesc_reg &other) const
- {
- return (name == other.name
- && target_regnum == other.target_regnum
- && save_restore == other.save_restore
- && bitsize == other.bitsize
- && group == other.group
- && type == other.type);
- }
-
- bool operator!= (const tdesc_reg &other) const
- {
- return !(*this == other);
- }
-};
-
-typedef std::unique_ptr<tdesc_reg> tdesc_reg_up;
-
/* A named type from a target description. */
struct tdesc_type_field