This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: PING: Re: [PATCH] gas/arc: Don't rely on bfd list of cpu type for cpu selection
- From: Claudiu Zissulescu <Claudiu dot Zissulescu at synopsys dot com>
- To: Andrew Burgess <andrew dot burgess at embecosm dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Cc: "noamca at mellanox dot com" <noamca at mellanox dot com>, "Cupertino dot Miranda at synopsys dot com" <Cupertino dot Miranda at synopsys dot com>, anonymous <johnandsara2 at cox dot net>
- Date: Wed, 14 Sep 2016 10:25:01 +0000
- Subject: RE: PING: Re: [PATCH] gas/arc: Don't rely on bfd list of cpu type for cpu selection
- Authentication-results: sourceware.org; auth=none
- References: <1467998467-16904-1-git-send-email-andrew.burgess@embecosm.com> <20160914102010.GB31794@embecosm.com>
I see no issue with your patch, though I haven't tested with the latest sources.
Best,
Claudiu
> -----Original Message-----
> From: Andrew Burgess [mailto:andrew.burgess@embecosm.com]
> Sent: Wednesday, September 14, 2016 12:20 PM
> To: binutils@sourceware.org
> Cc: noamca@mellanox.com; Claudiu.Zissulescu@synopsys.com;
> Cupertino.Miranda@synopsys.com; anonymous <johnandsara2@cox.net>
> Subject: PING: Re: [PATCH] gas/arc: Don't rely on bfd list of cpu type for cpu
> selection
>
> 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
> >