This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
PING: Re: [PATCH] gas/arc: Don't rely on bfd list of cpu type for cpu selection
- From: Andrew Burgess <andrew dot burgess at embecosm dot com>
- To: binutils at sourceware dot org
- Cc: noamca at mellanox dot com, Claudiu dot Zissulescu at synopsys dot com, Cupertino dot Miranda at synopsys dot com, anonymous <johnandsara2 at cox dot net>
- Date: Wed, 14 Sep 2016 11:20:10 +0100
- Subject: 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>
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
>