[v4,03/10] Commonise tdesc_reg

Message ID 20180322084429.26250-4-alan.hayward@arm.com
State New
Headers show
Series
  • Remove gdbserver dependency on xml files
Related show

Commit Message

Alan Hayward March 22, 2018, 8:44 a.m.
From: Alan Hayward <alan.hayward@arm.com>


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 V3 version in init_target_desc(),
where I now use reg_def references as introduced in the previous patch.

2018-03-21  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.
	* 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        |  40 ++++++++++++++++
 gdb/common/tdesc.h        | 111 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/Makefile.in |   3 ++
 gdb/gdbserver/tdesc.c     |  53 ++++++++++++---------
 gdb/gdbserver/tdesc.h     |   5 +-
 gdb/target-descriptions.c | 116 ----------------------------------------------
 7 files changed, 191 insertions(+), 139 deletions(-)
 create mode 100644 gdb/common/tdesc.c

-- 
2.14.3 (Apple Git-98)

Comments

Simon Marchi March 23, 2018, 7:50 p.m. | #1
On 2018-03-22 04:44 AM, alan.hayward@arm.com wrote:
> From: Alan Hayward <alan.hayward@arm.com>

> 

> 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 V3 version in init_target_desc(),

> where I now use reg_def references as introduced in the previous patch.


Hi Alan,

I have just a few superficial comments because I don't have the big picture
yet, I'll continue with the next patches and might come back.

> 2018-03-21  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.

> 	* target-descriptions.c: (tdesc_element_visitor): Move from here.


Spurious colon after the filename.

> 	(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        |  40 ++++++++++++++++

>  gdb/common/tdesc.h        | 111 ++++++++++++++++++++++++++++++++++++++++++++

>  gdb/gdbserver/Makefile.in |   3 ++

>  gdb/gdbserver/tdesc.c     |  53 ++++++++++++---------

>  gdb/gdbserver/tdesc.h     |   5 +-

>  gdb/target-descriptions.c | 116 ----------------------------------------------

>  7 files changed, 191 insertions(+), 139 deletions(-)

>  create mode 100644 gdb/common/tdesc.c

> 

> diff --git a/gdb/Makefile.in b/gdb/Makefile.in

> index 44db1937b6..f1d2f0193a 100644

> --- a/gdb/Makefile.in

> +++ b/gdb/Makefile.in

> @@ -966,6 +966,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 \

> @@ -1443,6 +1444,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 \

> diff --git a/gdb/common/tdesc.c b/gdb/common/tdesc.c

> new file mode 100644

> index 0000000000..a5f2de4b5b

> --- /dev/null

> +++ b/gdb/common/tdesc.c

> @@ -0,0 +1,40 @@

> +/* Target description support for GDB.

> +

> +   Copyright (C) 2017 Free Software Foundation, Inc.


2018

> +

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

> +

> +#ifdef GDBSERVER

> +#include "server.h"

> +#else

> +#include "defs.h"

> +#endif


We are aiming to remove all uses of #ifdef GDBSERVER in the code base, so we shouldn't
add any.  You might want to read this if you haven't seen it already:

  https://sourceware.org/gdb/wiki/Common

Replace this with #include "common-defs.h".  You can also remove the same snippet in
common/tdesc.h.  Header files assume that defs.h/server.h/common-defs.h has been
included already, so they don't need to do it.

> @@ -236,26 +261,10 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,

>  		  int regnum, int save_restore, const char *group,

>  		  int bitsize, const char *type)

>  {

> -  struct target_desc *tdesc = (struct target_desc *) feature;

> -  int current_size = tdesc->reg_defs.size ();

> -

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

> -      current_size++;

> -    }

> -

> -  gdb_assert (regnum == 0

> -	      || regnum + 1 == tdesc->reg_defs.size ());

> -

> -  struct reg *reg = &tdesc->reg_defs.back ();

> +  tdesc_reg *reg = new tdesc_reg (feature, name, regnum, save_restore,

> +				  group, bitsize, type);

>  

> -  reg->name = name;

> -  reg->size = bitsize;

> +  tdesc->registers.emplace_back (reg);


This doesn't build, I guess it should be feature->...

Simon
Alan Hayward March 26, 2018, 10:50 a.m. | #2
> On 23 Mar 2018, at 19:50, Simon Marchi <simon.marchi@ericsson.com> wrote:

> 

> On 2018-03-22 04:44 AM, alan.hayward@arm.com wrote:

>> From: Alan Hayward <alan.hayward@arm.com>

>> 

>> 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 V3 version in init_target_desc(),

>> where I now use reg_def references as introduced in the previous patch.

> 

> Hi Alan,

> 

> I have just a few superficial comments because I don't have the big picture

> yet, I'll continue with the next patches and might come back.

> 

>> 2018-03-21  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.

>> 	* target-descriptions.c: (tdesc_element_visitor): Move from here.

> 

> Spurious colon after the filename.

> 

>> 	(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        |  40 ++++++++++++++++

>> gdb/common/tdesc.h        | 111 ++++++++++++++++++++++++++++++++++++++++++++

>> gdb/gdbserver/Makefile.in |   3 ++

>> gdb/gdbserver/tdesc.c     |  53 ++++++++++++---------

>> gdb/gdbserver/tdesc.h     |   5 +-

>> gdb/target-descriptions.c | 116 ----------------------------------------------

>> 7 files changed, 191 insertions(+), 139 deletions(-)

>> create mode 100644 gdb/common/tdesc.c

>> 

>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in

>> index 44db1937b6..f1d2f0193a 100644

>> --- a/gdb/Makefile.in

>> +++ b/gdb/Makefile.in

>> @@ -966,6 +966,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 \

>> @@ -1443,6 +1444,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 \

>> diff --git a/gdb/common/tdesc.c b/gdb/common/tdesc.c

>> new file mode 100644

>> index 0000000000..a5f2de4b5b

>> --- /dev/null

>> +++ b/gdb/common/tdesc.c

>> @@ -0,0 +1,40 @@

>> +/* Target description support for GDB.

>> +

>> +   Copyright (C) 2017 Free Software Foundation, Inc.

> 

> 2018

> 

>> +

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

>> +

>> +#ifdef GDBSERVER

>> +#include "server.h"

>> +#else

>> +#include "defs.h"

>> +#endif

> 

> We are aiming to remove all uses of #ifdef GDBSERVER in the code base, so we shouldn't

> add any.  You might want to read this if you haven't seen it already:

> 

>  https://sourceware.org/gdb/wiki/Common

> 

> Replace this with #include "common-defs.h".  You can also remove the same snippet in

> common/tdesc.h.  Header files assume that defs.h/server.h/common-defs.h has been

> included already, so they don't need to do it.


Aha! I was trying to find a better way of doing those includes. That was the best I could reduce
it down to. Will update, and read through the links on that wiki page.

> 

>> @@ -236,26 +261,10 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,

>> 		  int regnum, int save_restore, const char *group,

>> 		  int bitsize, const char *type)

>> {

>> -  struct target_desc *tdesc = (struct target_desc *) feature;

>> -  int current_size = tdesc->reg_defs.size ();

>> -

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

>> -      current_size++;

>> -    }

>> -

>> -  gdb_assert (regnum == 0

>> -	      || regnum + 1 == tdesc->reg_defs.size ());

>> -

>> -  struct reg *reg = &tdesc->reg_defs.back ();

>> +  tdesc_reg *reg = new tdesc_reg (feature, name, regnum, save_restore,

>> +				  group, bitsize, type);

>> 

>> -  reg->name = name;

>> -  reg->size = bitsize;

>> +  tdesc->registers.emplace_back (reg);

> 

> This doesn't build, I guess it should be feature->…

> 


Yeah, after the 02 changes this patch needs a little bit of a rewrite.

I’m going to be away from a pc for two weeks now. Will update this (and part 4 too),
when I get back.


Alan.

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 44db1937b6..f1d2f0193a 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -966,6 +966,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 \
@@ -1443,6 +1444,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 \
diff --git a/gdb/common/tdesc.c b/gdb/common/tdesc.c
new file mode 100644
index 0000000000..a5f2de4b5b
--- /dev/null
+++ b/gdb/common/tdesc.c
@@ -0,0 +1,40 @@ 
+/* Target description support for GDB.
+
+   Copyright (C) 2017 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/>.  */
+
+#ifdef GDBSERVER
+#include "server.h"
+#else
+#include "defs.h"
+#endif
+
+#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 ());
+}
diff --git a/gdb/common/tdesc.h b/gdb/common/tdesc.h
index cc11651dca..ac6dfdf7bb 100644
--- a/gdb/common/tdesc.h
+++ b/gdb/common/tdesc.h
@@ -18,6 +18,12 @@ 
 #ifndef ARCH_TDESC_H
 #define ARCH_TDESC_H 1
 
