Re: [PATCH v3 2/8] Commonise tdesc_reg

[Philipp Rudo <prudo@linux.vnet.ibm.com>]

On Thu, 1 Mar 2018 11:39:36 +0000
Alan Hayward <Alan.Hayward@arm.com> wrote:

[...]

> diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
> index e50a848e2f9f280a84ab139cfce4d1f17bd05884..806e87da185623da29d39d9fd74c8722f795e086 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;
>      }

I think it would make sense here to also change the reg_defs vector to hold the
struct reg directly instead of a pointer to it.  Then the for-loop (should)
read like this

  gdb_assert (treg->target_regnum == 0
	      || treg->target_regnum >= tdesc->reg_defs.size ());
  tdesc->reg_defs.resize (treg->target_regnum);
  struct reg *reg = &tdesc->reg_defs.back ()
  reg->name = treg->name.c_str ();
  reg->size = treg->bitsize;
  reg->offset = offset;
  offset += reg->size;

With this you don't have to allocate so many small chunks. Furthermore in
combination with copy_target_description this construct smells like a memory
leak to me.

Thanks
Philipp

 
> @@ -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 common/tdesc.h.  */
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index 5d34e29a867f8d39cd0451eb48cbf39614c9acba..3186bf886fc5f1b62e0ff77a4e5a2bc0fe52b250 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
>