From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from angie.orcam.me.uk (angie.orcam.me.uk [78.133.224.34]) by sourceware.org (Postfix) with ESMTP id 5B5F43858401 for ; Thu, 1 Jun 2023 15:35:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5B5F43858401 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=orcam.me.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=orcam.me.uk Received: by angie.orcam.me.uk (Postfix, from userid 500) id 46D3392009C; Thu, 1 Jun 2023 17:35:07 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by angie.orcam.me.uk (Postfix) with ESMTP id 3F89492009B; Thu, 1 Jun 2023 16:35:07 +0100 (BST) Date: Thu, 1 Jun 2023 16:35:07 +0100 (BST) From: "Maciej W. Rozycki" To: Ying Huang cc: libc-alpha@sourceware.org, yunqiang.su@oss.cipunited.com Subject: Re: [PATCH v3] MIPS: Sync elf.h from binutils In-Reply-To: <20230516094024.224161-1-ying.huang@oss.cipunited.com> Message-ID: References: <20230516094024.224161-1-ying.huang@oss.cipunited.com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-1169.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_STATUS,KAM_INFOUSMEBIZ,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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