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

[Pedro Alves <palves at redhat dot com>]

On 05/16/2017 01:02 PM, Philipp Rudo wrote:
> 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?

Agreed.  I was reading the patch and thinking how there must be a better
way to handle this.

Thanks,
Pedro Alves