Re: [PATCH RESEND 0/2] RISC-V: Handle the siginfo_t offset problem

[Alistair Francis <alistair23@gmail.com>]

On Thu, Jul 4, 2019 at 2:19 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jul 4, 2019 at 9:20 AM Andreas Schwab <schwab@suse.de> wrote:
> >
> > On Jul 03 2019, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > > On Wed, Jul 3, 2019 at 12:08 AM Andreas Schwab <schwab@suse.de> wrote:
> > >>
> > >> On Jul 02 2019, Alistair Francis <alistair.francis@wdc.com> wrote:
> > >>
> > >> > In the RISC-V 32-bit glibc port [1] the siginfo_t struct in the kernel
> > >> > doesn't line up with the struct in glibc. In glibc world the _sifields
> > >> > union is 8 byte alligned (although I can't figure out why)
> > >>
> > >> Try ptype/o in gdb.
> > >
> > > That's a useful tip, I'll be sure to use that next time.
> >
> > It was a serious note.  If the structs don't line up then there is a
> > mismatch in types that cannot be solved by adding spurious padding.  You
> > need to fix the types instead.
>
> Would it be an option to align all the basic typedefs (off_t, time_t,
> clock_t, ...)
> between glibc and kernel then, and just use the existing
> sysdeps/unix/sysv/linux/generic/bits/typesizes.h after all to avoid
> surprises like this?
>
> As of v2 the functional difference is
>
> -#define __INO_T_TYPE        __ULONGWORD_TYPE
> +#define __INO_T_TYPE    __UQUAD_TYPE
>  #define __INO64_T_TYPE        __UQUAD_TYPE
>  #define __MODE_T_TYPE        __U32_TYPE
> -#define __NLINK_T_TYPE        __U32_TYPE
> -#define __OFF_T_TYPE        __SLONGWORD_TYPE
> +#define __NLINK_T_TYPE    __UQUAD_TYPE
> +#define __OFF_T_TYPE    __SQUAD_TYPE
>  #define __OFF64_T_TYPE        __SQUAD_TYPE
> -#define __RLIM_T_TYPE        __ULONGWORD_TYPE
> +#define __RLIM_T_TYPE      __UQUAD_TYPE
>  #define __RLIM64_T_TYPE        __UQUAD_TYPE
> -#define    __BLKCNT_T_TYPE        __SLONGWORD_TYPE
> +#define __BLKCNT_T_TYPE    __SQUAD_TYPE
>  #define    __BLKCNT64_T_TYPE    __SQUAD_TYPE
> -#define    __FSBLKCNT_T_TYPE    __ULONGWORD_TYPE
> +#define __FSBLKCNT_T_TYPE  __UQUAD_TYPE
>  #define    __FSBLKCNT64_T_TYPE    __UQUAD_TYPE
> -#define    __FSFILCNT_T_TYPE    __ULONGWORD_TYPE
> +#define __FSFILCNT_T_TYPE  __UQUAD_TYPE
>  #define    __FSFILCNT64_T_TYPE    __UQUAD_TYPE
> -#define    __FSWORD_T_TYPE        __SWORD_TYPE
> +#define __FSWORD_T_TYPE   __SQUAD_TYPE
> -#define __CLOCK_T_TYPE        __SLONGWORD_TYPE
> -#define __TIME_T_TYPE        __SLONGWORD_TYPE
> +#define __CLOCK_T_TYPE     __SQUAD_TYPE
> +#define __TIME_T_TYPE      __SQUAD_TYPE
>  #define __USECONDS_T_TYPE    __U32_TYPE
> -#define __SUSECONDS_T_TYPE    __SLONGWORD_TYPE
> +#define __SUSECONDS_T_TYPE __SQUAD_TYPE
> -#define __BLKSIZE_T_TYPE    __S32_TYPE
> +#define __BLKSIZE_T_TYPE   __SQUAD_TYPE
>  #define __FSID_T_TYPE        struct { int __val[2]; }
>  #define __SSIZE_T_TYPE        __SWORD_TYPE
> -#define __SYSCALL_SLONG_TYPE    __SLONGWORD_TYPE
> -#define __SYSCALL_ULONG_TYPE    __ULONGWORD_TYPE
> -#define __CPU_MASK_TYPE     __ULONGWORD_TYPE
> +#define __SYSCALL_SLONG_TYPE __SQUAD_TYPE
> +#define __SYSCALL_ULONG_TYPE __UQUAD_TYPE
> +#define __CPU_MASK_TYPE    __UQUAD_TYPE
>
> -#ifdef __LP64__
>  # define __RLIM_T_MATCHES_RLIM64_T    1
> -#else
> -# define __RLIM_T_MATCHES_RLIM64_T    0
> -#endif
>
> +#define __ASSUME_TIME64_SYSCALLS 1
> +#define __ASSUME_RLIM64_SYSCALLS 1
>
> Since the sysdeps/unix/sysv/linux/generic/bits/typesizes.h definitions
> generally match the kernel, anything diverging from that has the potential
> of breaking it, so the difference should probably be kept to the absolute
> minimum.
>
> I think these ones are wrong and will cause bugs similar to the clock_t
> issue if they are used with kernel interfaces:
> __NLINK_T_TYPE, __FSWORD_T_TYPE, __CLOCK_T_TYPE,
> __BLKSIZE_T_TYPE, __SYSCALL_ULONG_TYPE,
> __SYSCALL_SLONG_TYPE, __CPU_MASK_TYPE
>
> These are fine as long as they are only used in user space and to
> wrap kernel syscalls, but I think most of them can end up being
> passed to the kernel, so it seems safer not to have rv32 diverge
> without a good reason.
>
> The remaining ones (__INO_T_TYPE, __OFF_T_TYPE, __BLKCNT_T_TYPE,
> __FSBLKCNT_T_TYPE, __FSFILCNT_T_TYPE, __TIME_T_TYPE) all
> follow the pattern where the kernel has an old 32-bit type and a new
> 64-bit type, but the kernel tries not to expose the 32-bit interfaces
> to user space on new architectures and only provide the 64-bit
> replacements, but there are a couple of interfaces that never got
> replaced, typically in driver and file system ioctls.
>
> Since glibc already has code to deal with the 64-bit types and that
> is well tested, it would seem safer to me to just #undef the old
> types completely rather than defining them to 64-bit, which would
> make them incompatible with the kernel's types.

#undef-ing these results in build failures unfortunately, it seems
like they are required.

I'm sending a v3 RFC to the glibc list right now. We can continue the
discussion there.

Alistair

>
>        Arnd