This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCHv3] Add support for O32 FPXX ABI


Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> +    case E_MIPS_ARCH_32R2:
> +      abiflags->isa_level = 32;
> +      if (abiflags->isa_rev < 2)
> +	abiflags->isa_rev = 2;
> +      break;
...
> +    case E_MIPS_ARCH_64R2:
> +      abiflags->isa_level = 64;
> +      if (abiflags->isa_rev < 2)
> +	abiflags->isa_rev = 2;

Please add a comment saying that this is to handle r3 and r5, which don't
have their own header flag.

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

We need to be C90-compatible, so no mid-function variable definitions.

> @@ -14840,14 +15256,163 @@ _bfd_mips_fp_abi_string (int fp)
>      case Val_GNU_MIPS_ABI_FP_SOFT:
>        return "-msoft-float";
>  
> +    case Val_GNU_MIPS_ABI_FP_OLD_64:
> +      return "-mips32r2 -mfp64 (12 callee-saved)";

Needs to be translatable (via _(...)) now that it has some English in it.

> +static void
> +print_mips_isa_ext (FILE *file, unsigned int isa_ext)
> +{
> +  switch (isa_ext)
> +    {
> +    case 0:
> +      fputs (_("None"), file);
> +      break;
> +    case AFL_EXT_XLR:
> +      fputs ("RMI XLR instruction", file);

This still sounds a bit odd when compared with the others.  Maybe just
"RMI XLR"?

> +  if (mask == 0)
> +    fputs (_("\n\tNone"), stdout);

Let's make it:

  fprintf ("\n\t%s", _("None"), stdout);

to cut down on the number of translations needed.  Same for the other
case where this was used.

> @@ -3612,6 +3615,12 @@ md_begin (void)
>  	}
>        }
>  
> +    sec = subseg_new (".MIPS.abiflags", (subsegT) 0);
> +    bfd_set_section_flags (stdoutput, sec,
> +			   SEC_READONLY | SEC_DATA | SEC_ALLOC | SEC_LOAD);
> +    bfd_set_section_alignment (stdoutput, sec, 3);
> +    mips_flags_frag = frag_more (sizeof (Elf_External_ABIFlags_v0));
> +

Just to make sure I'm following: does this mean that the ABI is now
committed to making .MIPS.abiflags loadable even for bare-metal?

> @@ -3635,6 +3644,83 @@ md_begin (void)
>      init_vr4120_conflicts ();
>  }
>  
> +static inline void
> +fpabi_incompatible_with (int fpabi, const char *what)
> +{
> +  as_warn (_(".gnu_attribute %d,%d is incompatible with `%s'"),
> +	   Tag_GNU_MIPS_ABI_FP, fpabi, what);
> +}
> +
> +static inline void
> +fpabi_requires (int fpabi, const char *what)
> +{
> +  as_warn (_(".gnu_attribute %d,%d requires `%s'"),
> +	   Tag_GNU_MIPS_ABI_FP, fpabi, what);
> +}
> +
> +static void
> +check_fpabi (int fpabi)
> +{
> +  bfd_boolean needs_check = FALSE;
> +  /* Check -mabi and register sizes.  */

This comment really describes the whole function so should go before
the prototype.

> +    case Val_GNU_MIPS_ABI_FP_SINGLE:
> +      if (file_mips_opts.soft_float)
> +	fpabi_incompatible_with (fpabi, "softfloat");
> +      else if (!file_mips_opts.single_float)
> +	fpabi_incompatible_with (fpabi, "doublefloat");
> +      break;

Could this just be:

    case Val_GNU_MIPS_ABI_FP_SINGLE:
      if (!file_mips_opts.single_float)
	fpabi_requires (fpabi, "singlefloat");
      break;

> +	      if (ISA_HAS_MXHC1 (mips_opts.isa))
> +	        macro_build (NULL, "mthc1", "t,G", AT, op[0]);
> +	      else if (mips_opts.fp != 32)
> +		as_bad (_("Unable to generate `%s' compliant code "
> +			  "without mthc1"),
> +			(mips_opts.fp == 64) ? "fp64" : "fpxx");

FPR_SIZE rather than mips_opts.fp?

> @@ -18402,10 +18633,52 @@ mips_convert_symbolic_attribute (const char *name)
>  void
>  md_mips_end (void)
>  {
> +  int fpabi = Val_GNU_MIPS_ABI_FP_ANY;
> +
>    mips_emit_delays ();
>    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 ();
> +
> +  /* Set a floating-point ABI if the user did not.  */
> +  if (!obj_elf_seen_attribute (OBJ_ATTR_GNU, Tag_GNU_MIPS_ABI_FP))
> +    {
> +      /* 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:
> +	      if (file_mips_opts.gp == 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.gp == 32)
> +		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);
> +    }
> +
> +  /* Perform consistency checks on the floating-point ABI.  */
> +  fpabi = bfd_elf_get_obj_attr_int (stdoutput, OBJ_ATTR_GNU,
> +				    Tag_GNU_MIPS_ABI_FP);
> +  if (fpabi != Val_GNU_MIPS_ABI_FP_ANY)
> +    check_fpabi (fpabi);

Not sure we should run check_fpabi on the implicitly-chosen ABI,
since it would produce messages about .gnu_attribute when the user
hasn't used it.  Maybe:

  if (obj_elf_seen_attribute (OBJ_ATTR_GNU, Tag_GNU_MIPS_ABI_FP))
    {
      /* Perform consistency checks on the floating-point ABI.  */
      fpabi = bfd_elf_get_obj_attr_int (stdoutput, OBJ_ATTR_GNU,
					Tag_GNU_MIPS_ABI_FP);
      if (fpabi != Val_GNU_MIPS_ABI_FP_ANY)
	check_fpabi (fpabi);
    }
  else
    {
      ...
    }

I find testsuite stuff almost impossible to review, but it certainly
looks impressively thorough, thanks.  As far as the .exp bits go:

> +    run_dump_test_arches "attr-gnu-4-0" "-32" [mips_arch_list_matching mips1]
> +    run_dump_test_arches "attr-gnu-4-0" "-64" [mips_arch_list_matching mips3 !sb1]

Please keep to 80 columns by e.g.

    run_dump_test_arches "attr-gnu-4-0" "-64" \
				[mips_arch_list_matching mips3 !sb1]

Why the sb1 exclusion though?

Tests in ld-mips-elf don't need:

> +#target: mips*-*-*

Rather than:

> +#warning: warning: cannot find entry symbol __start; defaulting to .*

let's just add something like -e 0 to the command line.

Please post the ld-lib.exp part as a separate patch so that others
are more likely to see it.  I like the idea though.  Apart from the
some 80-column violations it looks good to me.

The rest looks OK to me too.  TBH I've read through versions of this
patch so many times that I'm probably not going to pick up anything
useful (compared to someone coming to it fresh).

Thanks,
Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]