PING: Re: [PATCH] gas/arc: Don't rely on bfd list of cpu type for cpu selection

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

Ping.

I did receive feedback from a couple of sources (all CC'd) to which I
replied.  Since then I've had no follow up.

Given I think that my response addresses the feedback I'd like to push
this patch again.

If anyone feels I've not addressed their feedback, then I apologise,
please feel free to restate your objections and I'll try to address
any points raised.

This patch might need a rebase before merging, but I'm keen to hear
any high level objections first.

Many thanks,
Andrew



* Andrew Burgess <andrew.burgess@embecosm.com> [2016-07-08 18:21:07 +0100]:

> In the ARC assembler, when a cpu type is specified using the .cpu
> directive, we rely on the bfd list of arc machine types in order to
> validate the cpu name passed in.
> 
> This validation is only used in order to check that the cpu type passed
> to the .cpu directive matches any machine type selected earlier on the
> command line.  Once that initial check has passed a full check is
> performed using the assemblers internal list of know cpu types.
> 
> The problem is that the assembler knows about more cpu types than bfd,
> some cpu types known by the assembler are actually aliases for a base
> cpu type plus a specific set of assembler extensions.  One such example
> is NPS400, though more could be added later.
> 
> This commit removes the need for the assembler to use the bfd list of
> machine types for validation.  Instead the error checking, to ensure
> that any value passed to a '.cpu' directive matches any earlier command
> line selection, is moved into the function arc_select_cpu.
> 
> I have taken the opportunity to bundle the 4 separate static globals
> that describe the currently selected machine type into a single
> structure (called selected_cpu).
> 
> gas/ChangeLog:
> 
> 	* config/tc-arc.c (arc_target): Delete.
> 	(arc_target_name): Delete.
> 	(arc_features): Delete.
> 	(arc_mach_type): Delete.
> 	(mach_type_specified_p): Delete.
> 	(enum mach_selection_type): New enum.
> 	(mach_selection_mode): New static global.
> 	(selected_cpu): New static global.
> 	(arc_eflag): Rename to ...
> 	(arc_initial_eflag): ...this, and make const.
> 	(arc_select_cpu): Update comment, new parameter, check how
> 	previous machine type selection was made, and record this
> 	selection.  Use selected_cpu instead of old globals.
> 	(arc_option): Remove use of arc_get_mach, instead use
> 	arc_select_cpu to validate machine type selection.  Use
> 	selected_cpu over old globals.
> 	(allocate_tok): Use selected_cpu over old globals.
> 	(find_opcode_match): Likewise.
> 	(assemble_tokens): Likewise.
> 	(arc_cons_fix_new): Likewise.
> 	(arc_extinsn): Likewise.
> 	(arc_extcorereg): Likewise.
> 	(md_begin): Update default machine type selection, use
> 	selected_cpu over old globals.
> 	(md_parse_option): Update machine type selection option handling,
> 	use selected_cpu over old globals.
> 
> bfd/ChangeLog:
> 
> 	* cpu-arc.c (arc_get_mach): Delete.
> ---
>  bfd/ChangeLog       |   4 ++
>  bfd/cpu-arc.c       |  17 -----
>  gas/ChangeLog       |  29 ++++++++
>  gas/config/tc-arc.c | 191 +++++++++++++++++++++++++++-------------------------
>  4 files changed, 133 insertions(+), 108 deletions(-)
> 
> diff --git a/bfd/cpu-arc.c b/bfd/cpu-arc.c
> index 07a052b..e63f3c1 100644
> --- a/bfd/cpu-arc.c
> +++ b/bfd/cpu-arc.c
> @@ -54,20 +54,3 @@ static const bfd_arch_info_type arch_info_struct[] =
>  
>  const bfd_arch_info_type bfd_arc_arch =
>    ARC (bfd_mach_arc_arc600, "ARC600", TRUE, &arch_info_struct[0]);
> -
> -/* Utility routines.  */
> -
> -/* Given cpu type NAME, return its bfd_mach_arc_xxx value.
> -   Returns -1 if not found.  */
> -int arc_get_mach (char *name);
> -
> -int
> -arc_get_mach (char *name)
> -{
> -  const bfd_arch_info_type *p;
> -
> -  for (p = &bfd_arc_arch; p != NULL; p = p->next)
> -    if (strcmp (name, p->printable_name) == 0)
> -      return p->mach;
> -  return -1;
> -}
> diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
> index 2046604..545c7d8 100644
> --- a/gas/config/tc-arc.c
> +++ b/gas/config/tc-arc.c
> @@ -400,16 +400,19 @@ static void assemble_insn
>    (const struct arc_opcode *, const expressionS *, int,
>     const struct arc_flags *, int, struct arc_insn *);
>  
> -/* The cpu for which we are generating code.  */
> -static unsigned arc_target;
> -static const char *arc_target_name;
> -static unsigned arc_features;
> -
> -/* The default architecture.  */
> -static int arc_mach_type;
> -
> -/* TRUE if the cpu type has been explicitly specified.  */
> -static bfd_boolean mach_type_specified_p = FALSE;
> +/* The selection of the machine type can come from different sources.  This
> +   enum is used to track how the selection was made in order to perform
> +   error checks.  */
> +enum mach_selection_type
> +  {
> +    MACH_SELECTION_NONE,
> +    MACH_SELECTION_FROM_DEFAULT,
> +    MACH_SELECTION_FROM_CPU_DIRECTIVE,
> +    MACH_SELECTION_FROM_COMMAND_LINE
> +  };
> +
> +/* How the current machine type was selected.  */
> +static enum mach_selection_type mach_selection_mode = MACH_SELECTION_NONE;
>  
>  /* The hash table of instruction opcodes.  */
>  static struct hash_control *arc_opcode_hash;
> @@ -444,6 +447,9 @@ static const struct cpu_type
>    { 0, 0, 0, 0, 0 }
>  };
>  
> +/* Information about the cpu/variant we're assembling for.  */
> +static struct cpu_type selected_cpu;
> +
>  /* Used by the arc_reloc_op table.  Order is important.  */
>  #define O_gotoff  O_md1     /* @gotoff relocation.  */
>  #define O_gotpc   O_md2     /* @gotpc relocation.  */
> @@ -634,7 +640,7 @@ const struct arc_relaxable_ins arc_relaxable_insns[] =
>  const unsigned arc_num_relaxable_ins = ARRAY_SIZE (arc_relaxable_insns);
>  
>  /* Flags to set in the elf header.  */
> -static flagword arc_eflag = 0x00;
> +static const flagword arc_initial_eflag = 0x00;
>  
>  /* Pre-defined "_GLOBAL_OFFSET_TABLE_".  */
>  symbolS * GOT_symbol = 0;
> @@ -750,22 +756,46 @@ md_number_to_chars_midend (char *buf, valueT val, int n)
>  }
>  
>  /* Select an appropriate entry from CPU_TYPES based on ARG and initialise
> -   the relevant static global variables.  */
> +   the relevant static global variables.  Parameter SEL describes where
> +   this selection originated from.  */
>  
>  static void
> -arc_select_cpu (const char *arg)
> +arc_select_cpu (const char *arg, enum mach_selection_type sel)
>  {
>    int cpu_flags = 0;
>    int i;
>  
> +  /* We should only set a default if we've not made a selection from some
> +     other source.  */
> +  gas_assert (sel != MACH_SELECTION_FROM_DEFAULT
> +              || mach_selection_mode == MACH_SELECTION_NONE);
> +
> +  /* Look for a matching entry in CPU_TYPES array.  */
>    for (i = 0; cpu_types[i].name; ++i)
>      {
>        if (!strcasecmp (cpu_types[i].name, arg))
>          {
> -          arc_target = cpu_types[i].flags;
> -          arc_target_name = cpu_types[i].name;
> -          arc_features = cpu_types[i].features;
> -          arc_mach_type = cpu_types[i].mach;
> +          /* If a previous selection was made on the command line, then we
> +             allow later selections on the command line to override earlier
> +             ones.  However, a selection from a '.cpu NAME' directive must
> +             match the command line selection, or we give a warning.  */
> +          if (mach_selection_mode == MACH_SELECTION_FROM_COMMAND_LINE)
> +            {
> +              gas_assert (sel == MACH_SELECTION_FROM_COMMAND_LINE
> +                          || sel == MACH_SELECTION_FROM_CPU_DIRECTIVE);
> +              if (sel == MACH_SELECTION_FROM_CPU_DIRECTIVE
> +                  && selected_cpu.mach != cpu_types[i].mach)
> +                {
> +                  as_warn (_("Command-line value overrides \".cpu\" directive"));
> +                  return;
> +                }
> +            }
> +
> +          /* Initialise static global data about selected machine type.  */
> +          selected_cpu.flags = cpu_types[i].flags;
> +          selected_cpu.name = cpu_types[i].name;
> +          selected_cpu.features = cpu_types[i].features;
> +          selected_cpu.mach = cpu_types[i].mach;
>            cpu_flags = cpu_types[i].eflags;
>            break;
>          }
> @@ -774,7 +804,8 @@ arc_select_cpu (const char *arg)
>    if (!cpu_types[i].name)
>      as_fatal (_("unknown architecture: %s\n"), arg);
>    gas_assert (cpu_flags != 0);
> -  arc_eflag = (arc_eflag & ~EF_ARC_MACH_MSK) | cpu_flags;
> +  selected_cpu.eflags = (arc_initial_eflag & ~EF_ARC_MACH_MSK) | cpu_flags;
> +  mach_selection_mode = sel;
>  }
>  
>  /* Here ends all the ARCompact extension instruction assembling
> @@ -877,62 +908,41 @@ arc_lcomm (int ignore)
>  static void
>  arc_option (int ignore ATTRIBUTE_UNUSED)
>  {
> -  int mach = -1;
>    char c;
>    char *cpu;
> +  const char *cpu_name;
>  
>    c = get_symbol_name (&cpu);
> -  mach = arc_get_mach (cpu);
>  
> -  if (mach == -1)
> -    goto bad_cpu;
> +  if ((!strcmp ("ARC600", cpu))
> +      || (!strcmp ("ARC601", cpu))
> +      || (!strcmp ("A6", cpu)))
> +    cpu_name = "arc600";
> +  else if ((!strcmp ("ARC700", cpu))
> +           || (!strcmp ("A7", cpu)))
> +    cpu_name = "arc700";
> +  else if (!strcmp ("EM", cpu))
> +    cpu_name = "arcem";
> +  else if (!strcmp ("HS", cpu))
> +    cpu_name = "archs";
> +  else if (!strcmp ("NPS400", cpu))
> +    cpu_name = "nps400";
> +  else
> +    cpu_name = NULL;
>  
> -  if (!mach_type_specified_p)
> -    {
> -      if ((!strcmp ("ARC600", cpu))
> -	  || (!strcmp ("ARC601", cpu))
> -	  || (!strcmp ("A6", cpu)))
> -	{
> -	  md_parse_option (OPTION_MCPU, "arc600");
> -	}
> -      else if ((!strcmp ("ARC700", cpu))
> -	       || (!strcmp ("A7", cpu)))
> -	{
> -	  md_parse_option (OPTION_MCPU, "arc700");
> -	}
> -      else if (!strcmp ("EM", cpu))
> -	{
> -	  md_parse_option (OPTION_MCPU, "arcem");
> -	}
> -      else if (!strcmp ("HS", cpu))
> -	{
> -	  md_parse_option (OPTION_MCPU, "archs");
> -	}
> -      else if (!strcmp ("NPS400", cpu))
> -	{
> -	  md_parse_option (OPTION_MCPU, "nps400");
> -	}
> -      else
> -	as_fatal (_("could not find the architecture"));
> +  if (cpu_name != NULL)
> +    arc_select_cpu (cpu_name, MACH_SELECTION_FROM_CPU_DIRECTIVE);
> +  else
> +    as_fatal (_("invalid architecture `%s' in .cpu directive"), cpu);
>  
> -      if (!bfd_set_arch_mach (stdoutput, bfd_arch_arc, mach))
> -	as_fatal (_("could not set architecture and machine"));
> +  if (!bfd_set_arch_mach (stdoutput, bfd_arch_arc, selected_cpu.mach))
> +    as_fatal (_("could not set architecture and machine"));
>  
> -      /* Set elf header flags.  */
> -      bfd_set_private_flags (stdoutput, arc_eflag);
> -    }
> -  else
> -    if (arc_mach_type != mach)
> -      as_warn (_("Command-line value overrides \".cpu\" directive"));
> +  /* Set elf header flags.  */
> +  bfd_set_private_flags (stdoutput, selected_cpu.eflags);
>  
>    restore_line_pointer (c);
>    demand_empty_rest_of_line ();
> -  return;
> -
> - bad_cpu:
> -  restore_line_pointer (c);
> -  as_bad (_("invalid identifier for \".cpu\""));
> -  ignore_rest_of_line ();
>  }
>  
>  /* Smartly print an expression.  */
> @@ -1539,19 +1549,19 @@ allocate_tok (expressionS *tok, int ntok, int cidx)
>  static bfd_boolean
>  check_cpu_feature (insn_subclass_t sc)
>  {
> -  if (is_code_density_p (sc) && !(arc_features & ARC_CD))
> +  if (is_code_density_p (sc) && !(selected_cpu.features & ARC_CD))
>      return FALSE;
>  
> -  if (is_spfp_p (sc) && !(arc_features & ARC_SPFP))
> +  if (is_spfp_p (sc) && !(selected_cpu.features & ARC_SPFP))
>      return FALSE;
>  
> -  if (is_dpfp_p (sc) && !(arc_features & ARC_DPFP))
> +  if (is_dpfp_p (sc) && !(selected_cpu.features & ARC_DPFP))
>      return FALSE;
>  
> -  if (is_fpuda_p (sc) && !(arc_features & ARC_FPUDA))
> +  if (is_fpuda_p (sc) && !(selected_cpu.features & ARC_FPUDA))
>      return FALSE;
>  
> -  if (is_nps400_p (sc) && !(arc_features & ARC_NPS400))
> +  if (is_nps400_p (sc) && !(selected_cpu.features & ARC_NPS400))
>      return FALSE;
>  
>    return TRUE;
> @@ -1678,7 +1688,7 @@ find_opcode_match (const struct arc_opcode_hash_entry *entry,
>  
>        /* Don't match opcodes that don't exist on this
>  	 architecture.  */
> -      if (!(opcode->cpu & arc_target))
> +      if (!(opcode->cpu & selected_cpu.flags))
>  	goto match_failed;
>  
>        if (!check_cpu_feature (opcode->subclass))
> @@ -2257,7 +2267,7 @@ find_special_case_long_opcode (const char *opname,
>  
>        opcode = &arc_long_opcodes[i].base_opcode;
>  
> -      if (!(opcode->cpu & arc_target))
> +      if (!(opcode->cpu & selected_cpu.flags))
>          continue;
>  
>        if (!check_cpu_feature (opcode->subclass))
> @@ -2376,7 +2386,7 @@ assemble_tokens (const char *opname,
>  	as_bad (_("inappropriate arguments for opcode '%s'"), opname);
>        else
>  	as_bad (_("opcode '%s' not supported for target %s"), opname,
> -		arc_target_name);
> +		selected_cpu.name);
>      }
>    else
>      as_bad (_("unknown opcode '%s'"), opname);
> @@ -2469,17 +2479,17 @@ md_begin (void)
>  {
>    const struct arc_opcode *opcode = arc_opcodes;
>  
> -  if (!mach_type_specified_p)
> -    arc_select_cpu (TARGET_WITH_CPU);
> +  if (mach_selection_mode == MACH_SELECTION_NONE)
> +    arc_select_cpu (TARGET_WITH_CPU, MACH_SELECTION_FROM_DEFAULT);
>  
>    /* The endianness can be chosen "at the factory".  */
>    target_big_endian = byte_order == BIG_ENDIAN;
>  
> -  if (!bfd_set_arch_mach (stdoutput, bfd_arch_arc, arc_mach_type))
> +  if (!bfd_set_arch_mach (stdoutput, bfd_arch_arc, selected_cpu.mach))
>      as_warn (_("could not set architecture and machine"));
>  
>    /* Set elf header flags.  */
> -  bfd_set_private_flags (stdoutput, arc_eflag);
> +  bfd_set_private_flags (stdoutput, selected_cpu.eflags);
>  
>    /* Set up a hash table for the instructions.  */
>    arc_opcode_hash = hash_new ();
> @@ -2563,7 +2573,7 @@ md_begin (void)
>      {
>        const char *retval;
>  
> -      if (!(auxr->cpu & arc_target))
> +      if (!(auxr->cpu & selected_cpu.flags))
>  	continue;
>  
>        if ((auxr->subclass != NONE)
> @@ -3323,8 +3333,7 @@ md_parse_option (int c, const char *arg ATTRIBUTE_UNUSED)
>  
>      case OPTION_MCPU:
>        {
> -        arc_select_cpu (arg);
> -        mach_type_specified_p = TRUE;
> +        arc_select_cpu (arg, MACH_SELECTION_FROM_COMMAND_LINE);
>  	break;
>        }
>  
> @@ -3340,8 +3349,8 @@ md_parse_option (int c, const char *arg ATTRIBUTE_UNUSED)
>  
>      case OPTION_CD:
>        /* This option has an effect only on ARC EM.  */
> -      if (arc_target & ARC_OPCODE_ARCv2EM)
> -	arc_features |= ARC_CD;
> +      if (selected_cpu.flags & ARC_OPCODE_ARCv2EM)
> +	selected_cpu.features |= ARC_CD;
>        else
>  	as_warn (_("Code density option invalid for selected CPU"));
>        break;
> @@ -3351,21 +3360,21 @@ md_parse_option (int c, const char *arg ATTRIBUTE_UNUSED)
>        break;
>  
>      case OPTION_NPS400:
> -      arc_features |= ARC_NPS400;
> +      selected_cpu.features |= ARC_NPS400;
>        break;
>  
>      case OPTION_SPFP:
> -      arc_features |= ARC_SPFP;
> +      selected_cpu.features |= ARC_SPFP;
>        break;
>  
>      case OPTION_DPFP:
> -      arc_features |= ARC_DPFP;
> +      selected_cpu.features |= ARC_DPFP;
>        break;
>  
>      case OPTION_FPUDA:
>        /* This option has an effect only on ARC EM.  */
> -      if (arc_target & ARC_OPCODE_ARCv2EM)
> -	arc_features |= ARC_FPUDA;
> +      if (selected_cpu.flags & ARC_OPCODE_ARCv2EM)
> +	selected_cpu.features |= ARC_FPUDA;
>        else
>  	as_warn (_("FPUDA invalid for selected CPU"));
>        break;
> @@ -4097,10 +4106,10 @@ arc_cons_fix_new (fragS *frag,
>  static void
>  check_zol (symbolS *s)
>  {
> -  switch (arc_mach_type)
> +  switch (selected_cpu.mach)
>      {
>      case bfd_mach_arc_arcv2:
> -      if (arc_target & ARC_OPCODE_ARCv2EM)
> +      if (selected_cpu.flags & ARC_OPCODE_ARCv2EM)
>  	return;
>  
>        if (is_br_jmp_insn_p (arc_last_insns[0].opcode)
> @@ -4425,8 +4434,8 @@ arc_extinsn (int ignore ATTRIBUTE_UNUSED)
>  
>    /* Check the opcode ranges.  */
>    moplow = 0x05;
> -  mophigh = (arc_target & (ARC_OPCODE_ARCv2EM
> -			   | ARC_OPCODE_ARCv2HS)) ? 0x07 : 0x0a;
> +  mophigh = (selected_cpu.flags & (ARC_OPCODE_ARCv2EM
> +                                   | ARC_OPCODE_ARCv2HS)) ? 0x07 : 0x0a;
>  
>    if ((einsn.major > mophigh) || (einsn.major < moplow))
>      as_fatal (_("major opcode not in range [0x%02x - 0x%02x]"), moplow, mophigh);
> @@ -4451,7 +4460,7 @@ arc_extinsn (int ignore ATTRIBUTE_UNUSED)
>        break;
>      }
>  
> -  arc_ext_opcodes = arcExtMap_genOpcode (&einsn, arc_target, &errmsg);
> +  arc_ext_opcodes = arcExtMap_genOpcode (&einsn, selected_cpu.flags, &errmsg);
>    if (arc_ext_opcodes == NULL)
>      {
>        if (errmsg)
> @@ -4675,7 +4684,7 @@ arc_extcorereg (int opertype)
>        /* Auxiliary register.  */
>        auxr = XNEW (struct arc_aux_reg);
>        auxr->name = ereg.name;
> -      auxr->cpu = arc_target;
> +      auxr->cpu = selected_cpu.flags;
>        auxr->subclass = NONE;
>        auxr->address = ereg.number;
>        retval = hash_insert (arc_aux_hash, auxr->name, (void *) auxr);
> -- 
> 2.6.4
>