From: Alistair Francis via Libc-alpha <libc-alpha@sourceware.org>
To: "Maciej W. Rozycki" <macro@wdc.com>
Cc: GNU C Library <libc-alpha@sourceware.org>,
Alistair Francis <alistair.francis@wdc.com>
Subject: Re: [PATCH v2 09/18] RISC-V: The ABI implementation for 32-bit
Date: Fri, 10 Jul 2020 09:45:10 -0700 [thread overview]
Message-ID: <CAKmqyKMH9P2hM4XF954Nxox_TZ1vHLR_9fAapRFHHHFL7htM3Q@mail.gmail.com> (raw)
In-Reply-To: <alpine.LFD.2.21.2007091330151.24175@redsun52.ssa.fujisawa.hgst.com>
On Thu, Jul 9, 2020 at 4:33 PM Maciej W. Rozycki via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:
>
> > This patch adds the ABI implementation about 32 bit version. It contains
> > the Linux-specific and RISC-V architecture code, I've collected here.
>
> I think this might be a little better worded, and perhaps discuss briefly
> at least some choices made.
>
> > diff --git a/sysdeps/riscv/bits/wordsize.h b/sysdeps/riscv/bits/wordsize.h
> > index faccc71828..ee430d9036 100644
> > --- a/sysdeps/riscv/bits/wordsize.h
> > +++ b/sysdeps/riscv/bits/wordsize.h
> > @@ -25,5 +25,7 @@
> > #if __riscv_xlen == 64
> > # define __WORDSIZE_TIME64_COMPAT32 1
> > #else
> > -# error "rv32i-based targets are not supported"
> > +# define __WORDSIZE_TIME64_COMPAT32 0
>
> Hmm, this will be problematic on RV64 hardware running mixed RV64/RV32
> userland. This is because files like /var/log/lastlog or /var/log/wtmp
> might then be processed by both RV64 and RV32 executables, meaning that
> the interpretation will be wrong for executables using the other format.
>
> We made the wrong choice with RV64, anticipating that `__time_t' will be
> 32-bit on RV32 and will have to live with that, by following whatever the
> solution is for ports that hold 32-bit time records in the affected
> structures. For now I think that means we have to keep the RV32 time
> records 32-bit, i.e. set __WORDSIZE_TIME64_COMPAT32 to 1, consistently
> with RV64.
Aw :(
I have changed this so __WORDSIZE_TIME64_COMPAT32 is 1 for both RV64 and RV32.
>
> I note there is an extensive discussion on the way to move forward here:
> <https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#utmp_types_and_APIs>
> We might as well try to implement it right away, so as to avoid being
> limited to 32-bit time records here.
Is there an advantage of doing it now or can we put this off for the
next release?
>
> At least these records are not supposed to refer to the future, so
> nothing will be broken right now, however if someone for whatever reason
> wants to keep a single login record for their system past 2038, then
> they'll have an issue (a conversion tool would be straightforward though).
>
> NB some existing ports do have __WORDSIZE_TIME64_COMPAT32 set and cleared
> for their 64-bit and 32-bit ABIs respectively, as per the note in our
> top-level bits/wordsize.h, however this reflects the state as before we
> introduced the possibility for `__time_t' to be a 64-bit type with
> `__WORDSIZE == 32' ABIs. Given the turn of events I think the note ought
> to be updated accordingly; I gather it was missed with commit 07fe93cd9850
> ("generic/typesizes.h: Add support for 32-bit arches with 64-bit types").
>
> > +# define __WORDSIZE32_SIZE_ULONG 0
> > +# define __WORDSIZE32_PTRDIFF_LONG 0
>
> Ack; this matches GCC's <stddef.h>.
>
> > diff --git a/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h b/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> > index c3c72d6c10..363034c38a 100644
> > --- a/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> > +++ b/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> > @@ -32,7 +32,15 @@
> > # define __SIZEOF_PTHREAD_BARRIER_T 32
> > # define __SIZEOF_PTHREAD_BARRIERATTR_T 4
> > #else
> > -# error "rv32i-based systems are not supported"
> > +# define __SIZEOF_PTHREAD_ATTR_T 32
> > +# define __SIZEOF_PTHREAD_MUTEX_T 32
> > +# define __SIZEOF_PTHREAD_MUTEXATTR_T 4
> > +# define __SIZEOF_PTHREAD_COND_T 48
> > +# define __SIZEOF_PTHREAD_CONDATTR_T 4
> > +# define __SIZEOF_PTHREAD_RWLOCK_T 48
> > +# define __SIZEOF_PTHREAD_RWLOCKATTR_T 8
> > +# define __SIZEOF_PTHREAD_BARRIER_T 20
> > +# define __SIZEOF_PTHREAD_BARRIERATTR_T 4
>
> The values are correct, however some of these macros have the same
> expansion regardless of the ABI. For clarity please place them outside
> the conditional, as other ports do.
Fixed.
>
> > diff --git a/sysdeps/riscv/nptl/bits/struct_rwlock.h b/sysdeps/riscv/nptl/bits/struct_rwlock.h
> > index acfaa75e1b..b478da0132 100644
> > --- a/sysdeps/riscv/nptl/bits/struct_rwlock.h
> > +++ b/sysdeps/riscv/nptl/bits/struct_rwlock.h
> > @@ -32,14 +32,39 @@ struct __pthread_rwlock_arch_t
> > unsigned int __writers_futex;
> > unsigned int __pad3;
> > unsigned int __pad4;
> > +#if __riscv_xlen == 64
>
> Same concern about the use of `__riscv_xlen' vs `__WORDSIZE' as before.
Fixed.
>
> > int __cur_writer;
> > int __shared;
> > unsigned long int __pad1;
> > unsigned long int __pad2;
> > unsigned int __flags;
> > +#else
> > +# if __BYTE_ORDER == __BIG_ENDIAN
> > + unsigned char __pad1;
> > + unsigned char __pad2;
> > + unsigned char __shared;
> > + unsigned char __flags;
> > +# else
> > + unsigned char __flags;
> > + unsigned char __shared;
> > + unsigned char __pad1;
> > + unsigned char __pad2;
> > +# endif
> > + int __cur_writer;
> > +#endif
> > };
>
> I note with this change the RV32 structure will use the generic layout as
> per sysdeps/nptl/bits/struct_rwlock.h, however regrettably RV64 does not.
> Would it make sense to instead have the layout the same between RV64 and
> RV32, perhaps by redefining `__pad1' and `__pad2' in terms of `unsigned
> long long' (which would have alignment implications though) or otherwise?
I'm not sure which one is better. On one hand it seems better to be
more generic and therefore RV32 should use the generic interface. On
the other hand the more similar they are the better. I'm still leaning
towards we should be generic where possible.
>
> Is there any benefit from having `__flags' and `__shared' (and the
> reserve) grouped within a single 32-bit word? I gather there is, given
> the lengths gone to to match the bit lanes across the word regardless of
> the endianness. But what is it?
I have no idea.
>
> > -#define __PTHREAD_RWLOCK_INITIALIZER(__flags) \
> > +#if __riscv_xlen == 64
>
> Again, `__riscv_xlen' vs `__WORDSIZE'.
Fixed.
>
> > +# define __PTHREAD_RWLOCK_INITIALIZER(__flags) \
> > 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, __flags
> > +#else
> > +# if __BYTE_ORDER == __BIG_ENDIAN
>
> I would suggest using #elif so as to reduce the number of conditionals
> and eliminate nesting.
Fixed.
>
> > diff --git a/sysdeps/riscv/sfp-machine.h b/sysdeps/riscv/sfp-machine.h
> > index 08a84fd701..aef8c61a67 100644
> > --- a/sysdeps/riscv/sfp-machine.h
> > +++ b/sysdeps/riscv/sfp-machine.h
> > @@ -22,7 +22,32 @@
> >
> > #if __riscv_xlen == 32
>
> NB I think it's OK to keep this `__riscv_xlen' reference as soft-fp is
> largely an independent subsystem and shared between projects (at least
> libgcc and the Linux kernel).
No changed here then
>
> > -# error "rv32i-based targets are not supported"
> > +# define _FP_W_TYPE_SIZE 32
> > +# define _FP_W_TYPE unsigned long
> > +# define _FP_WS_TYPE signed long
> > +# define _FP_I_TYPE long
>
> Please align the RHS of these to the same column.
Fixed.
>
> > +# define _FP_DIV_MEAT_S(R, X, Y) _FP_DIV_MEAT_1_udiv_norm (S, R, X, Y)
> > +# define _FP_DIV_MEAT_D(R, X, Y) _FP_DIV_MEAT_2_udiv (D, R, X, Y)
> > +# define _FP_DIV_MEAT_Q(R, X, Y) _FP_DIV_MEAT_4_udiv (Q, R, X, Y)
> > +
> > +# define _FP_NANFRAC_S _FP_QNANBIT_S
> > +# define _FP_NANFRAC_D _FP_QNANBIT_D, 0
> > +# define _FP_NANFRAC_Q _FP_QNANBIT_Q, 0, 0, 0
>
> Likewise. There seems to be an established practice for this header
> across architectures to have no space between macro arguments or before
> the opening parenthesis. This might help with the alignment.
I still think it makes sense to follow the glibc style though, even if
other archs don't.
Let me know if it should be a different way and I'll update it.
>
> This looks otherwise OK to me (and virtually the same as libgcc's copy,
> except for the extra widening operations defined accordingly for FMA).
Great!
>
> > diff --git a/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h b/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h
> > new file mode 100644
> > index 0000000000..7e48f24345
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/riscv/jmp_buf-macros.h
>
> Shouldn't it go into sysdeps/unix/sysv/linux/riscv/rv32/jmp_buf-macros.h?
>
> > @@ -0,0 +1,53 @@
> > +/* jump buffer constants for RISC-V
>
> Please make it a proper sentence.
Fixed
>
> > + Copyright (C) 2017-2020 Free Software Foundation, Inc.
>
> This is a new file so just 2020 should do.
Fixed
>
> > +/* Produced by this program:
> > +
> > + #include <stdio.h>
> > + #include <unistd.h>
> > + #include <setjmp.h>
> > + #include <stddef.h>
> > +
> > + int main (int argc, char **argv)
> > + {
> > + printf ("#define JMP_BUF_SIZE %d\n", sizeof (jmp_buf));
> > + printf ("#define JMP_BUF_ALIGN %d\n", __alignof__ (jmp_buf));
> > + printf ("#define SIGJMP_BUF_SIZE %d\n", sizeof (sigjmp_buf));
> > + printf ("#define SIGJMP_BUF_ALIGN %d\n", __alignof__ (sigjmp_buf));
> > + printf ("#define MASK_WAS_SAVED_OFFSET %d\n", offsetof (struct __jmp_buf_tag, __mask_was_saved));
> > + printf ("#define SAVED_MASK_OFFSET %d\n", offsetof (struct __jmp_buf_tag, __saved_mask));
> > + } */
>
> Please reformat according to the GNU coding style.
Fixed
Alistair
>
> This file is otherwise OK.
>
> Maciej
next prev parent reply other threads:[~2020-07-10 16:55 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 via Libc-alpha
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 via Libc-alpha
2020-07-07 22:06 ` Maciej W. Rozycki via Libc-alpha
2020-07-10 15:27 ` Alistair Francis via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 02/18] RISC-V: Define __NR_* as __NR_*_time64/64 for 32-bit Alistair Francis via Libc-alpha
2020-07-08 0:09 ` Maciej W. Rozycki via Libc-alpha
2020-07-08 17:08 ` Adhemerval Zanella via Libc-alpha
2020-07-09 17:14 ` Alistair Francis via Libc-alpha
2020-07-16 0:23 ` Maciej W. Rozycki via Libc-alpha
2020-07-09 17:10 ` Alistair Francis via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 03/18] RISC-V: Add support for 32-bit vDSO calls Alistair Francis via Libc-alpha
2020-07-08 1:01 ` Maciej W. Rozycki via Libc-alpha
2020-07-08 18:17 ` Alistair Francis via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 04/18] RISC-V: Support dynamic loader for the 32-bit Alistair Francis via Libc-alpha
2020-07-08 1:35 ` Maciej W. Rozycki via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 05/18] RISC-V: Add path of library directories " Alistair Francis via Libc-alpha
2020-07-08 18:42 ` Maciej W. Rozycki via Libc-alpha
2020-07-09 17:03 ` Alistair Francis via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 06/18] RISC-V: Add arch-syscall.h for RV32 Alistair Francis via Libc-alpha
2020-07-08 19:33 ` Maciej W. Rozycki via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 07/18] RISC-V: nptl: update default pthread-offsets.h Alistair Francis via Libc-alpha
2020-07-09 0:14 ` Maciej W. Rozycki via Libc-alpha
2020-07-09 11:47 ` Adhemerval Zanella via Libc-alpha
2020-07-15 19:23 ` Maciej W. Rozycki via Libc-alpha
2020-08-10 17:34 ` Alistair Francis via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 08/18] riscv32: Add an architecture ipctypes.h Alistair Francis via Libc-alpha
2020-07-09 2:46 ` Maciej W. Rozycki via Libc-alpha
2020-07-09 11:36 ` Adhemerval Zanella via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 09/18] RISC-V: The ABI implementation for 32-bit Alistair Francis via Libc-alpha
2020-07-09 23:33 ` Maciej W. Rozycki via Libc-alpha
2020-07-10 16:45 ` Alistair Francis via Libc-alpha [this message]
2020-07-11 1:24 ` Maciej W. Rozycki via Libc-alpha
2020-08-10 21:29 ` Alistair Francis via Libc-alpha
2020-08-27 19:43 ` Adhemerval Zanella via Libc-alpha
2020-09-25 23:03 ` Alistair Francis via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 10/18] RISC-V: Hard float support " Alistair Francis via Libc-alpha
2020-07-11 0:49 ` Maciej W. Rozycki via Libc-alpha
2020-07-11 15:49 ` Alistair Francis via Libc-alpha
2020-07-11 22:13 ` Maciej W. Rozycki via Libc-alpha
2020-07-12 15:34 ` Alistair Francis via Libc-alpha
2020-07-12 22:10 ` Maciej W. Rozycki via Libc-alpha
2020-08-27 18:36 ` Alistair Francis via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 11/18] RISC-V: Add ABI lists Alistair Francis via Libc-alpha
2020-07-12 20:54 ` Maciej W. Rozycki via Libc-alpha
2020-07-13 16:14 ` Alistair Francis via Libc-alpha
2020-06-03 16:25 ` [PATCH v2 12/18] RISC-V: Add the RV32 libm-test-ulps Alistair Francis via Libc-alpha
2020-06-03 17:34 ` Joseph Myers
2020-06-05 4:01 ` Alistair Francis via Libc-alpha
2020-06-03 16:26 ` [PATCH v2 13/18] RISC-V: Fix llrint and llround missing exceptions on RV32 Alistair Francis via Libc-alpha
2020-06-03 16:26 ` [PATCH v2 14/18] RISC-V: Build Infastructure for 32-bit Alistair Francis via Libc-alpha
2020-06-03 16:26 ` [PATCH v2 15/18] riscv32: Specify the arch_minimum_kernel as 5.4 Alistair Francis via Libc-alpha
2020-06-03 16:26 ` [PATCH v2 16/18] RISC-V: Add rv32 path to RTLDLIST in ldd Alistair Francis via Libc-alpha
2020-06-03 16:26 ` [PATCH v2 17/18] Documentation for the RISC-V 32-bit port Alistair Francis via Libc-alpha
2020-06-03 16:26 ` [PATCH v2 18/18] Add RISC-V 32-bit target to build-many-glibcs.py Alistair Francis via Libc-alpha
2020-06-15 22:37 ` [PATCH v2 00/18] glibc port for 32-bit RISC-V (RV32) Alistair Francis via Libc-alpha
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
List information: https://www.gnu.org/software/libc/involved.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKmqyKMH9P2hM4XF954Nxox_TZ1vHLR_9fAapRFHHHFL7htM3Q@mail.gmail.com \
--to=libc-alpha@sourceware.org \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=macro@wdc.com \
/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).