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

["Ying Huang" <ying.huang@oss.cipunited.com>]

[-- Attachment #1: Type: text/plain, Size: 3964 bytes --]

Hi Maciej,

在 2023/6/1 23:35, Maciej W. Rozycki 写道:
> 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

I did not see error "unknown FP ABI" and update this file for considering to avoid this error.

Thannks for your suggestion and I would fix the whole code alignment problem and rewrite commit message.

And submit patch v4 that only contains sync elf.h file.


Thanks,

Ying