Re: [PATCHv3] Add support for O32 FPXX ABI

[Richard Sandiford <rdsandiford at googlemail dot com>]

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

OK.

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

Both changes looks like improvements to me.

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

But the point was that it would be flagged up as a user error rather
than an internal error.  Either we should raise it as an internal error
or we should go with the above.

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

I don't really mind.  Maybe we could define flags1 as being an ORed field
and leave flags2 as something that can only be processed if you know
what it means (error on nonzero, like you say)?

> Thanks for working through this again. Should I take this to mean you
> are OK with the patch?

Yeah, with the changes discussed, I think it's good to go in.

Thanks,
Richard