This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 1/2] Add support for O32 FPXX ABI
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- Cc: "macro\ at codesourcery dot com" <macro at codesourcery dot com>, "Joseph Myers \(joseph\ at codesourcery dot com\)" <joseph at codesourcery dot com>, "binutils\ at sourceware dot org" <binutils at sourceware dot org>, Rich Fuhler <Rich dot Fuhler at imgtec dot com>
- Date: Tue, 06 May 2014 20:19:23 +0100
- Subject: Re: [PATCH 1/2] Add support for O32 FPXX ABI
- Authentication-results: sourceware.org; auth=none
- References: <6D39441BF12EF246A7ABCE6654B023534DAAAB at LEMAIL01 dot le dot imgtec dot org> <871tx9z35k dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <6D39441BF12EF246A7ABCE6654B023534DFC40 at LEMAIL01 dot le dot imgtec dot org> <878urd37t7 dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <6D39441BF12EF246A7ABCE6654B023534DFD9B at LEMAIL01 dot le dot imgtec dot org> <87fvll1ha9 dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <6D39441BF12EF246A7ABCE6654B023534E07CB at LEMAIL01 dot le dot imgtec dot org> <87k3awnuf1 dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023534E0F50 at LEMAIL01 dot le dot imgtec dot org> <87ppknzv7m dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023534E16D7 at LEMAIL01 dot le dot imgtec dot org> <877g6vzpj1 dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023534E3030 at LEMAIL01 dot le dot imgtec dot org> <87eh0yl4xx dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023534E3AA8 at LEMAIL01 dot le dot imgtec dot org> <87d2ghk7fu dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B023535127C8 at LEMAIL01 dot le dot imgtec dot org> <87r44mve6s dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <6D39441BF12EF246A7ABCE6654B02353512A10 at LEMAIL01 dot le dot imgtec dot org> <87r44leu34 dot fsf at talisman dot default> <6D39441BF12EF246A7ABCE6654B0235351D76A at LEMAIL01 dot le dot imgtec dot org>
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.
> +/* 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.
> + 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.
> @@ -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?
> @@ -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").
> + 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.
> @@ -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.
> +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.
> +/* 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.
> @@ -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.
> +/* 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.
> + 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?
> @@ -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.)
> @@ -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.
> @@ -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).
> @@ -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).
> @@ -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;
}
> +/* 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)
> +
> + /* 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.
> + 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.
> @@ -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.
> 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...?
> +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
> +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.
> @@ -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?
> +@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.
> +@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 :-)
I think it'd be better to change gp32 into gp too, for consistency.
I can do that as a follow-up though.
Thanks,
Richard