Re: [PATCH v5 1/8] Commonise tdesc_reg

[Simon Marchi <simark@simark.ca>]

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