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>]

* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2016-07-11 08:15:55 +0000]:

> > -  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;
> 
> I would prefer your (very) old proposal here than this. It is more flexible.

I think you are referring to this patch:

  https://sourceware.org/ml/binutils/2016-04/msg00312.html

These two patches, the one linked above, and the original patch in
this thread, do touch the same area of code, however, they are both
solving different problems.  The patch linked above does not resolve
the issue that motivated the patch in this thread, and would not
without pulling in most of the elements from the patch in this thread.

When reviewing both of these patches in order to write this reply, I
realised that I had neglected to include a test with the patch in this
thread.  I've included a revised version of the patch in this mail
that adds a test for the original issue I'm trying to solve.

I do think that the patch linked would be a good improvement to the
ARC assembler, however, that thread ended up blocked.

The tree of strcmps you quoted above is not really new code.  I've
changed what happens in the body of each if block, but the if
conditions themselves are unchanged.

I would be happy to post an update for the linked patch after merging
this patch - this patch fixes (what I believe is) an assembler bug,
while the linked patch is (I believe) a code clean up.  Would that be
something you'd like to see?

I look forward to your feedback,
Thanks,
Andrew

---

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

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.
	* testsuite/gas/arc/nps400-0.s: Add .cpu directive.

bfd/ChangeLog:

	* cpu-arc.c (arc_get_mach): Delete.
---
 bfd/ChangeLog                    |   4 +
 bfd/cpu-arc.c                    |  17 ----
 gas/ChangeLog                    |  30 ++++++
 gas/config/tc-arc.c              | 191 ++++++++++++++++++++-------------------
 gas/testsuite/gas/arc/nps400-0.s |   1 +
 5 files changed, 135 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);
diff --git a/gas/testsuite/gas/arc/nps400-0.s b/gas/testsuite/gas/arc/nps400-0.s
index 2b6cc1d..8fbc1ab 100644
--- a/gas/testsuite/gas/arc/nps400-0.s
+++ b/gas/testsuite/gas/arc/nps400-0.s
@@ -1,2 +1,3 @@
+        .cpu NPS400
         .text
         nop
-- 
2.5.1