RE: [PATCH v2] Add support for O32 FPXX ABI

[Matthew Fortune <Matthew dot Fortune at imgtec dot com>]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> > I mentioned doing the FPR_SIZE change next but if it is OK I would
> > like to do that change after submitting the bulk of this work as it
> > will be safer to change it afterwards. The option handling rework
> > in this patch is intricate and I don't want to risk breaking that
> > by making a significant change to how FPR sizes are tested.
> 
> Sorry, but I think the FPR_SIZE stuff really should come first.

Posted separately.
 
> > I do still need to do 'something' for bare metal handling of all
> > this. My current thought is to specify a special symbol that the
> > user must define which will lead to the .MIPS.abiflags section
> > becoming loadable and the symbol point at it. If the special symbol
> > has not been provided by the user then leave .MIPS.abiflags as not
> > loadable.
> > The reasons:
> >
> > * If the user does not define the symbol then no additional data
> >   is added to the program. A boot rom or other loader would then
> >   be responsible for the configuration of hardware with
> >   information obtained when preparing the system by inspecting the
> >   .MIPS.abiflags section in the ELF.
> > * While the FPXX ABI only needs the FR mode requirement to be
> >   indicated to hardware, we also need to indicate that MSA is
> >   needed so that it can be enabled too. There then seems little
> >   point in doing anything other than the whole section.
> >
> > You may be able to suggest a more appropriate trigger for an end
> > user (or the standard crt) to include in their source to make the
> > .MIPS.abiflags loadable. Does that sound vaguely sane?
> 
> It might work, but I'm not sure how you're planning to integrate
> it with the linker script placement of the section.

Good point, it can only go in one place which means it will end up taking
space in a loadable segment whether it is loadable or not I guess.
My alternative thought was to have the user/crt define a specially named
data section of an appropriate size that would be filled in by the linker.
It's difficult to choose a name for such a section but perhaps
.MIPS.abiflags.0 where the .0 indicates the version.

> > @@ -14375,7 +14675,17 @@ mips_elf_merge_obj_attributes (bfd *ibfd, bfd
> *obfd)
> >        out_attr[Tag_GNU_MIPS_ABI_FP].type = 1;
> >        if (out_fp == Val_GNU_MIPS_ABI_FP_ANY)
> >  	out_attr[Tag_GNU_MIPS_ABI_FP].i = in_fp;
> > -      else if (in_fp != Val_GNU_MIPS_ABI_FP_ANY)
> > +      else if (out_fp == Val_GNU_MIPS_ABI_FP_XX
> > +	       && (in_fp == Val_GNU_MIPS_ABI_FP_DOUBLE
> > +		   || in_fp == Val_GNU_MIPS_ABI_FP_64))
> > +	{
> > +	  mips_elf_tdata (obfd)->abi_fp_bfd = ibfd;
> > +	  out_attr[Tag_GNU_MIPS_ABI_FP].i = in_attr[Tag_GNU_MIPS_ABI_FP].i;
> > +	}
> > +      else if (in_fp != Val_GNU_MIPS_ABI_FP_ANY
> > +	       && (in_fp != Val_GNU_MIPS_ABI_FP_XX
> > +		   || (out_fp != Val_GNU_MIPS_ABI_FP_64
> > +		       && out_fp != Val_GNU_MIPS_ABI_FP_DOUBLE)))
> 
> Minor, but I think this would be clearer with:
> 
>       else if (in_fp == Val_GNU_MIPS_ABI_FP_XX
> 	       && (out_fp == Val_GNU_MIPS_ABI_FP_DOUBLE
> 		   || out_fp == Val_GNU_MIPS_ABI_FP_64))
>         /* Keep the current setting */;
>       else if (in_fp != Val_GNU_MIPS_ABI_FP_ANY)

OK. I was torn between these two, I'm always wary of empty statements.

> > +      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);
> 
> I think this should check isa_ext too.

