[PATCHv3] Add support for O32 FPXX ABI

[Matthew Fortune]

> Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> > @@ -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?

Yes. The feeling is that the information is sufficiently valuable to be
made loadable for bare-metal. However, bare-metal users can and will do
anything they want regardless of the ABI. For a bare-metal user to locate
the .MIPS.abiflags section they will have to insert a symbol using their
linker script, so although the section is loadable there is no single
guaranteed way of locating it at runtime. This gives users the ability to
freely discard the section if they wish. To some extent the same is true
for linux (or other OSs) but the user would be taking the ABI into their
own hands, much like they would do for bare-metal.

Unless the structure grows significantly there is little reason to expect
any fallout from a bare-metal user who has now got 24 bytes more data that
they may not want. There are plans to put together recommendations for
bare-metal MIPS users to both cover these kind of details and standardize
on topics like semi-hosting so hopefully users will be better informed
in the coming months.

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

I have set everything up so that -msoft-float trumps all other options so
this code mirrors that by clarifying that -msoft-float won and has to be
removed. With that fixed the user may then also have to add -msingle-float.
I.e. It is not an error to have both soft-float and single-float as it
stands.

Reading through this code I can however see some inconsistencies between
what is reported as incompatible and what is reported as required. I suppose
changing the else-if branch of the above case to use fpabi_requires would
make some sense

    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_requires (fpabi, "singlefloat");
      break;

Similar for the FP_SOFT case:

    case Val_GNU_MIPS_ABI_FP_SOFT:
      if (!file_mips_opts.soft_float)
        fpabi_requires (fpabi, "softfloat");
      break;

It probably doesn't make much difference overall if I change this but what
do you think?

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

Yes, thanks.

> > @@ -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 have no major objection to this. I had structured it the way it was
to catch internal errors if the ABI inferring code was inconsistent
with the ABI checks. If the internals were correct then there should
be no warnings raised from inferred FP ABIs.

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

I'm still reviewing this too to ensure I will catch as many issues as
possible. I didn't expect it to be particularly review'able :-).

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

Good question and there isn't actually any need for this anymore. I had
it in due to sb1 automatically enabling MIPS3D and MDMX and I initially
included the ABI flags output in the expected output for these tests but
now ignore it and just check the GNU attribute alone. I've removed the
sb1 and r5900 exclusions.

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

done

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

I did have a thought on the ABI flags merging/checking logic. I have
essentially ignored the content of flags1 an flags2 currently as they
are entirely undefined at the moment. Do you think I should assert
them to be zero, allow them to be carried forward into the output if
the inputs match, or do nothing as now? I'm tempted to check that they
are zero.

Thanks for working through this again. Should I take this to mean you
are OK with the patch? Assuming that is the case then I will still hold
off committing for a short while so that I can do some more testing in
conjunction with the other parts of the toolchain. I would love another
review from someone who has not dug into the detail of this patch but
I'm not sure anyone will have the same will-power that you do for reading
such complex changes. Volunteers?

Regards,
Matthew