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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> I realised that the values of isa_ext in objects are largely
> irrelevant as they are covered by the bfd_mach so I simple determine
> what the correct isa_ext is from bfd_mach irrespective of what was
> in the input bfd's .MIPS.abiflags section.

That sounds fine for now.  The idea with having the field in the
abiflags section was to give us more bits, but there's less pressure
on EF_MIPS_MACH than most other parts of the e_flags.

> 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.

> 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.

> @@ -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)

> +      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.

> +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").

> +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.

> +      fputs ("Unknown", file);
> +    }

_(...) here too.  Would prefer an explicit break.

Same comments for the readelf version.

> @@ -3641,6 +3655,175 @@ 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) ? "not `-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) ? "not `-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 %s"),
> +		 Tag_GNU_MIPS_ABI_FP, fpabi, "`single-float'");
> +      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 %s"),
> +		 Tag_GNU_MIPS_ABI_FP, fpabi, "`soft-float'");
> +      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);

No, the "not" and quotes should be in the format string, so that they
get translated.  I think this would be cleaner if you had helper functions
that print the two main error messages.  E.g.:

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);

> +  /* 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.

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

> +/* 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.

> @@ -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.

> +/* 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;
>  }
>  
> @@ -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...

> +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.

Looks good otherwise.

Thanks,
Richard