From: Alistair Francis <alistair23@gmail.com>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: GNU C Library <libc-alpha@sourceware.org>
Subject: Re: [PATCH v2 02/18] RISC-V: Define __NR_* as __NR_*_time64/64 for 32-bit
Date: Thu, 9 Jul 2020 10:14:18 -0700 [thread overview]
Message-ID: <CAKmqyKOZQ+3Xdwx0pxyJ2hSih4zuJ9HSZzv-F-9Xy5tULNvtFg@mail.gmail.com> (raw)
In-Reply-To: <b915968e-da95-3e54-ef43-150559405059@linaro.org>
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
next prev parent reply other threads:[~2020-07-09 17:24 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-03 16:25 [PATCH v2 00/18] glibc port for 32-bit RISC-V (RV32) Alistair Francis
2020-06-03 16:25 ` [PATCH v2 01/18] RISC-V: Use 64-bit time_t and off_t for RV32 and RV64 Alistair Francis
2020-07-07 22:06 ` Maciej W. Rozycki
2020-07-10 15:27 ` Alistair Francis
2020-06-03 16:25 ` [PATCH v2 02/18] RISC-V: Define __NR_* as __NR_*_time64/64 for 32-bit Alistair Francis
2020-07-08 0:09 ` Maciej W. Rozycki
2020-07-08 17:08 ` Adhemerval Zanella
2020-07-09 17:14 ` Alistair Francis [this message]
2020-07-16 0:23 ` Maciej W. Rozycki
2020-07-09 17:10 ` Alistair Francis
2020-06-03 16:25 ` [PATCH v2 03/18] RISC-V: Add support for 32-bit vDSO calls Alistair Francis
2020-07-08 1:01 ` Maciej W. Rozycki
2020-07-08 18:17 ` Alistair Francis
2020-06-03 16:25 ` [PATCH v2 04/18] RISC-V: Support dynamic loader for the 32-bit Alistair Francis
2020-07-08 1:35 ` Maciej W. Rozycki
2020-06-03 16:25 ` [PATCH v2 05/18] RISC-V: Add path of library directories " Alistair Francis
2020-07-08 18:42 ` Maciej W. Rozycki
2020-07-09 17:03 ` Alistair Francis
2020-06-03 16:25 ` [PATCH v2 06/18] RISC-V: Add arch-syscall.h for RV32 Alistair Francis
2020-07-08 19:33 ` Maciej W. Rozycki
2020-06-03 16:25 ` [PATCH v2 07/18] RISC-V: nptl: update default pthread-offsets.h Alistair Francis
2020-07-09 0:14 ` Maciej W. Rozycki
2020-07-09 11:47 ` Adhemerval Zanella
2020-07-15 19:23 ` Maciej W. Rozycki
2020-08-10 17:34 ` Alistair Francis
2020-06-03 16:25 ` [PATCH v2 08/18] riscv32: Add an architecture ipctypes.h Alistair Francis
2020-07-09 2:46 ` Maciej W. Rozycki
2020-07-09 11:36 ` Adhemerval Zanella
2020-06-03 16:25 ` [PATCH v2 09/18] RISC-V: The ABI implementation for 32-bit Alistair Francis
2020-07-09 23:33 ` Maciej W. Rozycki
2020-07-10 16:45 ` Alistair Francis
2020-07-11 1:24 ` Maciej W. Rozycki
2020-08-10 21:29 ` Alistair Francis
2020-08-27 19:43 ` Adhemerval Zanella
2020-09-25 23:03 ` Alistair Francis
2020-06-03 16:25 ` [PATCH v2 10/18] RISC-V: Hard float support " Alistair Francis
2020-07-11 0:49 ` Maciej W. Rozycki
2020-07-11 15:49 ` Alistair Francis
2020-07-11 22:13 ` Maciej W. Rozycki
2020-07-12 15:34 ` Alistair Francis
2020-07-12 22:10 ` Maciej W. Rozycki
2020-08-27 18:36 ` Alistair Francis
2020-06-03 16:25 ` [PATCH v2 11/18] RISC-V: Add ABI lists Alistair Francis
2020-07-12 20:54 ` Maciej W. Rozycki
2020-07-13 16:14 ` Alistair Francis
2020-06-03 16:25 ` [PATCH v2 12/18] RISC-V: Add the RV32 libm-test-ulps Alistair Francis
2020-06-03 17:34 ` Joseph Myers
2020-06-05 4:01 ` Alistair Francis
2020-06-03 16:26 ` [PATCH v2 13/18] RISC-V: Fix llrint and llround missing exceptions on RV32 Alistair Francis
2020-06-03 16:26 ` [PATCH v2 14/18] RISC-V: Build Infastructure for 32-bit Alistair Francis
2020-06-03 16:26 ` [PATCH v2 15/18] riscv32: Specify the arch_minimum_kernel as 5.4 Alistair Francis
2020-06-03 16:26 ` [PATCH v2 16/18] RISC-V: Add rv32 path to RTLDLIST in ldd Alistair Francis
2020-06-03 16:26 ` [PATCH v2 17/18] Documentation for the RISC-V 32-bit port Alistair Francis
2020-06-03 16:26 ` [PATCH v2 18/18] Add RISC-V 32-bit target to build-many-glibcs.py Alistair Francis
2020-06-15 22:37 ` [PATCH v2 00/18] glibc port for 32-bit RISC-V (RV32) Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKmqyKOZQ+3Xdwx0pxyJ2hSih4zuJ9HSZzv-F-9Xy5tULNvtFg@mail.gmail.com \
--to=alistair23@gmail.com \
--cc=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).