[PATCH 1/2] Add support for O32 FPXX ABI
Matthew Fortune
Matthew.Fortune@imgtec.com
Tue May 6 22:51:00 GMT 2014
Richard Sandiford <rdsandiford@googlemail.com> writes:
> Thanks for all the work, mostly looks good to me FWIW.
>
> Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> > +/* Return true if the given ELF header flags describe a 32-bit binary. */
> > +
> > +static bfd_boolean
> > +mips_32bit_flags_p (flagword flags)
> > +{
> > + return ((flags & EF_MIPS_32BITMODE) != 0
> > + || (flags & EF_MIPS_ABI) == E_MIPS_ABI_O32
> > + || (flags & EF_MIPS_ABI) == E_MIPS_ABI_EABI32
> > + || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_1
> > + || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_2
> > + || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_32
> > + || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_32R2);
> > +}
>
> OK, this is preexisting code, but I'm not sure it's right.
> 32bitness should be taken purely from the ABI, not the architecture,
> otherwise we could treat 32-bit MIPS64r2 code differently from MIPS32r2
> code.
>
> Since o32 was the original (with no flags), I think we should have
> mips_64bit_flags_p instead.
>
> OTOH that should be done separately from your patch, so never mind.
I was uneasy about this code as well. I think I concluded it was right
because the 32BITMODE flag gets set if a 32-bit ABI is used with a
64-bit architecture. I can't hold all the intricacies of the flags in
my head so have to read all the comments each time to check.
> > +/* Infer the content of the ABI flags based on the elf header. */
> > +
> > +static void
> > +infer_mips_abiflags (bfd *abfd, Elf_Internal_ABIFlags_v0* abiflags)
> > +{
> > + obj_attribute *in_attr;
> > +
>
> Would feel more comfortable with a clearing memset here, and leave
> the explicit 0s out.
Almost did that :-) No idea why I didn't in the end.
> > + abiflags->version = 0;
> > +
> > + update_mips_abiflags_isa (abfd, abiflags);
> > +
> > + if (mips_32bit_flags_p (elf_elfheader (abfd)->e_flags))
> > + abiflags->gpr_size = AFL_REG_32;
> > + else
> > + abiflags->gpr_size = AFL_REG_64;
> > +
> > + abiflags->cpr1_size = AFL_REG_NONE;
> > +
> > + in_attr = elf_known_obj_attributes (abfd)[OBJ_ATTR_GNU];
> > + abiflags->fp_abi = in_attr[Tag_GNU_MIPS_ABI_FP].i;
> > +
> > + if (abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_SINGLE
> > + || abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_XX
> > + || (abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_DOUBLE
> > + && abiflags->gpr_size == AFL_REG_32))
> > + abiflags->cpr1_size = AFL_REG_32;
> > + else if (abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_DOUBLE
> > + || abiflags->fp_abi == Val_GNU_MIPS_ABI_FP_64)
> > + abiflags->cpr1_size = AFL_REG_64;
> > +
> > + abiflags->cpr2_size = AFL_REG_NONE;
> > +
> > + abiflags->isa_ext = 0;
>
> This can be set from EF_MIPS_MACH, at least to some extent.
Yes.
> > @@ -13665,12 +13887,47 @@ _bfd_mips_elf_final_link (bfd *abfd, struct bfd_link_info
> *info)
> >
> > /* Go through the sections and collect the .reginfo and .mdebug
> > information. */
> > + abiflags_sec = NULL;
> > reginfo_sec = NULL;
> > mdebug_sec = NULL;
> > gptab_data_sec = NULL;
> > gptab_bss_sec = NULL;
> > for (o = abfd->sections; o != NULL; o = o->next)
> > {
> > + if (strcmp (o->name, ".MIPS.abiflags") == 0)
> > + {
> > + /* We have found the .MIPS.abiflags section in the output file.
> > + Look through all the link_orders comprising it and remove them.
> > + The data is merged in _bfd_mips_elf_merge_private_bfd_data. */
> > + for (p = o->map_head.link_order; p != NULL; p = p->next)
> > + {
> > + asection *input_section;
> > +
> > + if (p->type != bfd_indirect_link_order)
> > + {
> > + if (p->type == bfd_data_link_order)
> > + continue;
> > + abort ();
> > + }
> > +
> > + input_section = p->u.indirect.section;
> > +
> > + /* Hack: reset the SEC_HAS_CONTENTS flag so that
> > + elf_link_input_bfd ignores this section. */
> > + input_section->flags &= ~SEC_HAS_CONTENTS;
> > + }
> > +
> > + /* Size has been set in _bfd_mips_elf_always_size_sections. */
> > + BFD_ASSERT(o->size == sizeof (Elf_External_ABIFlags_v0));
> > +
> > + /* Skip this section later on (I don't think this currently
> > + matters, but someday it might). */
> > + o->map_head.link_order = NULL;
> > +
> > + abiflags_sec = o;
> > +
> > + }
>
> Excess blank line.
>
> > @@ -14155,6 +14412,24 @@ _bfd_mips_elf_final_link (bfd *abfd, struct bfd_link_info
> *info)
> >
> > /* Now write out the computed sections. */
> >
> > + if (abiflags_sec != NULL)
> > + {
> > + Elf_External_ABIFlags_v0 ext;
> > + Elf_Internal_ABIFlags_v0 *abiflags;
> > +
> > + abiflags = &mips_elf_tdata (abfd)->abiflags;
> > +
> > + /* Set up the abiflags if no valid input sections were found. */
> > + if (!mips_elf_tdata (abfd)->abiflags_valid)
> > + {
> > + infer_mips_abiflags (abfd, abiflags);
> > + mips_elf_tdata (abfd)->abiflags_valid = TRUE;
> > + }
>
> I imagine this is right, but which case does it handle? Links with
> no MIPS ELF inputs?
It is both links with no MIPS ELF inputs and those with only empty inputs.
It is debatable as to what the end result would be from inferring flags
on such abfd as the e_flags may well be meaningless. The one thing that
will hold true is that .MIPS.abiflags and e_flags will be consistent.
> > @@ -14688,6 +15083,86 @@ _bfd_mips_elf_merge_private_bfd_data (bfd *ibfd, bfd *obfd)
> > if (null_input_bfd)
> > return TRUE;
> >
> > + /* Populate abiflags using existing information. */
> > + if (!mips_elf_tdata (ibfd)->abiflags_valid)
> > + {
> > + infer_mips_abiflags (ibfd, &mips_elf_tdata (ibfd)->abiflags);
> > + mips_elf_tdata (ibfd)->abiflags_valid = TRUE;
> > + }
> > + else
> > + {
> > + Elf_Internal_ABIFlags_v0 abiflags;
> > + Elf_Internal_ABIFlags_v0 *iabiflags;
> > + infer_mips_abiflags (ibfd, &abiflags);
> > + iabiflags = &mips_elf_tdata (ibfd)->abiflags;
> > +
> > + if (iabiflags->isa_level != abiflags.isa_level
> > + || iabiflags->isa_rev != abiflags.isa_rev)
> > + (*_bfd_error_handler)
> > + (_("%B: warning: Inconsistent ISA between e_flags and "
> > + ".MIPS.abiflags"), ibfd);
> > + if (abiflags.fp_abi != Val_GNU_MIPS_ABI_FP_ANY
> > + && iabiflags->fp_abi != abiflags.fp_abi)
> > + (*_bfd_error_handler)
> > + (_("%B: warning: Inconsistent FP ABI between e_flags and "
> > + ".MIPS.abiflags"), ibfd);
> > + if ((iabiflags->ases & abiflags.ases) != abiflags.ases)
> > + (*_bfd_error_handler)
> > + (_("%B: warning: Inconsistent ASEs between e_flags and "
> > + ".MIPS.abiflags"), ibfd);
>
> Minor nit, but "abiflags" vs. "iabiflags" is a bit hard to follow,
> since the "i" could be "implicit" (but I assume is "input").
Agreed. I was torn between short variables and keeping abiflags in
the name. Perhaps if I just used 'inferred' for the inferred flags.
The 'i' in iabiflags is supposed to mirror the other variables
associated with either (i)nput or (o)utput.
> > + if (!mips_elf_tdata (obfd)->abiflags_valid)
> > + {
> > + /* Copy input abiflags if output abiflags are not already valid. */
> > + mips_elf_tdata (obfd)->abiflags = mips_elf_tdata (ibfd)->abiflags;
> > + mips_elf_tdata (obfd)->abiflags_valid = TRUE;
> > + }
> > +
> > + if (! elf_flags_init (obfd))
> > + {
> > + elf_flags_init (obfd) = TRUE;
> > + elf_elfheader (obfd)->e_flags = new_flags;
> > + elf_elfheader (obfd)->e_ident[EI_CLASS]
> > + = elf_elfheader (ibfd)->e_ident[EI_CLASS];
> > +
> > + if (bfd_get_arch (obfd) == bfd_get_arch (ibfd)
> > + && (bfd_get_arch_info (obfd)->the_default
> > + || mips_mach_extends_p (bfd_get_mach (obfd),
> > + bfd_get_mach (ibfd))))
> > + {
> > + if (! bfd_set_arch_mach (obfd, bfd_get_arch (ibfd),
> > + bfd_get_mach (ibfd)))
> > + return FALSE;
> > + }
> > +
> > + return TRUE;
> > + }
> > +
> > + /* Update the output abiflags fp_abi using the computed fp_abi. */
> > + obj_attribute *out_attr = elf_known_obj_attributes (obfd)[OBJ_ATTR_GNU];
> > + mips_elf_tdata (obfd)->abiflags.fp_abi = out_attr[Tag_GNU_MIPS_ABI_FP].i;
> > +
> > +#define max(a,b) ((a) > (b) ? (a) : (b))
> > + /* Merge abiflags. */
> > + mips_elf_tdata (obfd)->abiflags.gpr_size
> > + = max (mips_elf_tdata (obfd)->abiflags.gpr_size,
> > + mips_elf_tdata (ibfd)->abiflags.gpr_size);
> > + mips_elf_tdata (obfd)->abiflags.cpr1_size
> > + = max (mips_elf_tdata (obfd)->abiflags.cpr1_size,
> > + mips_elf_tdata (ibfd)->abiflags.cpr1_size);
> > + mips_elf_tdata (obfd)->abiflags.cpr2_size
> > + = max (mips_elf_tdata (obfd)->abiflags.cpr2_size,
> > + mips_elf_tdata (ibfd)->abiflags.cpr2_size);
> > +#undef max
> > + mips_elf_tdata (obfd)->abiflags.ases
> > + |= mips_elf_tdata (ibfd)->abiflags.ases;
> > + mips_elf_tdata (obfd)->abiflags.isa_ext
> > + |= mips_elf_tdata (ibfd)->abiflags.isa_ext;
>
> isa_ext should be handled by bfd_set_arch_mach.
Are you saying we should ignore the isa_ext inputs and just use the new
overall arch to set this? The potential problem with bfd_set_arch_mach
is that it is called in a variety of different places and it can't
set just the isa_ext field and then mark the abiflags as valid. I'm
probably being dumb but I'm not sure how/where to hook this function
either? I haven't quite got my head around BFD target vectors.
> > @@ -14944,6 +15436,130 @@ _bfd_mips_elf_get_target_dtag (bfd_vma dtag)
> > }
> > }
> >
> > +static void
> > +print_mips_ases (FILE *file, unsigned int mask)
> > +{
> > + if (mask & AFL_ASE_DSP)
> > + fputs ("\n\tDSP ASE", file);
> > + if (mask & AFL_ASE_DSP64)
> > + fputs ("\n\tDSP ASE (64-bit)", file);
> > + if (mask & AFL_ASE_DSPR2)
> > + fputs ("\n\tDSP R2 ASE", file);
> > + if (mask & AFL_ASE_EVA)
> > + fputs ("\n\tEnhanced VA Scheme", file);
> > + if (mask & AFL_ASE_MCU)
> > + fputs ("\n\tMCU (MicroController) ASE", file);
> > + if (mask & AFL_ASE_MDMX)
> > + fputs ("\n\tMDMX ASE", file);
> > + if (mask & AFL_ASE_MIPS3D)
> > + fputs ("\n\tMIPS-3D ASE", file);
> > + if (mask & AFL_ASE_MT)
> > + fputs ("\n\tMT ASE", file);
> > + if (mask & AFL_ASE_SMARTMIPS)
> > + fputs ("\n\tSmartMIPS ASE", file);
> > + if (mask & AFL_ASE_VIRT)
> > + fputs ("\n\tVZ ASE", file);
> > + if (mask & AFL_ASE_VIRT64)
> > + fputs ("\n\tVZ ASE (64-bit)", file);
> > + if (mask & AFL_ASE_MSA)
> > + fputs ("\n\tMSA ASE", file);
> > + if (mask & AFL_ASE_MSA64)
> > + fputs ("\n\tMSA ASE (64-bit)", file);
> > + if (mask & AFL_ASE_MIPS16)
> > + fputs ("\n\tMIPS16 ASE", file);
> > + if (mask & AFL_ASE_MICROMIPS)
> > + fputs ("\n\tMICROMIPS ASE", file);
> > + if (mask & AFL_ASE_XPA)
> > + fputs ("\n\tXPA ASE", file);
> > + if (mask == 0)
> > + fputs ("\n\tNone", file);
>
> Hmm, wasn't the plan to get rid of the 64-bit versions? I think the external
> interface really shouldn't have this distinction. If we can do that while
> still using the AFL_* numbering internally then great, but if something
> has to give, it should be having a different external and internal numbering.
I think I'm going to have to give in and have different numbering for internal
and external for both ASEs and ISA extensions.
> > +static void
> > +print_mips_isa_ext (FILE *file, unsigned int mask)
> > +{
> > + if (mask & AFL_EXT_XLR)
> > + fputs ("\n\tRMI Xlr instruction", file);
> > + if (mask & AFL_EXT_OCTEON2)
> > + fputs ("\n\tCavium Networks Octeon2", file);
> > + if (mask & AFL_EXT_OCTEONP)
> > + fputs ("\n\tCavium Networks OcteonP", file);
> > + if (mask & AFL_EXT_LOONGSON_3A)
> > + fputs ("\n\tLoongson 3A", file);
> > + if (mask & AFL_EXT_OCTEON)
> > + fputs ("\n\tCavium Networks Octeon", file);
> > + if (mask & AFL_EXT_5900)
> > + fputs ("\n\tMIPS R5900", file);
> > + if (mask & AFL_EXT_4650)
> > + fputs ("\n\tMIPS R4650", file);
> > + if (mask & AFL_EXT_4010)
> > + fputs ("\n\tLSI R4010", file);
> > + if (mask & AFL_EXT_4100)
> > + fputs ("\n\tNEC VR4100", file);
> > + if (mask & AFL_EXT_3900)
> > + fputs ("\n\tToshiba R3900", file);
> > + if (mask & AFL_EXT_10000)
> > + fputs ("\n\tMIPS R10000", file);
> > + if (mask & AFL_EXT_SB1)
> > + fputs ("\n\tBroadcom SB-1", file);
> > + if (mask & AFL_EXT_4111)
> > + fputs ("\n\tNEC VR4111/VR4181", file);
> > + if (mask & AFL_EXT_4120)
> > + fputs ("\n\tNEC VR4120", file);
> > + if (mask & AFL_EXT_5400)
> > + fputs ("\n\tNEC VR5400", file);
> > + if (mask & AFL_EXT_5500)
> > + fputs ("\n\tNEC VR5500", file);
> > + if (mask & AFL_EXT_LOONGSON_2E)
> > + fputs ("\n\tST Microelectronics Loongson 2E", file);
> > + if (mask & AFL_EXT_LOONGSON_2F)
> > + fputs ("\n\tST Microelectronics Loongson 2F", file);
> > + if (mask == 0)
> > + fputs ("\n\tNone", file);
>
> As I said before, I think this should be an enum rather than a bitmask.
That's fine. I've concluded this one needs to be a different numbering
scheme for the ABI so I'll do an enum style.
> > +/* This is the struct we use to hold the module level set of options. Note
> > + that we must set the isa field to ISA_UNKNOWN and the ASE fields to
> > + -1 to indicate that they have not been initialized. */
> >
> > -/* 1 if -msingle-float, 0 if -mdouble-float. The default is 0. */
> > -static int file_mips_single_float = 0;
> > +static struct mips_set_options file_mips_opts =
> > +{
> > + /* isa */ ISA_UNKNOWN, /* ase */ 0, /* mips16 */ -1, /* micromips */ -1,
> > + /* noreorder */ 0, /* at */ ATREG, /* warn_about_macros */ 0,
> > + /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* insn32 */ FALSE,
> > + /* gp32 */ -1, /* fp */ -1, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
> > + /* soft_float */ FALSE, /* single_float */ FALSE,
> > + /* mips_defer_fp_warn */ FALSE
> > +};
> >
> > -/* True if -mnan=2008, false if -mnan=legacy. */
> > -static bfd_boolean mips_flag_nan2008 = FALSE;
> > +/* This is the struct we use to hold the current set of options. Note
> > + that we must set the isa field to ISA_UNKNOWN and the ASE fields to
> > + -1 to indicate that they have not been initialized. */
> >
> > static struct mips_set_options mips_opts =
> > {
> > /* isa */ ISA_UNKNOWN, /* ase */ 0, /* mips16 */ -1, /* micromips */ -1,
> > /* noreorder */ 0, /* at */ ATREG, /* warn_about_macros */ 0,
> > /* nomove */ 0, /* nobopt */ 0, /* noautoextend */ 0, /* insn32 */ FALSE,
> > - /* gp32 */ 0, /* fp32 */ 0, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
> > - /* soft_float */ FALSE, /* single_float */ FALSE
> > + /* gp32 */ -1, /* fp */ -1, /* arch */ CPU_UNKNOWN, /* sym32 */ FALSE,
> > + /* soft_float */ FALSE, /* single_float */ FALSE,
> > + /* mips_defer_fp_warn */ FALSE
> > };
>
> Let's not duplicate the comments like this -- please fuse them together
> more naturally.
OK. I expect you are only referring to large comments not the inline comments
For field names.
> > @@ -3602,6 +3627,173 @@ md_mips_end (void)
> > mips_emit_delays ();
> > if (! ECOFF_DEBUGGING)
> > md_obj_end ();
> > +
> > + int fpabi = bfd_elf_get_obj_attr_int (stdoutput, OBJ_ATTR_GNU,
> > + Tag_GNU_MIPS_ABI_FP);
> > + switch (fpabi)
> > + {
> > + case Val_GNU_MIPS_ABI_FP_DOUBLE:
> > + if ((file_mips_opts.gp32 ? 32 : 64) != file_mips_opts.fp
> > + || file_mips_opts.single_float == 1
> > + || file_mips_opts.soft_float == 1)
> > + as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> > + "`%s'"),
> > + Tag_GNU_MIPS_ABI_FP, fpabi,
> > + (file_mips_opts.single_float == 1) ? "single-float"
> > + : (file_mips_opts.soft_float == 1) ? "soft-float"
> > + : (file_mips_opts.fp == 32) ? "fp32"
> > + : (file_mips_opts.fp == 64) ? "fp64" : "fpxx");
> > + break;
> > + case Val_GNU_MIPS_ABI_FP_XX:
> > + if (mips_abi != O32_ABI
> > + || file_mips_opts.fp != 0
> > + || file_mips_opts.single_float == 1
> > + || file_mips_opts.soft_float == 1)
> > + as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> > + "`%s'"),
> > + Tag_GNU_MIPS_ABI_FP, fpabi,
> > + (mips_abi != O32_ABI) ? "!-mabi=32"
> > + : (file_mips_opts.single_float == 1) ? "single-float"
> > + : (file_mips_opts.soft_float == 1) ? "soft-float"
> > + : (file_mips_opts.fp == 32) ? "fp32"
> > + : (file_mips_opts.fp == 64) ? "fp64" : "fpxx");
> > + break;
> > + case Val_GNU_MIPS_ABI_FP_64:
> > + if (mips_abi != O32_ABI
> > + || file_mips_opts.fp != 64
> > + || file_mips_opts.single_float == 1
> > + || file_mips_opts.soft_float == 1)
> > + as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> > + "`%s'"),
> > + Tag_GNU_MIPS_ABI_FP, fpabi,
> > + (mips_abi != O32_ABI) ? "!-mabi=32"
> > + : (file_mips_opts.single_float == 1) ? "single-float"
> > + : (file_mips_opts.soft_float == 1) ? "soft-float"
> > + : (file_mips_opts.fp == 32) ? "fp32"
> > + : (file_mips_opts.fp == 64) ? "fp64" : "fpxx");
> > + break;
> > + case Val_GNU_MIPS_ABI_FP_SINGLE:
> > + if (file_mips_opts.single_float != 1)
> > + as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> > + "not `single-float'"),
> > + Tag_GNU_MIPS_ABI_FP, fpabi);
> > + break;
> > + case Val_GNU_MIPS_ABI_FP_SOFT:
> > + if (file_mips_opts.soft_float != 1)
> > + as_warn (_("Incorrect .gnu_attribute %d,%d found when module is "
> > + "not `soft-float'"),
> > + Tag_GNU_MIPS_ABI_FP, fpabi);
> > + break;
> > + case Val_GNU_MIPS_ABI_FP_OLD_64:
> > + as_warn (_("Incorrect .gnu_attribute %d,%d found. ABI not "
> > + "supported"),
> > + Tag_GNU_MIPS_ABI_FP, fpabi);
> > + break;
> > + default:
> > + if (fpabi != Val_GNU_MIPS_ABI_FP_ANY)
> > + as_warn (_("Incompatible module option and .gnu_attribute seen, "
> > + "unexpected Tag_GNU_MIPS_ABI_FP: %d"),
> > + fpabi);
> > + break;
>
> Warnings should start with lower case (the MIPS code should be consistent
> about that now, although the target-independent code isn't always).
> Please use %s even where only one incompatibility exists (single and
> soft float). Also, please use the "not `%s'" version when the ABI
> wasn't o32 but needed to be.
OK.
> > +/* Perform consistency checks on the current options. */
> > +
> > +static void
> > +mips_check_options (struct mips_set_options *opts, bfd_boolean abi_checks)
> > +{
> > + /* Check the size of integer registers agrees with the ABI and ISA. */
> > + if (opts->gp32 == 0 && !ISA_HAS_64BIT_REGS (opts->isa))
> > + as_bad (_("`gp64' used with a 32-bit processor"));
> > + else if (abi_checks == TRUE
> > + && opts->gp32 == 1 && ABI_NEEDS_64BIT_REGS (mips_abi))
> > + as_bad (_("`gp32' used with a 64-bit ABI"));
> > + else if (abi_checks == TRUE
> > + && opts->gp32 == 0 && ABI_NEEDS_32BIT_REGS (mips_abi))
> > + as_bad (_("`gp64' used with a 32-bit ABI"));
> > +
> > + /* Check the size of the float registers agrees with the ABI and ISA. */
> > + switch (opts->fp)
> > + {
> > + case 0:
> > + if (!CPU_HAS_LDC1_SDC1 (opts->arch))
> > + as_bad (_("`fpxx' used with a cpu lacking ldc1/sdc1 instructions"));
> > + else if (opts->single_float == 1
> > + || opts->soft_float == 1)
> > + as_bad (_("`fpxx' cannot be used with `single-float' or `soft-float'"));
> > + break;
> > + case 64:
> > + if (!ISA_HAS_64BIT_FPRS (opts->isa))
> > + as_bad (_("`fp64' used with a 32-bit fpu"));
> > + else if (abi_checks == TRUE
> > + && ABI_NEEDS_32BIT_REGS (mips_abi)
> > + && !ISA_HAS_MXHC1 (opts->isa))
> > + as_warn (_("`fp64' used with a 32-bit ABI"));
>
> You said before that -mfp64 and -msingle-float were compatible, but I'm not
> sure they are really. I think that should be an error too.
This area is fuzzy at best. I've seen evidence both ways round in binutils
(I can't point it out right now though) but GCC certainly only supports
-mfp32 -msingle-float so I'd be happy with enforcing that in gas and
dealing with any potential issues.
> > + if ((regno & 1) != 0)
> > + {
> > + if (mips_opts.mips_defer_fp_warn == TRUE)
> > + as_warn (_("Dangerous use of FP registers in fp%s when module is fp%s"),
> > + (mips_opts.fp == 32) ? "32"
> > + : (mips_opts.fp == 64) ? "64" : "xx",
> > + (file_mips_opts.fp == 32) ? "32"
> > + : (file_mips_opts.fp == 64) ? "64" : "xx");
>
> I don't think we should warn about code that's compatible with the
> current ".set fp". How would you write a warning-free fp=64 ifunc?
This is perhaps a non-intuitive piece of code but the mips_defer_fp_warn flag
will only be set if the overall module was not FPXX, the current mode is
not FPXX and the two do not match. The only way to support having both FP32
and FP64 in the same module is if the overall module is FPXX. The warning handles
the case of 'xx' when in reality it can never happen. It can only print fp32,fp64
or fp64,fp32. The reason the overall module must be FPXX (or matching the current
mode) is because that is the only way you get the choice of mode at runtime (FPXX)
or the correct mode to start with (FP64).
The specific example of fp=64 ifunc:
.module fp=xx
.set push
.set fp=64
add.s $f1, $f1, $f1
.set pop
> > @@ -5322,13 +5527,12 @@ match_float_constant (struct mips_arg_info *arg, expressionS
> *imm,
> > /* Handle 64-bit constants for which an immediate value is best. */
> > if (length == 8
> > && !mips_disable_float_construction
> > - /* Constants can only be constructed in GPRs and copied
> > - to FPRs if the GPRs are at least as wide as the FPRs.
> > - Force the constant into memory if we are using 64-bit FPRs
> > - but the GPRs are only 32 bits wide. */
> > - /* ??? No longer true with the addition of MTHC1, but this
> > - is legacy code... */
> > - && (using_gprs || !(HAVE_64BIT_FPRS && HAVE_32BIT_GPRS))
> > + /* Constants can only be constructed in GPRs and copied to FPRs if the
> > + GPRs are at least as wide as the FPRs or MTHC1 is available. */
> > + && (using_gprs
> > + || HAVE_64BIT_GPRS
> > + || ISA_HAS_MXHC1 (mips_opts.isa)
> > + || (HAVE_32BIT_FPRS && mips_opts.fp != 0))
>
> We should keep the bit about forcing it to memory otherwise.
> Can you explain the HAVE_32BIT_FPRS && mips_opts.fp != 0 thing a bit more?
> Maybe we should get rid of HAVE_32BIT_FPRS and HAVE_64BIT_FPRS now that
> there are three alternatives and just test mips_opts.fp directly.
> (Please do that as a separate patch though -- there are quite a few
> different things going on in the current patch.)
The final clause is to allow 64-bit constant generation when the value would
need to be moved to FPRs using a pair of MTC1s (not MTHC1). I retained the
HAVE_32BIT_FPRS clause as it expands to more than just fp != 64, but I don't
understand why it is more complex than that. The fp != 0 could just as easily
have been fp == 32 which is probably clearer and is there to ensure that
we don't break the FPXX ABI by using an MTC1 for the high part of a 64-bit
value.
There are still only two widths of register. The fact we are targeting FPXX
is only relevant in a small number of places, otherwise it is simply 32-bit
registers. So I would be changing HAVE_32BIT_FPRS to fp != 64 anyway.
> > @@ -6690,7 +6894,8 @@ append_insn (struct mips_cl_insn *ip, expressionS *address_expr,
> > && (mips_opts.at || mips_pic == NO_PIC)
> > /* Don't relax BPOSGE32/64 or BC1ANY2T/F and BC1ANY4T/F
> > as they have no complementing branches. */
> > - && !(ip->insn_mo->ase & (ASE_MIPS3D | ASE_DSP64 | ASE_DSP)));
> > + && !(ip->insn_mo->ase & (AFL_ASE_MIPS3D | AFL_ASE_DSP64
> > + | AFL_ASE_DSP)));
> >
> > if (!HAVE_CODE_COMPRESSION
> > && address_expr
>
> Would be great if you could split out the ASE renaming into a separate
> patch too.
I will either undo this or do a separate patch to somehow rid the internal
implementation of needing 64-bit ASEs in the same mask as the real ASEs.
> > @@ -13506,39 +13715,39 @@ md_parse_option (int c, char *arg)
> > break;
> >
> > case OPTION_MIPS1:
> > - file_mips_isa = ISA_MIPS1;
> > + file_mips_opts.isa = ISA_MIPS1;
> > break;
> >
> > case OPTION_MIPS2:
> > - file_mips_isa = ISA_MIPS2;
> > + file_mips_opts.isa = ISA_MIPS2;
> > break;
> >
> > case OPTION_MIPS3:
> > - file_mips_isa = ISA_MIPS3;
> > + file_mips_opts.isa = ISA_MIPS3;
> > break;
> >
> > case OPTION_MIPS4:
> > - file_mips_isa = ISA_MIPS4;
> > + file_mips_opts.isa = ISA_MIPS4;
> > break;
> >
> > case OPTION_MIPS5:
> > - file_mips_isa = ISA_MIPS5;
> > + file_mips_opts.isa = ISA_MIPS5;
> > break;
> >
> > case OPTION_MIPS32:
> > - file_mips_isa = ISA_MIPS32;
> > + file_mips_opts.isa = ISA_MIPS32;
> > break;
> >
> > case OPTION_MIPS32R2:
> > - file_mips_isa = ISA_MIPS32R2;
> > + file_mips_opts.isa = ISA_MIPS32R2;
> > break;
> >
> > case OPTION_MIPS64R2:
> > - file_mips_isa = ISA_MIPS64R2;
> > + file_mips_opts.isa = ISA_MIPS64R2;
> > break;
> >
> > case OPTION_MIPS64:
> > - file_mips_isa = ISA_MIPS64;
> > + file_mips_opts.isa = ISA_MIPS64;
> > break;
> >
> > case OPTION_MTUNE:
>
> I think this consolidation into file_mips_opts should be a separate patch too
> (without changing the logic).
That's fine. I wanted to take advice on splitting this up, I wanted to avoid
renaming the fp32 field in a separate patch to consolidating things.
> > @@ -14922,30 +15091,11 @@ struct mips_option_stack
> >
> > static struct mips_option_stack *mips_opts_stack;
> >
> > -/* Handle the .set pseudo-op. */
> > -
> > -static void
> > -s_mipsset (int x ATTRIBUTE_UNUSED)
> > +static bfd_boolean
> > +s_mipssettings (char * name)
>
> s_foo is just for directives -- let's call this parse_code_option or
> something (suggestions for better names welcome).
OK.
> > @@ -15155,23 +15281,87 @@ s_mipsset (int x ATTRIBUTE_UNUSED)
> > free (s);
> > }
> > }
> > - else if (strcmp (name, "sym32") == 0)
> > - mips_opts.sym32 = TRUE;
> > - else if (strcmp (name, "nosym32") == 0)
> > - mips_opts.sym32 = FALSE;
> > - else if (strchr (name, ','))
> > + else if (s_mipssettings (name) == FALSE)
> > + as_warn (_("tried to set unrecognized symbol: %s\n"), name);
> > +
> > + /* The use of .set [arch|cpu]= historically 'fixes' the width of gp and fp
> > + registers based on what is supported by the arch/cpu. */
> > + if (mips_opts.isa != prev_isa)
> > {
> > - /* Generic ".set" directive; use the generic handler. */
> > - *input_line_pointer = ch;
> > - input_line_pointer = name;
> > - s_set (0);
> > - return;
> > + switch (mips_opts.isa)
> > + {
> > + case 0:
> > + break;
> > + case ISA_MIPS1:
> > + case ISA_MIPS2:
> > + case ISA_MIPS32:
> > + case ISA_MIPS32R2:
> > + mips_opts.gp32 = 1;
> > + if (mips_opts.fp != 0)
> > + mips_opts.fp = 32;
> > + break;
> > + case ISA_MIPS3:
> > + case ISA_MIPS4:
> > + case ISA_MIPS5:
> > + case ISA_MIPS64:
> > + case ISA_MIPS64R2:
> > + mips_opts.gp32 = 0;
> > + if (mips_opts.arch == CPU_R5900)
> > + {
> > + if (mips_opts.fp != 0)
> > + mips_opts.fp = 32;
> > + }
> > + else if (mips_opts.fp != 0)
> > + mips_opts.fp = 64;
>
> Seems more natural as:
>
> if (mips_opts.fp != 0)
> {
> if (mips_opts.arch == CPU_R5900)
> mips_opts.fp = 32;
> else
> mips_opts.fp = 64;
> }
>
OK.
> > +/* Handle the .module pseudo-op. */
> > +
> > +static void
> > +s_module (int ignore ATTRIBUTE_UNUSED)
> > +{
> > + char *name = input_line_pointer, ch;
> > + int prev_arch = file_mips_opts.arch;
> > +
> > + while (!is_end_of_line[(unsigned char) *input_line_pointer])
> > + ++input_line_pointer;
> > + ch = *input_line_pointer;
> > + *input_line_pointer = '\0';
> > +
> > + if (file_mips_opts_checked == FALSE)
> > {
> > - as_warn (_("tried to set unrecognized symbol: %s\n"), name);
> > + if (s_mipssettings (name) == FALSE)
> > + as_warn (_(".module used with unrecognized symbol: %s\n"), name);
>
> !foo rather than foo == FALSE (throughout)
OK.
> > +
> > + /* Update module level settings from mips_opts. */
> > + file_mips_opts = mips_opts;
> > }
> > - mips_check_isa_supports_ases ();
> > + else
> > + as_warn (_("ignoring .module after generating code"));
>
> I think this should be an as_bad.
OK.
> > + if (prev_arch != file_mips_opts.arch
> > + && ! bfd_set_arch_mach (stdoutput, bfd_arch_mips, file_mips_opts.arch))
> > + as_warn (_("could not set architecture and machine"));
>
> I think we should do this unconditionally when checking the options
> rather than here.
OK. I was cautious as I didn't know if there would be any side effects
of calling bfd_set_arch_mach unnecessarily.
> > @@ -18272,3 +18560,33 @@ tc_mips_regname_to_dw2regnum (char *regname)
> >
> > return regnum;
> > }
> > +
> > +/* Given a symbolic attribute NAME, return the proper integer value.
> > + Returns -1 if the attribute is not known. */
> > +
> > +int
> > +mips_convert_symbolic_attribute (const char *name)
> > +{
> > + static const struct
> > + {
> > + const char * name;
> > + const int tag;
> > + }
> > + attribute_table[] =
> > + {
> > +#define T(tag) {#tag, tag}
> > + T (Tag_GNU_MIPS_ABI_FP),
> > + T (Tag_GNU_MIPS_ABI_MSA),
> > +#undef T
> > + };
> > + unsigned int i;
> > +
> > + if (name == NULL)
> > + return -1;
> > +
> > + for (i = 0; i < ARRAY_SIZE (attribute_table); i++)
> > + if (streq (name, attribute_table[i].name))
> > + return attribute_table[i].tag;
> > +
> > + return -1;
> > +}
>
> I think this should be a separate patch too.
OK.
> > diff --git a/gas/doc/c-mips.texi b/gas/doc/c-mips.texi
> > index 0c5e82d..37926d4 100644
> > --- a/gas/doc/c-mips.texi
> > +++ b/gas/doc/c-mips.texi
> > @@ -28,6 +28,7 @@ Assembly Language Programming'' in the same work.
> > * MIPS assembly options:: Directives to control code generation
> > * MIPS autoextend:: Directives for extending MIPS 16 bit instructions
> > * MIPS insn:: Directive to mark data as an instruction
> > +* MIPS FP ABIs:: Marking which FP ABI is in use
> > * MIPS NaN Encodings:: Directives to record which NaN encoding is being used
> > * MIPS Option Stack:: Directives to save and restore options
> > * MIPS ASE Instruction Generation Overrides:: Directives to control
> > @@ -119,6 +120,15 @@ The @code{.set gp=64} and @code{.set fp=64} directives allow the
> size
> > of registers to be changed for parts of an object. The default value is
> > restored by @code{.set gp=default} and @code{.set fp=default}.
> >
> > +@item -mfpxx
> > +Make no assumptions about whether 32-bit or 64-bit registers are available.
>
> ...floating-point registers...?
Indeed.
> > +This is provided to support having modules compatible with either
> > +@samp{-mfp32} or @samp{-mfp64}. This option can only be used with MIPS II
> > +and above.
> > +
> > +The @code{.set fp=xx} directive allows a part of an object to be marked
> > +as not making assumptions about 32-bit or 64-bita FP registers. The
> > +default value is restored by @code{.set fp=default}.
> > @item -mips16
> > @itemx -no-mips16
> > Generate code for the MIPS 16 processor. This is equivalent to putting
> > @@ -687,6 +697,22 @@ Traditional MIPS assemblers do not support this directive.
> > @node MIPS assembly options
> > @section Directives to control code generation
> >
> > +@cindex MIPS directives to override command line options
> > +@kindex @code{.module}
> > +The @code{.module} directive allows command line options to be set directly
> > +from assembly. The format of the directive matches the @code{.set}
> > +directive but only those options which are relevant to a whole module are
> > +supported. The effect of a @code{.module} directive is the same as the
> > +corresponding command line option. Where @code{.set} directives support
> > +returning to a default then the @code{.module} directives do not as they
> > +define the defaults.
> > +
> > +These module level directives must appear first in assembly and will raise
>
> Nit: module-level
OK.
> > +a warning if found after the first instruction, @code{.set} directive or
> > +any code generating directive.
>
> If it becomes a hard error we can remove the "and will raise..." bit.
I guess it is clear enough to just say "appear first in assembly" in hindsight.
> > @@ -749,6 +775,108 @@ baz:
> >
> > @end example
> >
> > +@node MIPS FP ABIs
> > +@section Directives to control the FP ABI
> > +@menu
> > +* MIPS FP ABI History:: History of FP ABIs
> > +* MIPS FP ABI Variants:: Supported FP ABIs
> > +* MIPS FP ABI Selection:: Automatic selection of FP ABI
> > +* MIPS FP ABI Compatibility:: Linking different FP ABI variants
> > +@end menu
> > +
> > +@node MIPS FP ABI History
> > +@subsection History of FP ABIs
> > +@cindex @code{.gnu_attribute 4, @var{n}} directive, MIPS
> > +@cindex @code{.gnu_attribute Tag_GNU_MIPS_ABI_FP, @var{n}} directive, MIPS
> > +The MIPS ABIs support a variety of different floating-point extensions
> > +where calling-convention and register sizes vary for floating-point data.
> > +The extensions exist to support a wide variety of optional architecture
> > +features. The resulting ABI variants are generally incompatible with each
> > +other and must be tracked carefully.
> > +
> > +Traditionally the use of an explicit @code{.gnu_attribute 4, @var{n}}
> > +directive is used to indicate which ABI is in use by a specific module.
> > +It was then left to the user to ensure that command line options and the
> > +selected ABI were compatible with some potential for inconsistencies.
> > +
> > +@node MIPS FP ABI Variants
> > +@subsection Supported FP ABIs
> > +The supported floating-point ABI variants are:
> > +
> > +@table @code
> > +@item 0 - No floating-point
> > +This variant is used to indicate that floating-point is not used within
> > +the module at all and therefore has no impact on the ABI. This is the
> > +default.
> > +
> > +@item 1 - Double-precision
> > +This variant indicates that double-precision support is used. For 64-bit
> > +ABIs this means that 64-bit wide floating-point registers are required.
> > +For 32-bit ABIs this means that 32-bit wide floating-point registers are
> > +required and double precision operations across pairs of registers.
>
> found the last sentence hard to parse: s/across/use/, or something?
Yes, clearly missing a word but changing to 'use' works.
> > +@item 2 - Single-precision
> > +This variant indicates that single-precision support is used. This is
> > +generally taken to mean that the ABI is also modified such that
> > +sizeof (double) == sizeof (float). This has an impact on calling
> > +convention and callee-save behaviour.
>
> I'm not sure about the sizeof (double) thing: I think it's legitimate
> to use -msingle-float with a 32-bit FPU and use software libraries
> for double-precision.
Granted. But I believe the intent of the existing single-precision ABI is
that it is also -fshort-double in GCC terms (r5900 I believe). We would
need to track both single-float and single-float short-double separately.
There are no current plans (from us at least) to support just
single-precision with softfloat doubles. The R6 spec re-introduces the
idea of a single precision only FPU and this is planned to be primarily
supported with short-doubles.
> > +@node MIPS FP ABI Selection
> > +@subsection Automatic selection of FP ABI
> > +@cindex @code{.module fp=@var{nn}} directive, MIPS
> > +In order to simplify and add safety to the process of selecting the
> > +correct floating-point ABI, the assembler will automatically infer the
> > +correct @code{.gnu_attribute 4, @var{n}} directive based on command line
> > +options @code{.module} overrides and instruction usage. Where an explicit
> > +@code{.gnu_attribute 4, @var{n}} directive has been seen then a warning
> > +will be raised if it does not match an inferred setting.
>
> Obviously the "and instruction usage" is the contentious bit here :-)
If we go with not having any safety net for hand written assembler then
instruction usage does still need to be included in this decision in order
to support getting modules that end up as FP 'ANY' i.e. no-float. Otherwise
we must write-off the ability to generate modules without a specific FP ABI
because there is no way to detect if a user has written .gnu_attribute 4,0
nor is there a command-line/module option to assert it.
Any thoughts on what to do with attribute 4,0? Since it is not very safe
in GCC terms then we could just not support it I suppose.
I expect you can guess that I'm still not convinced by providing no safety net
at all for hand-written code :-) But I do understand the desire to keep things
clear cut. The original plan would have avoided this issue as it defined new
directives to get into fpxx. Perhaps it won't be an issue but I really wanted
to be able to provide distribution maintainers with a tool to detect which
objects remained FP32 even after the transition to an FPXX toolchain.
> I think it'd be better to change gp32 into gp too, for consistency.
> I can do that as a follow-up though.
Almost did that but stuck to necessary changes.
Thanks for getting through the patch, it was a monster. I'll post a split
patches soon.
Matthew
More information about the Binutils
mailing list