[v2,2/8] Use tdesc_reg in gxdbserver tdesc

This patch commonises tdesc_reg and makes use of it in gdbserver tdesc.

tdesc_create_reg is changed to create a tdesc_reg and place it in
tdesc_feature.

After registers have been parsed, init_target_desc is called. I
have expanded this function so that it now populates the reg_defs
directly from the tdesc_reg registers.

The long term goal is to change the other files within gdbserver to
access member functions instead of reg_defs directly. reg_defs can
then be removed. This is out of the scope of this patch series.

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.

The gdbserver makefile change is a little strange. I had to add using
"..". I think there is a rule that needs changing, but I have no idea
where it is. Note that this is the first file in arch which is common
for all builds. I'm also wondering if tdesc.c/h should be in arch/ ?
Maybe it should be moved to common?

Alan.

2018-01-24  Alan Hayward  <alan.hayward@arm.com>

gdb/
	* Makefile.in: Add arch/tdesc.c
	* arch/tdesc.c: New file.
	* arch/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 arch/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.

On Wed, 24 Jan 2018 09:27:21 +0000
Alan Hayward <Alan.Hayward@arm.com> wrote:

> This patch commonises tdesc_reg and makes use of it in gdbserver tdesc.
> 
> tdesc_create_reg is changed to create a tdesc_reg and place it in
> tdesc_feature.
> 
> After registers have been parsed, init_target_desc is called. I
> have expanded this function so that it now populates the reg_defs
> directly from the tdesc_reg registers.
> 
> The long term goal is to change the other files within gdbserver to
> access member functions instead of reg_defs directly. reg_defs can
> then be removed. This is out of the scope of this patch series.
> 
> 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.
> 
> The gdbserver makefile change is a little strange. I had to add using
> "..". I think there is a rule that needs changing, but I have no idea
> where it is. Note that this is the first file in arch which is common
> for all builds. I'm also wondering if tdesc.c/h should be in arch/ ?
> Maybe it should be moved to common?

In my opinion arch/ should only hold architecture specific code.  So i would
prefer when you move tdesc.c/h to common/.

Philipp

