[PATCH v2 09/18] RISC-V: The ABI implementation for 32-bit

[Alistair Francis]

On Thu, Aug 27, 2020 at 12:43 PM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> On 10/08/2020 18:29, Alistair Francis via Libc-alpha wrote:
> > On Fri, Jul 10, 2020 at 6:24 PM Maciej W. Rozycki <macro@wdc.com> wrote:
> >>
> >> On Fri, 10 Jul 2020, Alistair Francis wrote:
> >>
> >>>>  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?
> >>
> >>  The change is major enough it'll have to wait for the next development
> >> cycle anyway.  It shouldn't matter that much for RV32 glibc deployments
> >> though, given the amount of suitable hardware available.
> >>
> >>>>  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").
> >>
> >>  Will you be able to look into it?
> >>
> >>  The context here is before Y2038 changes __WORDSIZE_TIME64_COMPAT32 would
> >> only be clear for 64-bit ABIs with those 64-bit systems that do not have
> >> any 32-bit ABI (compatibility mode) to support, such as the DEC Alpha.
> >> And it would always be clear for 32-bit ABIs, so as to use the proper
> >> `__time_t' type without changing the width of actual data held there in
> >> the structure.
> >>
> >>  I'm not sure what the story is behind the S/390 port though; perhaps it
> >> does not support ABI coexistence in a single run-time environment.
> >
> > I was just about to start this but Adhemerval has very kindly sent a
> > patch series already. I'm going to leave this as is (defining it as 1
> > for RV32 and RV64).
>
> Don't forget to help on review ;).

I'm trying :)

>
> >
> >
> >>
> >>>>>    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.
> >>
> >>  It would be good to get a second opinion here.
> >
> > The RV64 __pthread_rwlock_arch_t is exactly the same as the AArch64
> > version. My guess is that it was just copied to be the same.
> >
> > The generic version also has this comment
> >
> > /* Generic struct for both POSIX read-write lock.  New ports are expected
> >    to use the default layout, however archictetures can redefine it to add
> >    arch-specific extensions (such as lock-elision).  The struct have a size
> >    of 32 bytes on both LP32 and LP64 architectures.  */
> >
> > As we aren't adding any arch-specific extensions I think we should
> > keep the generic one. It's unfortunate we didn't do that for RV64, but
> > too late now.
> >
> > The only downside I see in using the generic version is what happens
> > if the value of _shared or flags overflows a char. There are currently
> > only 4 flags though, so I think it's ok.
>
> I come up with the generic interface by using the already one in place for
> mostly 64-bit architecture and the internal layout is mainly to avoid duplicate
> the internal type on multiple architectures (so we can use the generic
> implementation).
>
> The only usage I can think of where same structure for RV64 and RV32 may
> pose a role is to use a shared rwlock between a 32-bit and a 64-bit process
> (pthread_rwlockattr_setpshared with PTHREAD_PROCESS_SHARED).  But afaik this
> is not supported on other architectures.

Thanks for describing this. I'm going to stick with the current
generic implementation for RV32 then.

>
> >
> >>
> >>>>  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.
> >>
> >>  Especially given this.
>
> The internal member that are most concurrently accessed by the common operations
> (pthread_rwlock_{clock,timed}{wr,rd}lock) are the __readers, __writers,
> __wrphase_futex, __writers_futex, and __cur_writer.  The __shared and __flag
> are either accessed with non-atomic operation or at initialization,  so I don't
> think there is much different in packing or not in a 32-bit word.  My
> understanding is architectures usually does that to avoid possible internal
> padding, since the usual types for both member are uint8_t.

Thanks for the info here as well.

Alistair

>
> >>
> >>>>> +# 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.
> >>
> >>  There is the issue of the discrepancy compared to the libgcc version, and
> >> while `diff -l' and `patch -l' solve that for manual processing, more
> >> sophisticated tools may not cope and require manual intervention.
> >>
> >>  Again, I would suggest getting a second opinion.
> >
> > The current option matches the rest of the file, so I think it makes
> > sense to leave as is.
> >
> > If this is a problem we can just change the spacing for the entire
> > file in a future patch.
> >
> > Alistair
> >
> >>
> >>   Maciej