Re: [PATCH v3] MIPS: Sync elf.h from binutils

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Tue, 16 May 2023, Ying Huang wrote:

> 1.Add new MIPS related declarations, specifically the following:
> relocation types, machine flags, section type names,
> GNU attribute tags, values of Tag_GNU_MIPS_ABI_FP.
> On mips64, up to three reloc types may be specified per r_info,
> by the fields r_type, r_type2, and r_type3, so add new macros
> to get reloc type for mips64.
> 2.update sysdeps/mips/dl-machine-reject-phdr.h

 I suggest rephrasing and reformatting the commit description a little for 
clarity and to follow the GNU Coding Standards.  How about:

1. Add new definitions for the MIPS target, specifically: relocation 
   types, machine flags, section type names, and object attribute tags 
   and values.  On MIPS64, up to three relocations may be specified 
   within r_info, by the r_type, r_type2, and r_type3 fields, so add new 
   macros to get the respective reloc types for MIPS64.

2. Update sysdeps/mips/dl-machine-reject-phdr.h for 2008 NaN support.

?  However I think the two parts need to be separate patches, as the first 
part is a mechanical update for bits we've been missing and the second one 
is a semantics change.

> diff --git a/elf/elf.h b/elf/elf.h
> index 94ca23c1bb..e1a6ace066 100644
> --- a/elf/elf.h
> +++ b/elf/elf.h
> @@ -678,6 +678,9 @@ typedef Elf64_Xword	Elf64_Relr;
>  
>  #define ELF64_R_SYM(i)			((i) >> 32)
>  #define ELF64_R_TYPE(i)			((i) & 0xffffffff)
> +#define ELF64_MIPS_R_TYPE(i)		((i) & 0xff)
> +#define ELF64_MIPS_R_TYPE2(i)           (((i) >> 8) & 0xff)
> +#define ELF64_MIPS_R_TYPE3(i)           (((i) >> 16) & 0xff)

 There's something odd with indentation here.  Please substitute spaces 
with tabs throughout your additions, making sure the horizontal alignment 
of the right-hand side is correct (it might be wrong in binutils here or 
there, but it should be fixed there rather than propagated here).

> diff --git a/sysdeps/mips/dl-machine-reject-phdr.h b/sysdeps/mips/dl-machine-reject-phdr.h
> index 104b590661..edea869c46 100644
> --- a/sysdeps/mips/dl-machine-reject-phdr.h
> +++ b/sysdeps/mips/dl-machine-reject-phdr.h
> @@ -139,7 +141,8 @@ static const struct abi_req reqs[Val_GNU_MIPS_ABI_FP_MAX + 1] =
>       {false, false, false, false, false}, /* old-FP64 */
>       {false, false, true,  true,  true},  /* FPXX */
>       {false, false, false, true,  false}, /* FP64 */
> -     {false, false, false, true,  true}}; /* FP64A */
> +     {false, false, false, true,  true},  /* FP64A */
> +     {false, false, true,  true,  true}}; /* NAN2008 */

 It doesn't make sense to me.  The use of Val_GNU_MIPS_ABI_FP_NAN2008 has 
been superseded (in favour to EF_MIPS_NAN2008, set independently from any 
FP ABI so that you can make combinations), e.g. GAS has this piece:

    case Val_GNU_MIPS_ABI_FP_NAN2008:
      /* Silently ignore compatibility value.  */
      break;

And LD issues a warning about an unknown FP ABI whenever linking a set of 
modules involving ones marked with this value and ones marked with any 
other value except for Val_GNU_MIPS_ABI_FP_ANY both at a time.

 I think we should also reject such combinations.  It surely makes no 
sense to consider 2008 NaN incompatible and compatible with single and 
double float respectively, as your change proposes.

 Have you actually seen this attribute value in the wild though (e.g. by 
triggering the "uses unknown FP ABI" error here)?  And where did you get 
this piece of code from anyway?

  Maciej