From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.hgst.iphmx.com (esa3.hgst.iphmx.com [216.71.153.141]) by sourceware.org (Postfix) with ESMTPS id 665013858D35 for ; Wed, 8 Jul 2020 00:09:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 665013858D35 IronPort-SDR: OQt96svYbkgoQpysXumCJNhjWfTtD9Ft5ccWvXMrG6ICqMYiNf1G+H6OMx6AfK/P0O9m+iw4u7 2NkYr/59M1gv4AIV8u50cefGK84JyRViyBuSPVIyVkAJnHS7hIU1DshpieXNIJlmJHGp7C4/Rn rF7upF127EQEcGTGHCEaUt+F70MyLcVe0O6gTnlbK3Q0Ua5/te90vYBB6Wh7fCk0Pp5/Deqxox OZONjw/HboIlbjzPgspqWt8QvNmEMtlNmmKzqi2RNjq+wAkWpcfh+P6LBSTqMCQvfyhzcoaNso /H8= X-IronPort-AV: E=Sophos;i="5.75,325,1589212800"; d="scan'208";a="146186954" Received: from h199-255-45-15.hgst.com (HELO uls-op-cesaep02.wdc.com) ([199.255.45.15]) by ob1.hgst.iphmx.com with ESMTP; 08 Jul 2020 08:09:05 +0800 IronPort-SDR: zzoLc1Gv6/Pa0gJgKPimJSmRkhiSJZROuX6mbuTncJn87iuTgzupljS8Id4/oW6v3Z1AlT5mg+ 1FjVH8rS8aTF4MEeGFxsETk2mXm4+KU/w= Received: from uls-op-cesaip02.wdc.com ([10.248.3.37]) by uls-op-cesaep02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Jul 2020 16:57:11 -0700 IronPort-SDR: Q2GDHewa2X6SyTguRZLUKYPap8P0yCMVvA1zDsG6ma5sAS38ageJo+RbS5qbuoGHqeonCBXTxM 6FxDPPBKboSA== WDCIronportException: Internal Received: from unknown (HELO redsun52) ([10.149.66.28]) by uls-op-cesaip02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Jul 2020 17:09:05 -0700 Date: Wed, 8 Jul 2020 01:09:02 +0100 (BST) From: "Maciej W. Rozycki" To: Alistair Francis cc: libc-alpha@sourceware.org Subject: Re: [PATCH v2 02/18] RISC-V: Define __NR_* as __NR_*_time64/64 for 32-bit In-Reply-To: <56ade7ab382535b83feb14058df9a84aad0dcaac.1591201405.git.alistair.francis@wdc.com> Message-ID: References: <56ade7ab382535b83feb14058df9a84aad0dcaac.1591201405.git.alistair.francis@wdc.com> User-Agent: Alpine 2.21 (LFD 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jul 2020 00:09:08 -0000 Alistair, I think the change heading is too cryptic and does not express the intent of the change well enough. How about: RISC-V: Use 64-bit-time syscall numbers with the 32-bit port and then maybe explain in a little more details in the change description. On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote: > diff --git a/sysdeps/unix/sysv/linux/riscv/sysdep.h b/sysdeps/unix/sysv/linux/riscv/sysdep.h > index 83e4adf6a2..aa61e8b04d 100644 > --- a/sysdeps/unix/sysv/linux/riscv/sysdep.h > +++ b/sysdeps/unix/sysv/linux/riscv/sysdep.h > @@ -116,6 +116,67 @@ > > #include This file is weird, as it includes twice, first time indirectly via at the top, and then second time here. So I think this second inclusion can be removed (along with the preceding inclusion of , as it does not appear to change anything), and the following conditional moved to the top, just after the inclusion of . Oddly has not been protected against multiple inclusion, but its contents do not trigger compilation warnings if processed more than once. The two #ifdef/#ifndef __ASSEMBLER__ conditionals will then become adjacent and can be merged into a single #ifdef/#else one. This clean-up would probably better be made as a separate preceding change. > +#if __riscv_xlen == 32 I think using __WORDSIZE here would be more consistent with the rest of our code (we do use `__riscv_xlen' in a couple of places, but I think they ought to be cleaned up). > +/* Define the __NR_futex as __NR_futex64 as RV32 doesn't have a > + * __NR_futex syscall. > + */ > +# ifndef __NR_futex > +# define __NR_futex __NR_futex_time64 > +# endif The comment does not match the code. I think it makes no sense to comment on individual entries as they all repeat the same pattern and the same purpose, so an introductory comment covering them all at the beginning of the conditional would be better instead. I suppose you can then reuse it for the change description too. > +# ifndef __NR_clock_adjtime > +# define __NR_clock_adjtime __NR_clock_adjtime64 > +# endif > +#endif /* __riscv_xlen == 32 */ Since you have multiple inner conditionals separated by an empty line each I think it will make sense to have an empty line as well between the final one and the closing of the outer conditional. Likewise at the beginning. Maciej