+#ifdef GDBSERVER
+#include "server.h"
+#else
+#include "defs.h"
+#endif
+
 struct tdesc_feature;
 struct tdesc_type;
 struct tdesc_type_builtin;
@@ -26,6 +32,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);
 
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 75fbf7ec75..4a54235dae 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -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 \
diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
index 8e68a27c7c..dbf07b2c8d 100644
--- a/gdb/gdbserver/tdesc.c
+++ b/gdb/gdbserver/tdesc.c
@@ -69,10 +69,35 @@  init_target_desc (struct target_desc *tdesc)
 {
   int offset = 0;
 
-  for (reg &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 current_size = tdesc->reg_defs.size ();
+
+      /* Register number will either increase (possibly with gaps) or be
+	 zero.  */
+      gdb_assert (treg->target_regnum == 0
+		  || treg->target_regnum >= current_size);
+
+      tdesc->reg_defs.resize (treg->target_regnum != 0
+				? treg->target_regnum + 1
+				: current_size + 1);
+
+      /* Fill in any blank spaces.  */
+      while (current_size < treg->target_regnum)
+	{
+	  struct reg *reg = &tdesc->reg_defs[current_size];
+	  reg->name = "";
+	  reg->size = 0;
+	  reg->offset = offset;
+	  current_size++;
+	}
+
+      struct reg *reg = &tdesc->reg_defs.back ();
+      reg->name = treg->name.c_str ();
+      reg->size = treg->bitsize;
+      reg->offset = offset;
+      offset += reg->size;
     }
 
   tdesc->registers_size = offset / 8;
@@ -236,26 +261,10 @@  tdesc_create_reg (struct tdesc_feature *feature, const char *name,
 		  int regnum, int save_restore, const char *group,
 		  int bitsize, const char *type)
 {
-  struct target_desc *tdesc = (struct target_desc *) feature;
-  int current_size = tdesc->reg_defs.size ();
-
-  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;
-      current_size++;
-    }
-
-  gdb_assert (regnum == 0
-	      || regnum + 1 == tdesc->reg_defs.size ());
-
-  struct reg *reg = &tdesc->reg_defs.back ();
+  tdesc_reg *reg = new tdesc_reg (feature, name, regnum, save_restore,
+				  group, bitsize, type);
 
-  reg->name = name;
-  reg->size = bitsize;
+  tdesc->registers.emplace_back (reg);
 }
 
 /* See common/tdesc.h.  */
diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
index a62544341c..c64ce13b25 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/target-descriptions.c b/gdb/target-descriptions.c
index 5d34e29a86..3186bf886f 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -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