Message ID | 56ade7ab382535b83feb14058df9a84aad0dcaac.1591201405.git.alistair.francis@wdc.com |
---|---|
State | Committed |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 95FD8388C00E; Wed, 3 Jun 2020 16:34:17 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 95FD8388C00E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1591202057; bh=/1wI/axjrII5F4yaOX9QVTmJF06vtLSmbgAdZh/Y0HQ=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=foTvuB5OqFNvdS3yCVMl21sTKdxuMAkJL1qHlwnCsjsOveLj7R47Cw3FC/G4ij1sH +cv6qzKDrQQUYMWQIq+TJFeBnA/HshS8SApVxF6DbmJYAmwOomYrfoj/cE1QKcuyeH FzNDUX22ezPB50KkcBtBaMYok908poifZJ0V1mYk= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from esa3.hgst.iphmx.com (esa3.hgst.iphmx.com [216.71.153.141]) by sourceware.org (Postfix) with ESMTPS id 66F81388B018 for <libc-alpha@sourceware.org>; Wed, 3 Jun 2020 16:34:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 66F81388B018 IronPort-SDR: EQvQl/C3OXlX0LXIe0d1nRA/PNteOrIM5LeHkfbfq3WZ/ydFE1SEWdHCjJMKGWBdFurYaqnDF4 44Q9q5tEPYIqWQ+q8YoFXIpxtPwW0jm6IrE3FGTEDhRmrQTGHmpO+HVfc1HK4ng0HiEkANFvBb M2aZWj6WPqMHLdJKOsb0kCoiFowcSG7GiMB/ygTtnwoDsuJPUQcWtEarCglC1s1knZ6a/Pp1lq W6wtogyGebrBGpcXPMeeA8oA6cAg0tOq3WnprPuS+Pg9PxIJJYFAb0gF2hK8ADq3JKXOVPA92/ 3OA= X-IronPort-AV: E=Sophos;i="5.73,468,1583164800"; d="scan'208";a="143452226" Received: from uls-op-cesaip01.wdc.com (HELO uls-op-cesaep01.wdc.com) ([199.255.45.14]) by ob1.hgst.iphmx.com with ESMTP; 04 Jun 2020 00:34:15 +0800 IronPort-SDR: GLaJrhLaQo6ibui7kgRKHSL4mQt3QlDjeKsaKKVgfuj3wZDHTnndko2AcoZS/XA/ukRwVVVRL+ FCSaqa/sDVV/dYmw844ezK9San/yiqWKY= Received: from uls-op-cesaip02.wdc.com ([10.248.3.37]) by uls-op-cesaep01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Jun 2020 09:23:53 -0700 IronPort-SDR: 54nVoRCkpNqDb2epxSo5e3k724pOTfbBk18zQaRiepDtaYqCaAHN5NXtFrgh1PziiJvxFPhxTD pr2ydQEkXNuw== WDCIronportException: Internal Received: from cne220230.ad.shared (HELO risc6-mainframe.hgst.com) ([10.86.57.144]) by uls-op-cesaip02.wdc.com with ESMTP; 03 Jun 2020 09:34:14 -0700 To: libc-alpha@sourceware.org Subject: [PATCH v2 02/18] RISC-V: Define __NR_* as __NR_*_time64/64 for 32-bit Date: Wed, 3 Jun 2020 09:25:29 -0700 Message-Id: <56ade7ab382535b83feb14058df9a84aad0dcaac.1591201405.git.alistair.francis@wdc.com> X-Mailer: git-send-email 2.26.2 In-Reply-To: <cover.1591201405.git.alistair.francis@wdc.com> References: <cover.1591201405.git.alistair.francis@wdc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-16.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, 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 <libc-alpha.sourceware.org> List-Unsubscribe: <http://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <http://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Alistair Francis via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Alistair Francis <alistair.francis@wdc.com> Cc: alistair.francis@wdc.com Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
glibc port for 32-bit RISC-V (RV32)
|
|
Commit Message
Alistair Francis
June 3, 2020, 4:25 p.m. UTC
--- sysdeps/unix/sysv/linux/riscv/sysdep.h | 61 ++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)
Comments
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 <sysdeps/unix/sysdep.h> This file is weird, as it includes <sysdeps/unix/sysdep.h> twice, first time indirectly via <sysdeps/unix/sysv/linux/generic/sysdep.h> at the top, and then second time here. So I think this second inclusion can be removed (along with the preceding inclusion of <errno.h>, as it does not appear to change anything), and the following conditional moved to the top, just after the inclusion of <tls.h>. Oddly <sysdeps/unix/sysdep.h> 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
On 07/07/2020 21:09, Maciej W. Rozycki via Libc-alpha wrote: > 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 <sysdeps/unix/sysdep.h> > > This file is weird, as it includes <sysdeps/unix/sysdep.h> twice, first > time indirectly via <sysdeps/unix/sysv/linux/generic/sysdep.h> at the top, > and then second time here. So I think this second inclusion can be > removed (along with the preceding inclusion of <errno.h>, as it does not > appear to change anything), and the following conditional moved to the > top, just after the inclusion of <tls.h>. Oddly <sysdeps/unix/sysdep.h> > has not been protected against multiple inclusion, but its contents do not > trigger compilation warnings if processed more than once. This multiple inclusion is done internally in some places so the file that wants to override a definition would first include the generic definitions and then #undef/#define the new one (kernel-features.h still does it). I think this is kind of confusing, but this is how sysdep.h is currently organized. We have been changing bit per bit, but there is a lot of place where inclusing is done without guards. > > 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. I think the correct way to how ARC port is doing which is: 1. Add any required syscall suppression for 32-bit off_t/time_t on fixup-asm-unistd.h so arch-syscall.h will have only the required definitions. This might be the case for riscv32 since its kernel ABI only supports 64-bit off_t/time_t. 2. Regenerate arch-syscall.h if it were the case. 3. On sysdep.h redefine only the syscall where the generic implementation still does not have actual 64-bit time_t support: /* "workarounds" for generic code needing to handle 64-bit time_t. */ /* Fix sysdeps/unix/sysv/linux/clock_getcpuclockid.c. */ #define __NR_clock_getres __NR_clock_getres_time64 /* Fix sysdeps/nptl/lowlevellock-futex.h. */ #define __NR_futex __NR_futex_time64 [...] (with proper guards it should require the #ifndef/#define) 4. Add a comment it is a workaround to handle 64-bit time_t and on each #define comment for which implementation it intends to.
On Tue, Jul 7, 2020 at 5:09 PM Maciej W. Rozycki via Libc-alpha <libc-alpha@sourceware.org> wrote: > > 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 Fixed. > > and then maybe explain in a little more details in the change description. Done. > > 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 <sysdeps/unix/sysdep.h> > > This file is weird, as it includes <sysdeps/unix/sysdep.h> twice, first > time indirectly via <sysdeps/unix/sysv/linux/generic/sysdep.h> at the top, > and then second time here. So I think this second inclusion can be > removed (along with the preceding inclusion of <errno.h>, as it does not > appear to change anything), and the following conditional moved to the > top, just after the inclusion of <tls.h>. Oddly <sysdeps/unix/sysdep.h> > 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). I have split out these changes into a seperate patch. > > > +/* 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. I have changed this based on what Adhemerval said (the same as the ARC port). Alistair > > Maciej
On Wed, Jul 8, 2020 at 10:08 AM Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote: > > > > On 07/07/2020 21:09, Maciej W. Rozycki via Libc-alpha wrote: > > 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 <sysdeps/unix/sysdep.h> > > > > This file is weird, as it includes <sysdeps/unix/sysdep.h> twice, first > > time indirectly via <sysdeps/unix/sysv/linux/generic/sysdep.h> at the top, > > and then second time here. So I think this second inclusion can be > > removed (along with the preceding inclusion of <errno.h>, as it does not > > appear to change anything), and the following conditional moved to the > > top, just after the inclusion of <tls.h>. Oddly <sysdeps/unix/sysdep.h> > > has not been protected against multiple inclusion, but its contents do not > > trigger compilation warnings if processed more than once. > > This multiple inclusion is done internally in some places so the file > that wants to override a definition would first include the generic > definitions and then #undef/#define the new one (kernel-features.h > still does it). I'm not sure if this is required. I split out the clean up into a seperate patch and I don't see any regressions. > > I think this is kind of confusing, but this is how sysdep.h is currently > organized. We have been changing bit per bit, but there is a lot of > place where inclusing is done without guards. > > > > > 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. > > I think the correct way to how ARC port is doing which is: > > 1. Add any required syscall suppression for 32-bit off_t/time_t on > fixup-asm-unistd.h so arch-syscall.h will have only the required > definitions. This might be the case for riscv32 since its kernel > ABI only supports 64-bit off_t/time_t. > > 2. Regenerate arch-syscall.h if it were the case. > > 3. On sysdep.h redefine only the syscall where the generic implementation > still does not have actual 64-bit time_t support: > > /* "workarounds" for generic code needing to handle 64-bit time_t. */ > > /* Fix sysdeps/unix/sysv/linux/clock_getcpuclockid.c. */ > #define __NR_clock_getres __NR_clock_getres_time64 > /* Fix sysdeps/nptl/lowlevellock-futex.h. */ > #define __NR_futex __NR_futex_time64 > [...] > > (with proper guards it should require the #ifndef/#define) > > 4. Add a comment it is a workaround to handle 64-bit time_t > and on each #define comment for which implementation it intends to. Thanks! I have updated this to be similar to the ARC port. I also paraphrased the points above in the commit message (I hope that is ok). Alistair
[Added cc's back.] On Wed, 8 Jul 2020, Adhemerval Zanella 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 <sysdeps/unix/sysdep.h> > > > > This file is weird, as it includes <sysdeps/unix/sysdep.h> twice, first > > time indirectly via <sysdeps/unix/sysv/linux/generic/sysdep.h> at the top, > > and then second time here. So I think this second inclusion can be > > removed (along with the preceding inclusion of <errno.h>, as it does not > > appear to change anything), and the following conditional moved to the > > top, just after the inclusion of <tls.h>. Oddly <sysdeps/unix/sysdep.h> > > has not been protected against multiple inclusion, but its contents do not > > trigger compilation warnings if processed more than once. > > This multiple inclusion is done internally in some places so the file > that wants to override a definition would first include the generic > definitions and then #undef/#define the new one (kernel-features.h > still does it). > > I think this is kind of confusing, but this is how sysdep.h is currently > organized. We have been changing bit per bit, but there is a lot of > place where inclusing is done without guards. Fair enough, however the RISC-V port does not appear to make use of it as the removal of the second inclusion does not change anything (I actually double-checked). Maciej
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 <sysdeps/unix/sysdep.h> +#if __riscv_xlen == 32 +/* 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 + +# ifndef __NR_rt_sigtimedwait +# define __NR_rt_sigtimedwait __NR_rt_sigtimedwait_time64 +# endif + +# ifndef __NR_ppoll +# define __NR_ppoll __NR_ppoll_time64 +# endif + +# ifndef __NR_utimensat +# define __NR_utimensat __NR_utimensat_time64 +# endif + +# ifndef __NR_pselect6 +# define __NR_pselect6 __NR_pselect6_time64 +# endif + +# ifndef __NR_recvmmsg +# define __NR_recvmmsg __NR_recvmmsg_time64 +# endif + +# ifndef __NR_semtimedop +# define __NR_semtimedop __NR_semtimedop_time64 +# endif + +# ifndef __NR_mq_timedreceive +# define __NR_mq_timedreceive __NR_mq_timedreceive_time64 +# endif + +# ifndef __NR_mq_timedsend +# define __NR_mq_timedsend __NR_mq_timedsend_time64 +# endif + +# ifndef __NR_clock_getres +# define __NR_clock_getres __NR_clock_getres_time64 +# endif + +# ifndef __NR_timerfd_settime +# define __NR_timerfd_settime __NR_timerfd_settime64 +# endif + +# ifndef __NR_timerfd_gettime +# define __NR_timerfd_gettime __NR_timerfd_gettime64 +# endif + +# ifndef __NR_sched_rr_get_interval +# define __NR_sched_rr_get_interval __NR_sched_rr_get_interval_time64 +# endif + +# ifndef __NR_clock_adjtime +# define __NR_clock_adjtime __NR_clock_adjtime64 +# endif +#endif /* __riscv_xlen == 32 */ + #undef SYS_ify #define SYS_ify(syscall_name) __NR_##syscall_name