> 
> Alan.
> 
> 2018-01-24  Alan Hayward  <alan.hayward@arm.com>
> 
> gdb/
> 	* Makefile.in: Add arch/tdesc.c
> 	* arch/tdesc.c: New file.
> 	* arch/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 arch/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.
> 
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 364ea7afc66d00d0be44c4369f3ddc924de7b6aa..05ecf64451de4df3a5593d158404d526a71c0b29 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -911,6 +911,7 @@ COMMON_SFILES = \
>  	agent.c \
>  	annotate.c \
>  	arch-utils.c \
> +	arch/tdesc.c \
>  	auto-load.c \
>  	auxv.c \
>  	ax-gdb.c \
> diff --git a/gdb/arch/tdesc.h b/gdb/arch/tdesc.h
> index cc11651dcaa7abe81598b69f509ef34ff0d94dbf..e957822b348182b8cae9809cd21f72974ffe05c5 100644
> --- a/gdb/arch/tdesc.h
> +++ b/gdb/arch/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,99 @@ 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) = 0;
> +  virtual void visit_post (const target_desc *e) = 0;
> +
> +  virtual void visit_pre (const tdesc_feature *e) = 0;
> +  virtual void visit_post (const tdesc_feature *e) = 0;
> +
> +  virtual void visit (const tdesc_type_builtin *e) = 0;
> +  virtual void visit (const tdesc_type_vector *e) = 0;
> +  virtual void visit (const tdesc_type_with_fields *e) = 0;
> +
> +  virtual void visit (const tdesc_reg *e) = 0;
> +};
> +
> +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/arch/tdesc.c b/gdb/arch/tdesc.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..3d84ad8d3fa2e017f4c6f7d137a3be2b58427370
> --- /dev/null
> +++ b/gdb/arch/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 "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/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
> index 3ce086d70f23df445b174c49c489ec8415d7614a..8ae124875dd08f31e8424b18281cb1454ed40edb 100644
> --- a/gdb/gdbserver/Makefile.in
> +++ b/gdb/gdbserver/Makefile.in
> @@ -194,6 +194,7 @@ SFILES = \
>  	$(srcdir)/arch/arm.c \
>  	$(srcdir)/arch/arm-get-next-pcs.c \
>  	$(srcdir)/arch/arm-linux.c \
> +	$(srcdir)/../arch/tdesc.c \
>  	$(srcdir)/common/btrace-common.c \
>  	$(srcdir)/common/buffer.c \
>  	$(srcdir)/common/cleanups.c \
> @@ -232,6 +233,7 @@ TAGFILES = $(SOURCES) ${HFILES} ${ALLPARAM} ${POSSLIBS}
> 
>  OBS = \
>  	agent.o \
> +	arch/tdesc.o \
>  	ax.o \
>  	btrace-common.o \
>  	buffer.o \
> @@ -391,6 +393,7 @@ gdbreplay$(EXEEXT): $(GDBREPLAY_OBS) $(LIBGNU) $(LIBIBERTY)
>  	  $(XM_CLIBS) $(LIBGNU) $(LIBIBERTY)
> 
>  IPA_OBJS = \
> +	arch/tdesc-ipa.o \
>  	ax-ipa.o \
>  	common-utils-ipa.o \
>  	errors-ipa.o \
> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index 705b968b98af9405682507acbcfb41356da62857..cfc1a194def019ecf899e9184385be94de39d08c 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/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
> index 359ab97361c07ff2cbc2c8d5ff392c6233c329ff..5dec636baa6b899490e8c700eea21d363fb79fa3 100644
> --- a/gdb/gdbserver/tdesc.c
> +++ b/gdb/gdbserver/tdesc.c
> @@ -72,9 +72,27 @@ 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)
>      {
> +      /* Fill in any blank spaces.  */
> +      while (tdesc->reg_defs.size () < treg->target_regnum)
> +	{
> +	  struct reg *reg = XCNEW (struct reg);
> +	  reg->name = "";
> +	  reg->size = 0;
> +	  reg->offset = offset;
> +	  tdesc->reg_defs.push_back (reg);
> +	}
> +
> +      gdb_assert (treg->target_regnum == 0
> +		  || treg->target_regnum == tdesc->reg_defs.size ());
> +
> +      struct reg *reg = XCNEW (struct reg);
> +      reg->name = treg->name.c_str ();
> +      reg->size = treg->bitsize;
>        reg->offset = offset;
> +      tdesc->reg_defs.push_back (reg);
>        offset += reg->size;
>      }
> 
> @@ -241,23 +259,10 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,
>  {
>    struct target_desc *tdesc = (struct target_desc *) feature;
> 
> -  while (tdesc->reg_defs.size () < regnum)
> -    {
> -      struct reg *reg = XCNEW (struct reg);
> -
> -      reg->name = "";
> -      reg->size = 0;
> -      tdesc->reg_defs.push_back (reg);
> -    }
> -
> -  gdb_assert (regnum == 0
> -	      || regnum == tdesc->reg_defs.size ());
> -
> -  struct reg *reg = XCNEW (struct reg);
> +  tdesc_reg *reg = new tdesc_reg (feature, name, regnum, save_restore,
> +				  group, bitsize, type);
> 
> -  reg->name = name;
> -  reg->size = bitsize;
> -  tdesc->reg_defs.push_back (reg);
> +  tdesc->registers.emplace_back (reg);
>  }
> 
>  /* See arch/tdesc.h.  */
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index 1b20a12d769718e591dea6df8183c2e9ecfac990..881ef78d2e3adde409e8585deb67c96e2f0594ed 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -38,30 +38,6 @@
>  #include "completer.h"
>  #include "readline/tilde.h" /* tilde_expand */
> 
> -/* The interface to visit different elements of target description.  */
> -
> -class tdesc_element_visitor
> -{
> -public:
> -  virtual void visit_pre (const target_desc *e) = 0;
> -  virtual void visit_post (const target_desc *e) = 0;
> -
> -  virtual void visit_pre (const tdesc_feature *e) = 0;
> -  virtual void visit_post (const tdesc_feature *e) = 0;
> -
> -  virtual void visit (const tdesc_type_builtin *e) = 0;
> -  virtual void visit (const tdesc_type_vector *e) = 0;
> -  virtual void visit (const tdesc_type_with_fields *e) = 0;
> -
> -  virtual void visit (const tdesc_reg *e) = 0;
> -};
> -
> -class tdesc_element
> -{
> -public:
> -  virtual void accept (tdesc_element_visitor &v) const = 0;
> -};
> -
>  /* Types.  */
> 
>  struct property
> @@ -74,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
>