Re: [RFC 4/7] Share code in initialize_tdesc_ functions

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

Hi Yao,

On Thu, 11 May 2017 21:55:04 +0100
Yao Qi <qiyaoltc@gmail.com> wrote:

> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index 754f15b..a81ae0d 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c

[...]

> @@ -54,7 +57,10 @@ typedef struct tdesc_reg
>    char *name;
>  
>    /* The register number used by this target to refer to this
> -     register.  This is used for remote p/P packets and to determine
> +     register.  Positive number means it is got by increasing
> +     the previous register number by one.  Negative number means
> +     it is got from "regnum" attribute in the xml file.
> +     This is used for remote p/P packets and to determine
>       the ordering of registers in the remote g/G packets.  */
>    long target_regnum;

I'm not really sure if I like your idea with positive/negative regnums.  It
could easily lead to hard to find bugs because someone forgot a '-' somewhere.
I would prefer an extra field like 

bool explicit_regnum;
or
bool automatic_regnum;

[...]

> +/* Return the unique name of FEATURE.  The uniqueness means different
> +   target description features have different values.  This is used
> +   to differentiate different features with the same name.  */
> +
> +static std::string
> +tdesc_feature_unique_name (const struct tdesc_feature* feature)
> +{
> +  std::string name = std::string (feature->name);
> +
> +  if (name.compare ("org.gnu.gdb.arm.m-profile") == 0)
> +    {
> +      struct tdesc_reg *reg;
> +
> +      for (int ix = 0;
> +	   VEC_iterate (tdesc_reg_p, feature->registers, ix, reg);
> +	   ix++)
> +	if (reg->type != NULL && strcmp (reg->type, "arm_fpa_ext") == 0)
> +	  {
> +	    name.append ("_fpa");
> +	    break;
> +	  }
> +    }
> +  else if (name.compare ("org.gnu.gdb.arm.vfp") == 0)
> +    {
> +      name.append ("_");
> +      name.append (std::to_string (VEC_length (tdesc_reg_p,
> +					       feature->registers) - 1));
> +    }
> +  else if (name.compare ("org.gnu.gdb.i386.core") == 0)
> +    {
> +      struct tdesc_reg *reg = NULL;
> +
> +      for (int ix = 0;
> +	   VEC_iterate (tdesc_reg_p, feature->registers, ix, reg);
> +	   ix++)
> +	{
> +	  if (strcmp (reg->name, "ebp") == 0
> +	      || strcmp (reg->name, "rbp") == 0)
> +	    break;
> +	}
> +
> +      name.append ("_");
> +      if (strcmp (reg->name, "ebp") == 0)
> +	name.append ("i386");
> +      else
> +	{
> +	  if (strcmp (reg->type, "data_ptr") == 0)
> +	    name.append ("amd64");
> +	  else
> +	    name.append ("x32");
> +	}
> +    }
> +  else if (name.compare ("org.gnu.gdb.power.core") == 0)
> +    {
> +      struct tdesc_reg *reg = NULL;
> +      struct tdesc_reg *reg_mq = NULL;
> +      struct tdesc_reg *reg_r0 = NULL;
> +
> +      /* Four variants.  */
> +      for (int ix = 0;
> +	   VEC_iterate (tdesc_reg_p, feature->registers, ix, reg);
> +	   ix++)
> +	{
> +	  if (strcmp (reg->name, "r0") == 0)
> +	    reg_r0 = reg;
> +	  if (strcmp (reg->name, "mq") == 0)
> +	    reg_mq = reg;
> +
> +	  if (reg_r0 != NULL && reg_mq != NULL)
> +	    break;
> +	}
> +
> +      name.append ("_");
> +      if (reg_mq == NULL)
> +	name.append (std::to_string (reg_r0->bitsize));
> +      else
> +	{
> +	  if (reg_mq->target_regnum == -124)
> +	    name.append ("601");
> +	  else
> +	    name.append ("rs6000");
> +	}
> +    }
> +  else if (name.compare ("org.gnu.gdb.power.fpu") == 0)
> +    {
> +      struct tdesc_reg *reg = NULL;
> +
> +      /* Three variants.  */
> +      for (int ix = 0;
> +	   VEC_iterate (tdesc_reg_p, feature->registers, ix, reg);
> +	   ix++)
> +	{
> +	  if (strcmp (reg->name, "fpscr") == 0)
> +	    break;
> +	}
> +
> +      name.append ("_");
> +
> +      if (reg->target_regnum == -71)
> +	name.append ("rs6000");
> +      else
> +	name.append (std::to_string (reg->bitsize));
> +    }
> +  else if (name.compare ("org.gnu.gdb.power.linux") == 0)
> +    {
> +      struct tdesc_reg *reg = NULL;
> +
> +      for (int ix = 0;
> +	   VEC_iterate (tdesc_reg_p, feature->registers, ix, reg);
> +	   ix++)
> +	{
> +	  if (strcmp (reg->name, "orig_r3") == 0)
> +	    break;
> +	}
> +
> +      name.append (std::to_string (reg->bitsize));
> +    }
> +  else if (name.compare ("org.gnu.gdb.s390.linux") == 0)
> +    {
> +      struct tdesc_reg *reg = NULL;
> +      struct tdesc_reg *reg_orig_r2 = NULL;
> +      struct tdesc_reg *reg_sc = NULL;
> +      struct tdesc_reg *reg_lb = NULL;
> +
> +      /* Six variants.  */
> +      for (int ix = 0;
> +	   VEC_iterate (tdesc_reg_p, feature->registers, ix, reg);
> +	   ix++)
> +	{
> +	  if (strcmp (reg->name, "orig_r2") == 0)
> +	    reg_orig_r2 = reg;
> +	  if (strcmp (reg->name, "last_break") == 0)
> +	    reg_lb = reg;
> +	  if (strcmp (reg->name, "system_call") == 0)
> +	    reg_sc = reg;
> +
> +	  if (reg_orig_r2 != NULL && reg_sc != NULL && reg_lb != NULL)
> +	    break;
> +	}
> +
> +      name.append ("_");
> +      name.append (std::to_string (reg_orig_r2->bitsize));
> +
> +      if (reg_lb != NULL)
> +	{
> +	  name.append ("_");
> +
> +	  if (reg_sc == NULL)
> +	    name.append ("v1");
> +	  else
> +	    name.append ("v2");
> +	}
> +    }
> +  else if (name.compare ("org.gnu.gdb.s390.core") == 0)
> +    {
> +      struct tdesc_reg *reg = NULL;
> +      struct tdesc_reg *reg_r0 = NULL;
> +      struct tdesc_reg *reg_r0l = NULL;
> +
> +      /* Six variants.  */
> +      for (int ix = 0;
> +	   VEC_iterate (tdesc_reg_p, feature->registers, ix, reg);
> +	   ix++)
> +	{
> +	  if (strcmp (reg->name, "r0") == 0)
> +	    reg_r0 = reg;
> +	  if (strcmp (reg->name, "r0l") == 0)
> +	    reg_r0l = reg;
> +
> +	  if (reg_r0 != NULL && reg_r0l != NULL)
> +	    break;
> +	}
> +
> +      name.append ("_");
> +      if (reg_r0l != NULL)
> +	name.append ("s64");
> +      else
> +	name.append (std::to_string (reg_r0->bitsize));
> +    }
> +  else if (name.compare ("org.gnu.gdb.mips.cpu") == 0
> +	   || name.compare ("org.gnu.gdb.mips.cp0") == 0
> +	   || name.compare ("org.gnu.gdb.mips.fpu") == 0
> +	   || name.compare ("org.gnu.gdb.mips.dsp") == 0
> +	   || name.compare ("org.gnu.gdb.mips.linux") == 0)
> +    {
> +      struct tdesc_reg *reg = VEC_index (tdesc_reg_p, feature->registers, 0);
> +
> +      name.append (std::to_string (reg->bitsize));
> +    }
> +
> +  std::replace (name.begin (), name.end (), '.', '_');
> +  std::replace (name.begin (), name.end (), '-', '_');
> +
> +  return name;
> +}
> +

This function is actually the part I like least of your implementation.  Its
way to long and barely readable.  The way I understand, it is needed to create
unique macro and function names.  So why don't you simply use the filename of
the XML file where the feature is defined?  It already is unique.  Or use an
gdbarch hook so every architecture can decide for itself how to name them?

Philipp