OK. I renamed iabiflags to in_abiflags too, I thought I'd done that
previously but clearly had not.

> > +static void
> > +print_mips_ases (FILE *file, unsigned int mask)
> > +{
> > +  if (mask & AFL_ASE_DSP)
> > +    fputs ("\n\tDSP ASE", 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_MSA)
> > +    fputs ("\n\tMSA ASE", 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);
> 
> "None" should be marked for translation: _("\n\tNone").

OK. I have never put much thought into translation before. I'll
try to bear it in mind in the future.

> > +static void
> > +print_mips_isa_ext (FILE *file, unsigned int isa_ext)
> > +{
> > +  switch (isa_ext)
> > +    {
> > +    case 0:
> > +      fputs ("None", file);
> 
> Same here.
> 
> > +    case AFL_EXT_XLR:
> > +      fputs ("RMI Xlr instruction", file);
> > +      break;
> 
> This entry looks a bit weird: "RMI XLR processor"?
> 
> > +    case AFL_EXT_5900:
> > +      fputs ("MIPS R5900", file);
> > +      break;
> 
> "Toshiba" is probably more accurate.

I expect I should therefore do 'QED R4650' instead of MIPS?

> > +      fputs ("Unknown", file);
> > +    }
> 
> _(...) here too.  Would prefer an explicit break.
> 
> Same comments for the readelf version.

OK

...snip...
> static void
> check_fpabi (int fpabi)
> {
>   /* Check -mabi and register sizes.  */
>   switch (fpabi)
>     {
>     case Val_GNU_MIPS_ABI_FP_DOUBLE:
>       if (!file_mips_opts.gp32 && file_mips_opts.fp == 32)
>         fpabi_incompatible_with (fpabi, "gp=64 fp=32");
>       else if (file_mips_opts.gp32 && file_mips_opts.fp == 64)
>         fpabi_incompatible_with (fpabi, "gp=32 fp=64");
>       break;
> 
>     case Val_GNU_MIPS_ABI_FP_XX:
>       if (mips_abi != O32_ABI)
>         fpabi_requires (fpabi, "-mabi=32");
>       else if (file_mips_opts.fp != 0)
>         fpabi_requires (fpabi, "fp=xx");
>       break;
> 
>     case Val_GNU_MIPS_ABI_FP_64:
>       if (mips_abi != O32_ABI)
>         fpabi_requires (fpabi, "-mabi=32");
>       else if (file_mips_opts.fp != 64)
>         fpabi_requires (fpabi, "fp=64");
>       break;
> 
>     case Val_GNU_MIPS_ABI_FP_SINGLE:
>     case Val_GNU_MIPS_ABI_FP_SOFT:
>       break;
> 
>     case Val_GNU_MIPS_ABI_FP_OLD_64:
>       as_warn (_(".gnu_attribute %d,%d is no longer supported", fpabi);
>       return;
> 
>     default:
>       as_warn (_(".gnu_attribute %d,%d is not a recognized"
>                  " floating-point ABI", fpabi);
>       return;
>     }
> 
>   if (file_mips_opts.single_float && fpabi !=
> Val_GNU_MIPS_ABI_FP_SINGLE)
>     fpabi_incompatible_with (fpabi, "single-float");
>   if (file_mips_opts.soft_float && fpabi != Val_GNU_MIPS_ABI_FP_SOFT)
>     fpabi_incompatible_with (fpabi, "soft-float");
> }
> 
>   if (fpabi != Val_GNU_MIPS_ABI_FP_ANY)
>     check_fpabi (abi);

OK. With the following change:

  if (fpabi == Val_GNU_MIPS_ABI_FP_SINGLE && !file_mips_opts.single_float)
    fpabi_incompatible_with (fpabi, "doublefloat");
  if (fpabi == Val_GNU_MIPS_ABI_FP_SOFT && !file_mips_opts.soft_float)
    fpabi_incompatible_with (fpabi, "hardfloat");

> > +  /* 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 (opts->single_float == 1
> > +	       || opts->soft_float == 1)
> > +	as_bad (_("`fp64' cannot be used with `single-float' or `soft-
> float'"));
> > +      else if (abi_checks
> > +	       && ABI_NEEDS_32BIT_REGS (mips_abi)
> > +	       && !ISA_HAS_MXHC1 (opts->isa))
> > +	as_warn (_("`fp64' used with a 32-bit ABI"));
> > +      break;
> > +    case 32:
> > +      if (abi_checks
> > +	  && ABI_NEEDS_64BIT_REGS (mips_abi))
> > +	as_warn (_("`fp32' used with a 64-bit ABI"));
> > +      break;
> > +    default:
> > +      as_bad (_("Unknown size of floating point registers"));
> > +    }
> 
> Seems odd to require -mfp32 for -msoft-float.  I think we should just
> ignore the -mfp setting in that case.

OK

> The error messages should either use the full option name or the
> .module option (such as -mfp64 or fp=64).

OK. I've fixed a couple of my references to single-float and soft-float
to be singlefloat and softfloat as well to match the .module names.

> > +/* Perform consistency checks on the module level options exactly
> once.
> > +   This is a deferred check that happens:
> > +     at the first .set directive
> > +     or, at the first pseudo op that generates code
> > +     or, at the first instruction
> > +     or, at the end.  */
> > +
> > +static void
> > +file_mips_check_options (void)
> > +{
> > +  int fpabi = Val_GNU_MIPS_ABI_FP_ANY;
> > +
> > +  if (file_mips_opts_checked)
> > +    return;
> > +
> > +  mips_check_options (&file_mips_opts, TRUE);
> > +  file_mips_opts_checked = TRUE;
> > +
> > +  if (!bfd_set_arch_mach (stdoutput, bfd_arch_mips,
> file_mips_opts.arch))
> > +    as_warn (_("could not set architecture and machine"));
> > +
> > +  /* Soft-float gets precedence over single-float, the two options
> should
> > +     not be used together so this should not matter.  */
> > +  if (file_mips_opts.soft_float == 1)
> > +    fpabi = Val_GNU_MIPS_ABI_FP_SOFT;
> > +  /* Single-float gets precedence over all double_float cases.  */
> > +  else if (file_mips_opts.single_float == 1)
> > +    fpabi = Val_GNU_MIPS_ABI_FP_SINGLE;
> > +  else
> > +    {
> > +      switch (file_mips_opts.fp)
> > +	{
> > +	case 32:
> > +	  fpabi = Val_GNU_MIPS_ABI_FP_DOUBLE;
> > +	  break;
> > +	case 0:
> > +	  fpabi = Val_GNU_MIPS_ABI_FP_XX;
> > +	  break;
> > +	case 64:
> > +	  if (file_mips_opts.gp32)
> > +	    fpabi = Val_GNU_MIPS_ABI_FP_64;
> > +	  else
> > +	    fpabi = Val_GNU_MIPS_ABI_FP_DOUBLE;
> > +	  break;
> > +	}
> > +    }
> > +
> > +  bfd_elf_add_obj_attr_int (stdoutput, OBJ_ATTR_GNU,
> > +			    Tag_GNU_MIPS_ABI_FP, fpabi);
> 
> Won't this overwrite an explicit .gnu_attribute?  Seems better to
> set it at the end if no explicit .gnu_attribute was given.

Darn it.  Yes, but I need to do it earlier still as I can't do it
at the end due to there being no way to distinguish between a file
without a .gnu_attribute and one with .gnu_attribute 4,0 after
assembly. I will have to add a hook for .gnu_attribute to run
file_mips_check_options and hand off to the generic code. i.e.

diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
index 5bb4f61..8def394 100644
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -1702,6 +1702,7 @@ static const pseudo_typeS mips_pseudo_table[] =
   {"insn", s_insn, 0},
   {"nan", s_nan, 0},
   {"module", s_module, 0},
+  {"gnu_attribute", s_mips_gnu_attribute, 0},

   /* Relatively generic pseudo-ops that happen to be used on MIPS
      chips.  */
@@ -15394,6 +15395,16 @@ s_module (int ignore ATTRIBUTE_UNUSED)
   demand_empty_rest_of_line ();
 }

+/* Handle the .gnu_attribute pseudo-op.  */
+
+static void
+s_mips_gnu_attribute (int ignore ATTRIBUTE_UNUSED)
+{
+  file_mips_check_options ();
+
+  obj_elf_vendor_attribute (OBJ_ATTR_GNU);
+}
+
 /* Handle the .abicalls pseudo-op.  I believe this is equivalent to
    .option pic2.  It means to generate SVR4 PIC calls.  */
=== end ===

> > @@ -5361,13 +5546,13 @@ 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.
> > +	 Force the constant into memory otherwise.  */
> > +      && (using_gprs
> > +	  || HAVE_64BIT_GPRS
> > +	  || ISA_HAS_MXHC1 (mips_opts.isa)
> > +	  || (HAVE_32BIT_FPRS && mips_opts.fp != 0))
> >        && ((data[0] == 0 && data[1] == 0)
> >  	  || (data[2] == 0 && data[3] == 0))
> >        && ((data[4] == 0 && data[5] == 0)
> 
> Sorry, but I can't get over the fact that HAVE_32BIT_FPRS &&
> mips_opts.fp != 0
> makes no conceptual sense :-)  Please do the FPR_SIZE thing first.

Done.

> > +/* Handle the .module pseudo-op.  */
> > +
> > +static void
> > +s_module (int ignore ATTRIBUTE_UNUSED)
> > +{
> > +  char *name = input_line_pointer, ch;
> > +
> > +  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)
> >      {
> > -      as_warn (_("tried to set unrecognized symbol: %s\n"), name);
> > +      if (!parse_code_option (name))
> > +	as_warn (_(".module used with unrecognized symbol: %s\n"), name);
> 
> I think this should be a hard error.  I know it isn't for .set,
> but there's no historical baggage for .module.
> 
> > @@ -17416,7 +17672,9 @@ mips_elf_final_processing (void)
> >      elf_elfheader (stdoutput)->e_flags |= EF_MIPS_NAN2008;
> >
> >    /* 32 bit code with 64 bit FP registers.  */
> > -  if (file_mips_opts.fp == 64 && ABI_NEEDS_32BIT_REGS (mips_abi))
> > +  fpabi = bfd_elf_get_obj_attr_int (stdoutput, OBJ_ATTR_GNU,
> > +				    Tag_GNU_MIPS_ABI_FP);
> > +  if (fpabi == Val_GNU_MIPS_ABI_FP_OLD_64)
> >      elf_elfheader (stdoutput)->e_flags |= EF_MIPS_FP64;
> >  }
> >  

Did you intend to comment on this?

> > @@ -17531,6 +17789,9 @@ md_obj_end (void)
> >    /* Check for premature end, nesting errors, etc.  */
> >    if (cur_proc_ptr)
> >      as_warn (_("missing .end at end of assembly"));
> > +
> > +  /* Just in case no code was emitted, do the consistency check.  */
> > +  file_mips_check_options ();
> >  }
> 
> Probably makes sense to keep this and the final fpabi check together.
> Having md_obj_end separate from md_mips_end serves no purpose these
> days anyway...

OK

> > +The floating-point ABI is inferred as follows.  If @samp{-msoft-
> float}
> > +has been used the module will be marked as soft-float.  The hard-
> float
> > +ABIs are then only inferred if a floating point instruction is seen.
> 
> No longer true.

Fixed.

Patch to follow after reworking tests.

Regards,
Matthew