perf tools build broken for RISCV 32 bit

perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi,

When building perf for RISCV 32 bit (v5.10.7) I got the following

| In file included from bench/futex-hash.c:29:
| bench/futex.h: In function ‘futex_wait’:
| bench/futex.h:37:10: error: ‘SYS_futex’ undeclared (first use in this function); did you mean ‘SYS_tee’?

This issue is similar to the one reported in https://lkml.org/lkml/2019/4/19/631

I found that patching tools/arch/riscv/include/uapi/asm/unistd.h as following:

 #ifdef __LP64__
 #define __ARCH_WANT_NEW_STAT
 #define __ARCH_WANT_SET_GET_RLIMIT
+#else
+#define __ARCH_WANT_TIME32_SYSCALLS
 #endif /* __LP64__ */

solved the problem.

I also found that a similar patch for arch/riscv/include/uapi/asm/unistd.h
was removed in commit d4c08b9776b3, so probably this is not the right way(?).

Thank you, best regards.

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Thu, Jan 14, 2021 at 7:38 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
>
> Hi,
>
> When building perf for RISCV 32 bit (v5.10.7) I got the following
>
> | In file included from bench/futex-hash.c:29:
> | bench/futex.h: In function ‘futex_wait’:
> | bench/futex.h:37:10: error: ‘SYS_futex’ undeclared (first use in this function); did you mean ‘SYS_tee’?
>
> This issue is similar to the one reported in https://lkml.org/lkml/2019/4/19/631
>
> I found that patching tools/arch/riscv/include/uapi/asm/unistd.h as following:
>
>  #ifdef __LP64__
>  #define __ARCH_WANT_NEW_STAT
>  #define __ARCH_WANT_SET_GET_RLIMIT
> +#else
> +#define __ARCH_WANT_TIME32_SYSCALLS
>  #endif /* __LP64__ */
>
> solved the problem.
>
> I also found that a similar patch for arch/riscv/include/uapi/asm/unistd.h
> was removed in commit d4c08b9776b3, so probably this is not the right way(?).

In short, it won't work, as rv32 does not provide the time32 syscalls.
Your patch will make the application build, but it will not be able to
call futex().

You will in fact run into a related problem on any 32-bit architecture
if CONFIG_COMPAT_32BIT_TIME is disabled, or if you pass a non-NULL
timeout parameter and build with a time64-enabled libc.

The fix in the application is to call either __NR_futex or __NR_futex64
depending on the definition of time_t in the C library. I would recommend
doing it like

#ifdef __NR_futex
#define do_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
         __NR_futex : __NR_futex_time64
#else
#define do_futex __NR_futex
#done

       Arnd

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

thank you for the quick reply and support.

On Thu, Jan 14, 2021 at 08:16:28PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 14, 2021 at 7:38 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> >
> > Hi,
> >
> > When building perf for RISCV 32 bit (v5.10.7) I got the following
> >
> > | In file included from bench/futex-hash.c:29:
> > | bench/futex.h: In function ‘futex_wait’:
> > | bench/futex.h:37:10: error: ‘SYS_futex’ undeclared (first use in this function); did you mean ‘SYS_tee’?
> >
> > This issue is similar to the one reported in https://lkml.org/lkml/2019/4/19/631
> >
> > I found that patching tools/arch/riscv/include/uapi/asm/unistd.h as following:
> >
> >  #ifdef __LP64__
> >  #define __ARCH_WANT_NEW_STAT
> >  #define __ARCH_WANT_SET_GET_RLIMIT
> > +#else
> > +#define __ARCH_WANT_TIME32_SYSCALLS
> >  #endif /* __LP64__ */
> >
> > solved the problem.
> >
> > I also found that a similar patch for arch/riscv/include/uapi/asm/unistd.h
> > was removed in commit d4c08b9776b3, so probably this is not the right way(?).
>
> In short, it won't work, as rv32 does not provide the time32 syscalls.
> Your patch will make the application build, but it will not be able to
> call futex().
>
> You will in fact run into a related problem on any 32-bit architecture
> if CONFIG_COMPAT_32BIT_TIME is disabled, or if you pass a non-NULL
> timeout parameter and build with a time64-enabled libc.
>

I'm using glibc 2.32 which supports 64 bit time_t on RISCV 32.

In particular, searching for __NR_futex in glibc RISCV source code I got:

sysdeps/unix/sysv/linux/riscv/rv32/arch-syscall.h: #define __NR_futex_time64 422
sysdeps/unix/sysv/linux/riscv/sysdep.h: #define __NR_futex __NR_futex_time64
sysdeps/unix/sysv/linux/riscv/rv64/arch-syscall.h: #define __NR_futex 98

but in the generated bits/syscall.h, included in bench/futex.h, I found:

#ifdef __NR_futex
# define SYS_futex __NR_futex
#endif

#ifdef __NR_futex_time64
# define SYS_futex_time64 __NR_futex_time64
#endif

So the problem is that userspace applications can't see the definition
of __NR_futex which is in sysdep.h, but there are no problems in calling
futex() libc wrapper because glibc syscall.c includes that header.

A possible fix for the perf tool would be to use futex() libc wrapper
in tools/perf/bench/futex.h instead of syscall(), but, if I understand correctly,
there are some drawbacks that does not permit it.

So what should be the right solution?

> The fix in the application is to call either __NR_futex or __NR_futex64
> depending on the definition of time_t in the C library. I would recommend
> doing it like
>
> #ifdef __NR_futex
> #define do_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
>          __NR_futex : __NR_futex_time64
> #else
> #define do_futex __NR_futex
> #done
>
>        Arnd

Where should be this fix applied? To perf code?

Thank you,

Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

sorry for my mistake. Effectively glibc does not provide a wrapper for
futex syscall, so its usage is obviously not a solution.

Following what can be found in glibc riscv sysdep.h file:

|#if __WORDSIZE == 32
|/* Workarounds for generic code needing to handle 64-bit time_t.
|...
|/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
|#define __NR_futex              __NR_futex_time64

a possible solution to fix the build of perf tool on RISCV 32 bit could
be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
|+
|+#ifndef __NR_futex
|+#define __NR_futex __NR_futex_time64
|+#endif

What do you think about it? Could it be a correct solution?

Best regards,

Emiliano

On Thu, Jan 14, 2021 at 08:16:28PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 14, 2021 at 7:38 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> >
> > Hi,
> >
> > When building perf for RISCV 32 bit (v5.10.7) I got the following
> >
> > | In file included from bench/futex-hash.c:29:
> > | bench/futex.h: In function ‘futex_wait’:
> > | bench/futex.h:37:10: error: ‘SYS_futex’ undeclared (first use in this function); did you mean ‘SYS_tee’?
> >
> > This issue is similar to the one reported in https://lkml.org/lkml/2019/4/19/631
> >
> > I found that patching tools/arch/riscv/include/uapi/asm/unistd.h as following:
> >
> >  #ifdef __LP64__
> >  #define __ARCH_WANT_NEW_STAT
> >  #define __ARCH_WANT_SET_GET_RLIMIT
> > +#else
> > +#define __ARCH_WANT_TIME32_SYSCALLS
> >  #endif /* __LP64__ */
> >
> > solved the problem.
> >
> > I also found that a similar patch for arch/riscv/include/uapi/asm/unistd.h
> > was removed in commit d4c08b9776b3, so probably this is not the right way(?).
>
> In short, it won't work, as rv32 does not provide the time32 syscalls.
> Your patch will make the application build, but it will not be able to
> call futex().
>
> You will in fact run into a related problem on any 32-bit architecture
> if CONFIG_COMPAT_32BIT_TIME is disabled, or if you pass a non-NULL
> timeout parameter and build with a time64-enabled libc.
>
> The fix in the application is to call either __NR_futex or __NR_futex64
> depending on the definition of time_t in the C library. I would recommend
> doing it like
>
> #ifdef __NR_futex
> #define do_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
>          __NR_futex : __NR_futex_time64
> #else
> #define do_futex __NR_futex
> #done
>
>        Arnd

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

sorry for my mistake. Effectively glibc does not provide a wrapper for
futex syscall, so its usage is obviously not a solution.

Following what can be found in glibc riscv sysdep.h file:

|#if __WORDSIZE == 32
|/* Workarounds for generic code needing to handle 64-bit time_t.
|...
|/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
|#define __NR_futex              __NR_futex_time64

a possible solution to fix the build of perf tool on RISCV 32 bit could
be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
|+
|+#ifndef __NR_futex
|+#define __NR_futex __NR_futex_time64
|+#endif

What do you think about it? Could it be a correct solution?

Best regards,

Emiliano

On Thu, Jan 14, 2021 at 08:16:28PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 14, 2021 at 7:38 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> >
> > Hi,
> >
> > When building perf for RISCV 32 bit (v5.10.7) I got the following
> >
> > | In file included from bench/futex-hash.c:29:
> > | bench/futex.h: In function ‘futex_wait’:
> > | bench/futex.h:37:10: error: ‘SYS_futex’ undeclared (first use in this function); did you mean ‘SYS_tee’?
> >
> > This issue is similar to the one reported in https://lkml.org/lkml/2019/4/19/631
> >
> > I found that patching tools/arch/riscv/include/uapi/asm/unistd.h as following:
> >
> >  #ifdef __LP64__
> >  #define __ARCH_WANT_NEW_STAT
> >  #define __ARCH_WANT_SET_GET_RLIMIT
> > +#else
> > +#define __ARCH_WANT_TIME32_SYSCALLS
> >  #endif /* __LP64__ */
> >
> > solved the problem.
> >
> > I also found that a similar patch for arch/riscv/include/uapi/asm/unistd.h
> > was removed in commit d4c08b9776b3, so probably this is not the right way(?).
>
> In short, it won't work, as rv32 does not provide the time32 syscalls.
> Your patch will make the application build, but it will not be able to
> call futex().
>
> You will in fact run into a related problem on any 32-bit architecture
> if CONFIG_COMPAT_32BIT_TIME is disabled, or if you pass a non-NULL
> timeout parameter and build with a time64-enabled libc.
>
> The fix in the application is to call either __NR_futex or __NR_futex64
> depending on the definition of time_t in the C library. I would recommend
> doing it like
>
> #ifdef __NR_futex
> #define do_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
>          __NR_futex : __NR_futex_time64
> #else
> #define do_futex __NR_futex
> #done
>
>        Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 12:35 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> |#if __WORDSIZE == 32
> |/* Workarounds for generic code needing to handle 64-bit time_t.
> |...
> |/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
> |#define __NR_futex              __NR_futex_time64
>
> a possible solution to fix the build of perf tool on RISCV 32 bit could
> be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
> |+
> |+#ifndef __NR_futex
> |+#define __NR_futex __NR_futex_time64
> |+#endif
>
> What do you think about it? Could it be a correct solution?

No, that would be much worse: __NR_futex must only ever point
to the system call that takes the old timespec argument. If you define
something like this, it breaks applications that try to do the right thing.

       Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 12:35 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> |#if __WORDSIZE == 32
> |/* Workarounds for generic code needing to handle 64-bit time_t.
> |...
> |/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
> |#define __NR_futex              __NR_futex_time64
>
> a possible solution to fix the build of perf tool on RISCV 32 bit could
> be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
> |+
> |+#ifndef __NR_futex
> |+#define __NR_futex __NR_futex_time64
> |+#endif
>
> What do you think about it? Could it be a correct solution?

No, that would be much worse: __NR_futex must only ever point
to the system call that takes the old timespec argument. If you define
something like this, it breaks applications that try to do the right thing.

       Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

thank you for the quick reply and support.

On Mon, Jan 18, 2021 at 12:56:11PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 18, 2021 at 12:35 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > |#if __WORDSIZE == 32
> > |/* Workarounds for generic code needing to handle 64-bit time_t.
> > |...
> > |/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
> > |#define __NR_futex              __NR_futex_time64
> >
> > a possible solution to fix the build of perf tool on RISCV 32 bit could
> > be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
> > |+
> > |+#ifndef __NR_futex
> > |+#define __NR_futex __NR_futex_time64
> > |+#endif
> >
> > What do you think about it? Could it be a correct solution?
>
> No, that would be much worse: __NR_futex must only ever point
> to the system call that takes the old timespec argument. If you define
> something like this, it breaks applications that try to do the right thing.
>
>        Arnd

OK, but what should be the right solution for RISCV 32 bit in this case?
Is it a problem in the libc implementation or in RISCV/perf implementation?

Thank you, best regards.

Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

thank you for the quick reply and support.

On Mon, Jan 18, 2021 at 12:56:11PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 18, 2021 at 12:35 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > |#if __WORDSIZE == 32
> > |/* Workarounds for generic code needing to handle 64-bit time_t.
> > |...
> > |/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
> > |#define __NR_futex              __NR_futex_time64
> >
> > a possible solution to fix the build of perf tool on RISCV 32 bit could
> > be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
> > |+
> > |+#ifndef __NR_futex
> > |+#define __NR_futex __NR_futex_time64
> > |+#endif
> >
> > What do you think about it? Could it be a correct solution?
>
> No, that would be much worse: __NR_futex must only ever point
> to the system call that takes the old timespec argument. If you define
> something like this, it breaks applications that try to do the right thing.
>
>        Arnd

OK, but what should be the right solution for RISCV 32 bit in this case?
Is it a problem in the libc implementation or in RISCV/perf implementation?

Thank you, best regards.

Emiliano

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 3:25 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> On Mon, Jan 18, 2021 at 12:56:11PM +0100, Arnd Bergmann wrote:
> > On Mon, Jan 18, 2021 at 12:35 PM Emiliano Ingrassia
> > <ingrassia@epigenesys.com> wrote:
> > > |#if __WORDSIZE == 32
> > > |/* Workarounds for generic code needing to handle 64-bit time_t.
> > > |...
> > > |/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
> > > |#define __NR_futex              __NR_futex_time64
> > >
> > > a possible solution to fix the build of perf tool on RISCV 32 bit could
> > > be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
> > > |+
> > > |+#ifndef __NR_futex
> > > |+#define __NR_futex __NR_futex_time64
> > > |+#endif
> > >
> > > What do you think about it? Could it be a correct solution?
> >
> > No, that would be much worse: __NR_futex must only ever point
> > to the system call that takes the old timespec argument. If you define
> > something like this, it breaks applications that try to do the right thing.
>
> OK, but what should be the right solution for RISCV 32 bit in this case?
> Is it a problem in the libc implementation or in RISCV/perf implementation?

The problem is in perf, along with as any other application that wants to
use futex on a 32-bit architecture with 64-bit time_t.

I already said what the application needs to do in order to get the
right system call, in the absence of a wrapper that is part of every
C library.

       Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 3:25 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> On Mon, Jan 18, 2021 at 12:56:11PM +0100, Arnd Bergmann wrote:
> > On Mon, Jan 18, 2021 at 12:35 PM Emiliano Ingrassia
> > <ingrassia@epigenesys.com> wrote:
> > > |#if __WORDSIZE == 32
> > > |/* Workarounds for generic code needing to handle 64-bit time_t.
> > > |...
> > > |/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
> > > |#define __NR_futex              __NR_futex_time64
> > >
> > > a possible solution to fix the build of perf tool on RISCV 32 bit could
> > > be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
> > > |+
> > > |+#ifndef __NR_futex
> > > |+#define __NR_futex __NR_futex_time64
> > > |+#endif
> > >
> > > What do you think about it? Could it be a correct solution?
> >
> > No, that would be much worse: __NR_futex must only ever point
> > to the system call that takes the old timespec argument. If you define
> > something like this, it breaks applications that try to do the right thing.
>
> OK, but what should be the right solution for RISCV 32 bit in this case?
> Is it a problem in the libc implementation or in RISCV/perf implementation?

The problem is in perf, along with as any other application that wants to
use futex on a 32-bit architecture with 64-bit time_t.

I already said what the application needs to do in order to get the
right system call, in the absence of a wrapper that is part of every
C library.

       Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi,

after some searching I found this patch from Yocto project:

https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.

I don't know if it's ever been proposed as mainline patch.

What do you think about it? Could it be the right solution?

Thank you, best regards.

Emiliano

On Mon, Jan 18, 2021 at 03:25:05PM +0100, Emiliano Ingrassia wrote:
> Hi Arnd,
>
> thank you for the quick reply and support.
>
> On Mon, Jan 18, 2021 at 12:56:11PM +0100, Arnd Bergmann wrote:
> > On Mon, Jan 18, 2021 at 12:35 PM Emiliano Ingrassia
> > <ingrassia@epigenesys.com> wrote:
> > > |#if __WORDSIZE == 32
> > > |/* Workarounds for generic code needing to handle 64-bit time_t.
> > > |...
> > > |/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
> > > |#define __NR_futex              __NR_futex_time64
> > >
> > > a possible solution to fix the build of perf tool on RISCV 32 bit could
> > > be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
> > > |+
> > > |+#ifndef __NR_futex
> > > |+#define __NR_futex __NR_futex_time64
> > > |+#endif
> > >
> > > What do you think about it? Could it be a correct solution?
> >
> > No, that would be much worse: __NR_futex must only ever point
> > to the system call that takes the old timespec argument. If you define
> > something like this, it breaks applications that try to do the right thing.
> >
> >        Arnd
>
> OK, but what should be the right solution for RISCV 32 bit in this case?
> Is it a problem in the libc implementation or in RISCV/perf implementation?
>
> Thank you, best regards.
>
> Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi,

after some searching I found this patch from Yocto project:

https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.

I don't know if it's ever been proposed as mainline patch.

What do you think about it? Could it be the right solution?

Thank you, best regards.

Emiliano

On Mon, Jan 18, 2021 at 03:25:05PM +0100, Emiliano Ingrassia wrote:
> Hi Arnd,
>
> thank you for the quick reply and support.
>
> On Mon, Jan 18, 2021 at 12:56:11PM +0100, Arnd Bergmann wrote:
> > On Mon, Jan 18, 2021 at 12:35 PM Emiliano Ingrassia
> > <ingrassia@epigenesys.com> wrote:
> > > |#if __WORDSIZE == 32
> > > |/* Workarounds for generic code needing to handle 64-bit time_t.
> > > |...
> > > |/* Fix sysdeps/nptl/lowlevellock-futex.h.  */
> > > |#define __NR_futex              __NR_futex_time64
> > >
> > > a possible solution to fix the build of perf tool on RISCV 32 bit could
> > > be the following patch to file tools/arch/riscv/include/uapi/asm/unistd.h:
> > > |+
> > > |+#ifndef __NR_futex
> > > |+#define __NR_futex __NR_futex_time64
> > > |+#endif
> > >
> > > What do you think about it? Could it be a correct solution?
> >
> > No, that would be much worse: __NR_futex must only ever point
> > to the system call that takes the old timespec argument. If you define
> > something like this, it breaks applications that try to do the right thing.
> >
> >        Arnd
>
> OK, but what should be the right solution for RISCV 32 bit in this case?
> Is it a problem in the libc implementation or in RISCV/perf implementation?
>
> Thank you, best regards.
>
> Emiliano

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
>
> Hi,
>
> after some searching I found this patch from Yocto project:
>
> https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
>
> I don't know if it's ever been proposed as mainline patch.
>
> What do you think about it? Could it be the right solution?

No. While this makes it work on rv32, it does not fix it in a
way that makes it work on other architectures with a time64
libc. If you fix it, please do it in a way that works on all architectures.

I had really expected both Alistair and Khem to know better.

      Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
>
> Hi,
>
> after some searching I found this patch from Yocto project:
>
> https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
>
> I don't know if it's ever been proposed as mainline patch.
>
> What do you think about it? Could it be the right solution?

No. While this makes it work on rv32, it does not fix it in a
way that makes it work on other architectures with a time64
libc. If you fix it, please do it in a way that works on all architectures.

I had really expected both Alistair and Khem to know better.

      Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> >
> > Hi,
> >
> > after some searching I found this patch from Yocto project:
> >
> > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> >
> > I don't know if it's ever been proposed as mainline patch.
> >
> > What do you think about it? Could it be the right solution?
>
> No. While this makes it work on rv32, it does not fix it in a
> way that makes it work on other architectures with a time64
> libc. If you fix it, please do it in a way that works on all architectures.

Ok, I totally agree with you on this point.

So, it is right to conclude that the problem is not in perf
implementation but in the libc implementation?
That is, for example, glibc should define SYS_futex as SYS_futex_time64
in case of riscv 32 bit because of its support to 64 bit time_t?

>
> I had really expected both Alistair and Khem to know better.
>
>       Arnd

Thank you, best regards.

Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> >
> > Hi,
> >
> > after some searching I found this patch from Yocto project:
> >
> > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> >
> > I don't know if it's ever been proposed as mainline patch.
> >
> > What do you think about it? Could it be the right solution?
>
> No. While this makes it work on rv32, it does not fix it in a
> way that makes it work on other architectures with a time64
> libc. If you fix it, please do it in a way that works on all architectures.

Ok, I totally agree with you on this point.

So, it is right to conclude that the problem is not in perf
implementation but in the libc implementation?
That is, for example, glibc should define SYS_futex as SYS_futex_time64
in case of riscv 32 bit because of its support to 64 bit time_t?

>
> I had really expected both Alistair and Khem to know better.
>
>       Arnd

Thank you, best regards.

Emiliano

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:

> Ok, I totally agree with you on this point.
>
> So, it is right to conclude that the problem is not in perf
> implementation but in the libc implementation?

No, libc does not provide a futex abstraction, and the SYS_* macros
just refer to the low-level interfaces that operate on kernel types rather
than libc types. An application that uses them must pass the
correct types.

> That is, for example, glibc should define SYS_futex as SYS_futex_time64
> in case of riscv 32 bit because of its support to 64 bit time_t?

As I explained, SYS_futex would refer to the time32 syscall, which
does not exist on rv32, redefining it to a different syscall would lead to
data corruption as well.

If you call futex_time64, you have to pass a __kernel_timespec structure,
while the old futex call must take a __kernel_old_timespec structure.
Depending on the architecture, only one of the two syscalls might be
available, and if you pass a timespec defined by libc, you have to
know which of the two syscalls that corresponds to, or convert
the arguments when calling it.

      Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:

> Ok, I totally agree with you on this point.
>
> So, it is right to conclude that the problem is not in perf
> implementation but in the libc implementation?

No, libc does not provide a futex abstraction, and the SYS_* macros
just refer to the low-level interfaces that operate on kernel types rather
than libc types. An application that uses them must pass the
correct types.

> That is, for example, glibc should define SYS_futex as SYS_futex_time64
> in case of riscv 32 bit because of its support to 64 bit time_t?

As I explained, SYS_futex would refer to the time32 syscall, which
does not exist on rv32, redefining it to a different syscall would lead to
data corruption as well.

If you call futex_time64, you have to pass a __kernel_timespec structure,
while the old futex call must take a __kernel_old_timespec structure.
Depending on the architecture, only one of the two syscalls might be
available, and if you pass a timespec defined by libc, you have to
know which of the two syscalls that corresponds to, or convert
the arguments when calling it.

      Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Mon, Jan 18, 2021 at 04:44:36PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
>
> > Ok, I totally agree with you on this point.
> >
> > So, it is right to conclude that the problem is not in perf
> > implementation but in the libc implementation?
>
> No, libc does not provide a futex abstraction, and the SYS_* macros
> just refer to the low-level interfaces that operate on kernel types rather
> than libc types. An application that uses them must pass the
> correct types.
>
> > That is, for example, glibc should define SYS_futex as SYS_futex_time64
> > in case of riscv 32 bit because of its support to 64 bit time_t?
>
> As I explained, SYS_futex would refer to the time32 syscall, which
> does not exist on rv32, redefining it to a different syscall would lead to
> data corruption as well.
>

I tried to apply the patch you proposed in your first reply:

> #ifdef __NR_futex
> #define do_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
>          __NR_futex : __NR_futex_time64
> #else
> #define do_futex __NR_futex
> #done

in a simple user space app which calls futex (I changed do_futex with
SYS_futex):

#include <stdlib.h>
#include <stdint.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <linux/futex.h>
#include <sys/time.h>

#ifdef __NR_futex
#define SYS_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
				           __NR_futex : __NR_futex_time64
#else
#define SYS_futex __NR_futex
#endif

static int
futex(uint32_t *uaddr, int futex_op, uint32_t val,
      const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
{
	return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3);
}

int main(void)
{
	return futex(NULL, FUTEX_WAKE, 0, NULL, NULL, 0);
}

But what I get is:

error: ‘__NR_futex’ undeclared (first use in this function)
   12 | #define SYS_futex __NR_futex


The problem is that if __NR_futex is not defined it will not work anyway.


So the only viable solution to correctly compile and use perf on riscv
32 bit is the implementation of time32 syscall for that architecture?

And this is true for all 32 bit applications that want to call futex
syscall with a non NULL timer parameter, right?


> If you call futex_time64, you have to pass a __kernel_timespec structure,
> while the old futex call must take a __kernel_old_timespec structure.
> Depending on the architecture, only one of the two syscalls might be
> available, and if you pass a timespec defined by libc, you have to
> know which of the two syscalls that corresponds to, or convert
> the arguments when calling it.
>
>       Arnd

Thank you, best regards.

Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Mon, Jan 18, 2021 at 04:44:36PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
>
> > Ok, I totally agree with you on this point.
> >
> > So, it is right to conclude that the problem is not in perf
> > implementation but in the libc implementation?
>
> No, libc does not provide a futex abstraction, and the SYS_* macros
> just refer to the low-level interfaces that operate on kernel types rather
> than libc types. An application that uses them must pass the
> correct types.
>
> > That is, for example, glibc should define SYS_futex as SYS_futex_time64
> > in case of riscv 32 bit because of its support to 64 bit time_t?
>
> As I explained, SYS_futex would refer to the time32 syscall, which
> does not exist on rv32, redefining it to a different syscall would lead to
> data corruption as well.
>

I tried to apply the patch you proposed in your first reply:

> #ifdef __NR_futex
> #define do_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
>          __NR_futex : __NR_futex_time64
> #else
> #define do_futex __NR_futex
> #done

in a simple user space app which calls futex (I changed do_futex with
SYS_futex):

#include <stdlib.h>
#include <stdint.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <linux/futex.h>
#include <sys/time.h>

#ifdef __NR_futex
#define SYS_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
				           __NR_futex : __NR_futex_time64
#else
#define SYS_futex __NR_futex
#endif

static int
futex(uint32_t *uaddr, int futex_op, uint32_t val,
      const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
{
	return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3);
}

int main(void)
{
	return futex(NULL, FUTEX_WAKE, 0, NULL, NULL, 0);
}

But what I get is:

error: ‘__NR_futex’ undeclared (first use in this function)
   12 | #define SYS_futex __NR_futex


The problem is that if __NR_futex is not defined it will not work anyway.


So the only viable solution to correctly compile and use perf on riscv
32 bit is the implementation of time32 syscall for that architecture?

And this is true for all 32 bit applications that want to call futex
syscall with a non NULL timer parameter, right?


> If you call futex_time64, you have to pass a __kernel_timespec structure,
> while the old futex call must take a __kernel_old_timespec structure.
> Depending on the architecture, only one of the two syscalls might be
> available, and if you pass a timespec defined by libc, you have to
> know which of the two syscalls that corresponds to, or convert
> the arguments when calling it.
>
>       Arnd

Thank you, best regards.

Emiliano

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 5:34 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
>
> in a simple user space app which calls futex (I changed do_futex with
> SYS_futex):
>
> #include <stdlib.h>
> #include <stdint.h>
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <linux/futex.h>
> #include <sys/time.h>
>
> #ifdef __NR_futex
> #define SYS_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
>                                            __NR_futex : __NR_futex_time64
> #else
> #define SYS_futex __NR_futex
> #endif

Note that SYS_futex will clash with a libc defined macro of the same name,
so better avoid that.

I guess you also need a third path

#ifndef __NR_futex /* using time64/__kernel_timespec */
...
#elif !defined(__NR_futex64) /* using __kernel_old_timespec */
...
#else /* libc dependent */
...
#endif

> static int
> futex(uint32_t *uaddr, int futex_op, uint32_t val,
>       const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
> {
>         return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3);
> }
>
> int main(void)
> {
>         return futex(NULL, FUTEX_WAKE, 0, NULL, NULL, 0);
> }
>
> But what I get is:
>
> error: ‘__NR_futex’ undeclared (first use in this function)
>    12 | #define SYS_futex __NR_futex
>
>
> The problem is that if __NR_futex is not defined it will not work anyway.

Right, sorry about that, see above.

> So the only viable solution to correctly compile and use perf on riscv
> 32 bit is the implementation of time32 syscall for that architecture?

You only need a time32 syscall if your libc defines the time32
version of 'struct timespec'.

> And this is true for all 32 bit applications that want to call futex
> syscall with a non NULL timer parameter, right?

Also the ones that pass a NULL parameter, as the kernel does
not necessarily implement both.

         Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 5:34 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
>
> in a simple user space app which calls futex (I changed do_futex with
> SYS_futex):
>
> #include <stdlib.h>
> #include <stdint.h>
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <linux/futex.h>
> #include <sys/time.h>
>
> #ifdef __NR_futex
> #define SYS_futex (sizeof(time_t) == sizeof(__kernel_long_t)) ? \
>                                            __NR_futex : __NR_futex_time64
> #else
> #define SYS_futex __NR_futex
> #endif

Note that SYS_futex will clash with a libc defined macro of the same name,
so better avoid that.

I guess you also need a third path

#ifndef __NR_futex /* using time64/__kernel_timespec */
...
#elif !defined(__NR_futex64) /* using __kernel_old_timespec */
...
#else /* libc dependent */
...
#endif

> static int
> futex(uint32_t *uaddr, int futex_op, uint32_t val,
>       const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
> {
>         return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3);
> }
>
> int main(void)
> {
>         return futex(NULL, FUTEX_WAKE, 0, NULL, NULL, 0);
> }
>
> But what I get is:
>
> error: ‘__NR_futex’ undeclared (first use in this function)
>    12 | #define SYS_futex __NR_futex
>
>
> The problem is that if __NR_futex is not defined it will not work anyway.

Right, sorry about that, see above.

> So the only viable solution to correctly compile and use perf on riscv
> 32 bit is the implementation of time32 syscall for that architecture?

You only need a time32 syscall if your libc defines the time32
version of 'struct timespec'.

> And this is true for all 32 bit applications that want to call futex
> syscall with a non NULL timer parameter, right?

Also the ones that pass a NULL parameter, as the kernel does
not necessarily implement both.

         Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> > On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia <ingrassia@epigenesys.com> wrote:
> > >
> > > Hi,
> > >
> > > after some searching I found this patch from Yocto project:
> > >
> > > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> > >
> > > I don't know if it's ever been proposed as mainline patch.
> > >
> > > What do you think about it? Could it be the right solution?
> >
> > No. While this makes it work on rv32, it does not fix it in a
> > way that makes it work on other architectures with a time64
> > libc. If you fix it, please do it in a way that works on all architectures.
>
> Ok, I totally agree with you on this point.
>
> So, it is right to conclude that the problem is not in perf
> implementation but in the libc implementation?
> That is, for example, glibc should define SYS_futex as SYS_futex_time64
> in case of riscv 32 bit because of its support to 64 bit time_t?

Actually, there might be a somewhat less ugly workaround, if we decide
to put a correct wrapper into the exported kernel headers, e.g. adding
something along these lines to include/uapi/linux/futex.h or a new header
from the kernel:

#ifndef __KERNEL__
#include <linux/futex.h>
#include <linux/time_types.h>
#include <asm/unistd.h>
#include <sys/time.h>
#include <unistd.h>
#include <errno.h>

#define __kernel_futex __kernel_futex
typedef union __attribute__ ((__transparent_union__)) {
       const struct timespec *timeout;
       unsigned int intval;
} __kernel_futex_union;

struct __kernel_old_timespec {
       __kernel_time_t tv_sec;                 /* seconds */
       long            tv_nsec;                /* nanoseconds */
};

static inline int __kernel_futex(int *uaddr, int op, int val,
                                 __kernel_futex_union val2,
                                 unsigned *uaddr2, int val3)
{
        int cmd = op & FUTEX_CMD_MASK;
        int ret = -ENOSYS;
#ifdef __NR_futex64
        {
                if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
                    (sizeof(time_t) == sizeof(__u64))) {
                        ret = syscall(__NR_futex64, uaddr, op, val,
                                      val2.timeout, uaddr2, val3);
                }

                if (ret != -ENOSYS && ret != -EPERM)
                        return ret;
        }
#endif
#ifdef __NR_futex
        {
                if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
                    sizeof(time_t) == sizeof(__kernel_long_t)) {
                        ret = syscall(__NR_futex, uaddr, op, val,
                                       val2.timeout, uaddr2, val3);
                } else {
                        struct __kernel_old_timespec oldtimeout = {
                                .tv_sec  = val2.timeout->tv_sec,
                                .tv_nsec = val2.timeout->tv_nsec,
                        };
                        ret = syscall(__NR_futex, uaddr, op, val,
                                      &oldtimeout, uaddr2, val3);
                }
        }
#endif
        return ret;
}
#endif /* __KERNEL__

With this, applications can be changed to define their own

#ifdef __kernel_futex
#define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
      __kernel_futex(uaddr, futex_op, val, val2, uaddr2, val3)
#else
#define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
       syscall(__NR_futex, uaddr, futex_op, val, val2, uaddr2, val3)
#endif

         Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> > On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia <ingrassia@epigenesys.com> wrote:
> > >
> > > Hi,
> > >
> > > after some searching I found this patch from Yocto project:
> > >
> > > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> > >
> > > I don't know if it's ever been proposed as mainline patch.
> > >
> > > What do you think about it? Could it be the right solution?
> >
> > No. While this makes it work on rv32, it does not fix it in a
> > way that makes it work on other architectures with a time64
> > libc. If you fix it, please do it in a way that works on all architectures.
>
> Ok, I totally agree with you on this point.
>
> So, it is right to conclude that the problem is not in perf
> implementation but in the libc implementation?
> That is, for example, glibc should define SYS_futex as SYS_futex_time64
> in case of riscv 32 bit because of its support to 64 bit time_t?

Actually, there might be a somewhat less ugly workaround, if we decide
to put a correct wrapper into the exported kernel headers, e.g. adding
something along these lines to include/uapi/linux/futex.h or a new header
from the kernel:

#ifndef __KERNEL__
#include <linux/futex.h>
#include <linux/time_types.h>
#include <asm/unistd.h>
#include <sys/time.h>
#include <unistd.h>
#include <errno.h>

#define __kernel_futex __kernel_futex
typedef union __attribute__ ((__transparent_union__)) {
       const struct timespec *timeout;
       unsigned int intval;
} __kernel_futex_union;

struct __kernel_old_timespec {
       __kernel_time_t tv_sec;                 /* seconds */
       long            tv_nsec;                /* nanoseconds */
};

static inline int __kernel_futex(int *uaddr, int op, int val,
                                 __kernel_futex_union val2,
                                 unsigned *uaddr2, int val3)
{
        int cmd = op & FUTEX_CMD_MASK;
        int ret = -ENOSYS;
#ifdef __NR_futex64
        {
                if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
                    (sizeof(time_t) == sizeof(__u64))) {
                        ret = syscall(__NR_futex64, uaddr, op, val,
                                      val2.timeout, uaddr2, val3);
                }

                if (ret != -ENOSYS && ret != -EPERM)
                        return ret;
        }
#endif
#ifdef __NR_futex
        {
                if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
                    sizeof(time_t) == sizeof(__kernel_long_t)) {
                        ret = syscall(__NR_futex, uaddr, op, val,
                                       val2.timeout, uaddr2, val3);
                } else {
                        struct __kernel_old_timespec oldtimeout = {
                                .tv_sec  = val2.timeout->tv_sec,
                                .tv_nsec = val2.timeout->tv_nsec,
                        };
                        ret = syscall(__NR_futex, uaddr, op, val,
                                      &oldtimeout, uaddr2, val3);
                }
        }
#endif
        return ret;
}
#endif /* __KERNEL__

With this, applications can be changed to define their own

#ifdef __kernel_futex
#define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
      __kernel_futex(uaddr, futex_op, val, val2, uaddr2, val3)
#else
#define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
       syscall(__NR_futex, uaddr, futex_op, val, val2, uaddr2, val3)
#endif

         Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

thank you for the support.

On Mon, Jan 18, 2021 at 08:58:50PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> > > On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia <ingrassia@epigenesys.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > after some searching I found this patch from Yocto project:
> > > >
> > > > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> > > >
> > > > I don't know if it's ever been proposed as mainline patch.
> > > >
> > > > What do you think about it? Could it be the right solution?
> > >
> > > No. While this makes it work on rv32, it does not fix it in a
> > > way that makes it work on other architectures with a time64
> > > libc. If you fix it, please do it in a way that works on all architectures.
> >
> > Ok, I totally agree with you on this point.
> >
> > So, it is right to conclude that the problem is not in perf
> > implementation but in the libc implementation?
> > That is, for example, glibc should define SYS_futex as SYS_futex_time64
> > in case of riscv 32 bit because of its support to 64 bit time_t?
>
> Actually, there might be a somewhat less ugly workaround, if we decide
> to put a correct wrapper into the exported kernel headers, e.g. adding
> something along these lines to include/uapi/linux/futex.h or a new header
> from the kernel:
>
> #ifndef __KERNEL__
> #include <linux/futex.h>
> #include <linux/time_types.h>
> #include <asm/unistd.h>
> #include <sys/time.h>
> #include <unistd.h>
> #include <errno.h>
>
> #define __kernel_futex __kernel_futex
> typedef union __attribute__ ((__transparent_union__)) {
>        const struct timespec *timeout;
>        unsigned int intval;
> } __kernel_futex_union;
>
> struct __kernel_old_timespec {
>        __kernel_time_t tv_sec;                 /* seconds */
>        long            tv_nsec;                /* nanoseconds */
> };
>
> static inline int __kernel_futex(int *uaddr, int op, int val,
>                                  __kernel_futex_union val2,
>                                  unsigned *uaddr2, int val3)
> {
>         int cmd = op & FUTEX_CMD_MASK;
>         int ret = -ENOSYS;
> #ifdef __NR_futex64
>         {
>                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
>                     (sizeof(time_t) == sizeof(__u64))) {
>                         ret = syscall(__NR_futex64, uaddr, op, val,
>                                       val2.timeout, uaddr2, val3);
>                 }
>
>                 if (ret != -ENOSYS && ret != -EPERM)
>                         return ret;
>         }
> #endif
> #ifdef __NR_futex
>         {
>                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
>                     sizeof(time_t) == sizeof(__kernel_long_t)) {
>                         ret = syscall(__NR_futex, uaddr, op, val,
>                                        val2.timeout, uaddr2, val3);
>                 } else {
>                         struct __kernel_old_timespec oldtimeout = {
>                                 .tv_sec  = val2.timeout->tv_sec,
>                                 .tv_nsec = val2.timeout->tv_nsec,
>                         };
>                         ret = syscall(__NR_futex, uaddr, op, val,
>                                       &oldtimeout, uaddr2, val3);
>                 }
>         }
> #endif
>         return ret;
> }
> #endif /* __KERNEL__
>
> With this, applications can be changed to define their own
>
> #ifdef __kernel_futex
> #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
>       __kernel_futex(uaddr, futex_op, val, val2, uaddr2, val3)
> #else
> #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
>        syscall(__NR_futex, uaddr, futex_op, val, val2, uaddr2, val3)
> #endif
>
>          Arnd

I would like to do a recap on what we have so far.

The goal is to compile perf for RISCV 32 bit with glibc 2.32 which has support
for 64 bit time_t. The target kernel is 5.10.7 with option COMPAT_32BIT_TIME
enabled.

Actually in kernel 5.10.7 the file include/uapi/asm-generic/unistd.h contains
the definition of futex syscall based on some macros. In particular:
|...
|#if __BITS_PER_LONG == 32 || defined(__SYSCALL_COMPAT)
|#define __SC_3264(_nr, _32, _64) __SYSCALL(_nr, _32)
|#else
|#define __SC_3264(_nr, _32, _64) __SYSCALL(_nr, _64)
|#endif
|...
|/* kernel/futex.c */
|#if defined(__ARCH_WANT_TIME32_SYSCALLS) || __BITS_PER_LONG != 32
|#define __NR_futex 98
|__SC_3264(__NR_futex, sys_futex_time32, sys_futex)
|#endif
|...
|#define __NR_futex_time64 422
|__SYSCALL(__NR_futex_time64, sys_futex)
|...

So, in case of 64 bit architecture or 32 bit architecture with macro
__ARCH_WANT_TIME32_SYSCALLS defined, __NR_futex is defined and the proper
syscall associated with it.

In other cases, that is 32 bit architecture without support for time32 syscalls,
__NR_futex is not defined. __NR_futex_time64 is defined instead.

The syscall futex() expects timespec kernel structure, while futex_time32()
takes the old_timespec32 kernel structure as timer parameter.

To call futex() syscall in userspace it is necessary to use syscall() and
SYS_futex. In particular this is true in glibc which does not provide a wrapper
for it.

The userspace header file bits/syscall.h contains the needed definitions:
|...
|#ifdef __NR_futex
|# define SYS_futex __NR_futex
|#endif
|
|#ifdef __NR_futex_time64
|# define SYS_futex_time64 __NR_futex_time64
|#endif
|...

In case of architecture without __NR_futex definition nor futex() wrapper,
the syscall function should be called with SYS_futex_time64 instead of the
classic SYS_futex. This is not backward compatible and also not documented
as far as I know.

How this problem is solved in real scenarios? For example how can pthread
library, which uses futex syscall, be compiled correctly in glibc?

The solution is quite simple, glibc has a file called sysdep.h which in case of
RISCV 32 bit contains the following workarounds:
|...
|#if __WORDSIZE == 32
|
|/* 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
|...

So, in that case, __NR_futex is defined as __NR_futex_time64 and so SYS_futex is
defined and usable.

But this file can not be seen and used by userspace application.

So, what should be the right solution here?

Should the kernel export only __NR_futex and then check the timer struct
dimension and distinguish the various cases (time32 vs time64 futex)?

Or should the libc implementation take care to define SYS_futex once and for all
and then use the proper __NR_futex{_time64} based on its definition of time_t?

Considering that this is not only a problem of futex but of many others syscalls
(e.g. __NR_clock_gettime{64}, __NR_clock_settime{64}, and so on), probably
I would argue that the problem should be solved in the libc implementation
in some way.

What do you think?


Thank you, best regards.

Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

thank you for the support.

On Mon, Jan 18, 2021 at 08:58:50PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> > > On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia <ingrassia@epigenesys.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > after some searching I found this patch from Yocto project:
> > > >
> > > > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> > > >
> > > > I don't know if it's ever been proposed as mainline patch.
> > > >
> > > > What do you think about it? Could it be the right solution?
> > >
> > > No. While this makes it work on rv32, it does not fix it in a
> > > way that makes it work on other architectures with a time64
> > > libc. If you fix it, please do it in a way that works on all architectures.
> >
> > Ok, I totally agree with you on this point.
> >
> > So, it is right to conclude that the problem is not in perf
> > implementation but in the libc implementation?
> > That is, for example, glibc should define SYS_futex as SYS_futex_time64
> > in case of riscv 32 bit because of its support to 64 bit time_t?
>
> Actually, there might be a somewhat less ugly workaround, if we decide
> to put a correct wrapper into the exported kernel headers, e.g. adding
> something along these lines to include/uapi/linux/futex.h or a new header
> from the kernel:
>
> #ifndef __KERNEL__
> #include <linux/futex.h>
> #include <linux/time_types.h>
> #include <asm/unistd.h>
> #include <sys/time.h>
> #include <unistd.h>
> #include <errno.h>
>
> #define __kernel_futex __kernel_futex
> typedef union __attribute__ ((__transparent_union__)) {
>        const struct timespec *timeout;
>        unsigned int intval;
> } __kernel_futex_union;
>
> struct __kernel_old_timespec {
>        __kernel_time_t tv_sec;                 /* seconds */
>        long            tv_nsec;                /* nanoseconds */
> };
>
> static inline int __kernel_futex(int *uaddr, int op, int val,
>                                  __kernel_futex_union val2,
>                                  unsigned *uaddr2, int val3)
> {
>         int cmd = op & FUTEX_CMD_MASK;
>         int ret = -ENOSYS;
> #ifdef __NR_futex64
>         {
>                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
>                     (sizeof(time_t) == sizeof(__u64))) {
>                         ret = syscall(__NR_futex64, uaddr, op, val,
>                                       val2.timeout, uaddr2, val3);
>                 }
>
>                 if (ret != -ENOSYS && ret != -EPERM)
>                         return ret;
>         }
> #endif
> #ifdef __NR_futex
>         {
>                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
>                     sizeof(time_t) == sizeof(__kernel_long_t)) {
>                         ret = syscall(__NR_futex, uaddr, op, val,
>                                        val2.timeout, uaddr2, val3);
>                 } else {
>                         struct __kernel_old_timespec oldtimeout = {
>                                 .tv_sec  = val2.timeout->tv_sec,
>                                 .tv_nsec = val2.timeout->tv_nsec,
>                         };
>                         ret = syscall(__NR_futex, uaddr, op, val,
>                                       &oldtimeout, uaddr2, val3);
>                 }
>         }
> #endif
>         return ret;
> }
> #endif /* __KERNEL__
>
> With this, applications can be changed to define their own
>
> #ifdef __kernel_futex
> #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
>       __kernel_futex(uaddr, futex_op, val, val2, uaddr2, val3)
> #else
> #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
>        syscall(__NR_futex, uaddr, futex_op, val, val2, uaddr2, val3)
> #endif
>
>          Arnd

I would like to do a recap on what we have so far.

The goal is to compile perf for RISCV 32 bit with glibc 2.32 which has support
for 64 bit time_t. The target kernel is 5.10.7 with option COMPAT_32BIT_TIME
enabled.

Actually in kernel 5.10.7 the file include/uapi/asm-generic/unistd.h contains
the definition of futex syscall based on some macros. In particular:
|...
|#if __BITS_PER_LONG == 32 || defined(__SYSCALL_COMPAT)
|#define __SC_3264(_nr, _32, _64) __SYSCALL(_nr, _32)
|#else
|#define __SC_3264(_nr, _32, _64) __SYSCALL(_nr, _64)
|#endif
|...
|/* kernel/futex.c */
|#if defined(__ARCH_WANT_TIME32_SYSCALLS) || __BITS_PER_LONG != 32
|#define __NR_futex 98
|__SC_3264(__NR_futex, sys_futex_time32, sys_futex)
|#endif
|...
|#define __NR_futex_time64 422
|__SYSCALL(__NR_futex_time64, sys_futex)
|...

So, in case of 64 bit architecture or 32 bit architecture with macro
__ARCH_WANT_TIME32_SYSCALLS defined, __NR_futex is defined and the proper
syscall associated with it.

In other cases, that is 32 bit architecture without support for time32 syscalls,
__NR_futex is not defined. __NR_futex_time64 is defined instead.

The syscall futex() expects timespec kernel structure, while futex_time32()
takes the old_timespec32 kernel structure as timer parameter.

To call futex() syscall in userspace it is necessary to use syscall() and
SYS_futex. In particular this is true in glibc which does not provide a wrapper
for it.

The userspace header file bits/syscall.h contains the needed definitions:
|...
|#ifdef __NR_futex
|# define SYS_futex __NR_futex
|#endif
|
|#ifdef __NR_futex_time64
|# define SYS_futex_time64 __NR_futex_time64
|#endif
|...

In case of architecture without __NR_futex definition nor futex() wrapper,
the syscall function should be called with SYS_futex_time64 instead of the
classic SYS_futex. This is not backward compatible and also not documented
as far as I know.

How this problem is solved in real scenarios? For example how can pthread
library, which uses futex syscall, be compiled correctly in glibc?

The solution is quite simple, glibc has a file called sysdep.h which in case of
RISCV 32 bit contains the following workarounds:
|...
|#if __WORDSIZE == 32
|
|/* 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
|...

So, in that case, __NR_futex is defined as __NR_futex_time64 and so SYS_futex is
defined and usable.

But this file can not be seen and used by userspace application.

So, what should be the right solution here?

Should the kernel export only __NR_futex and then check the timer struct
dimension and distinguish the various cases (time32 vs time64 futex)?

Or should the libc implementation take care to define SYS_futex once and for all
and then use the proper __NR_futex{_time64} based on its definition of time_t?

Considering that this is not only a problem of futex but of many others syscalls
(e.g. __NR_clock_gettime{64}, __NR_clock_settime{64}, and so on), probably
I would argue that the problem should be solved in the libc implementation
in some way.

What do you think?


Thank you, best regards.

Emiliano

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Tue, Jan 19, 2021 at 5:53 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> On Mon, Jan 18, 2021 at 08:58:50PM +0100, Arnd Bergmann wrote:
> > On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
> > <ingrassia@epigenesys.com> wrote:
> > > On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> > > > On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia <ingrassia@epigenesys.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > after some searching I found this patch from Yocto project:
> > > > >
> > > > > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> > > > >
> > > > > I don't know if it's ever been proposed as mainline patch.
> > > > >
> > > > > What do you think about it? Could it be the right solution?
> > > >
> > > > No. While this makes it work on rv32, it does not fix it in a
> > > > way that makes it work on other architectures with a time64
> > > > libc. If you fix it, please do it in a way that works on all architectures.
> > >
> > > Ok, I totally agree with you on this point.
> > >
> > > So, it is right to conclude that the problem is not in perf
> > > implementation but in the libc implementation?
> > > That is, for example, glibc should define SYS_futex as SYS_futex_time64
> > > in case of riscv 32 bit because of its support to 64 bit time_t?
> >
> > Actually, there might be a somewhat less ugly workaround, if we decide
> > to put a correct wrapper into the exported kernel headers, e.g. adding
> > something along these lines to include/uapi/linux/futex.h or a new header
> > from the kernel:
> >
> > #ifndef __KERNEL__
> > #include <linux/futex.h>
> > #include <linux/time_types.h>
> > #include <asm/unistd.h>
> > #include <sys/time.h>
> > #include <unistd.h>
> > #include <errno.h>
> >
> > #define __kernel_futex __kernel_futex
> > typedef union __attribute__ ((__transparent_union__)) {
> >        const struct timespec *timeout;
> >        unsigned int intval;
> > } __kernel_futex_union;
> >
> > struct __kernel_old_timespec {
> >        __kernel_time_t tv_sec;                 /* seconds */
> >        long            tv_nsec;                /* nanoseconds */
> > };
> >
> > static inline int __kernel_futex(int *uaddr, int op, int val,
> >                                  __kernel_futex_union val2,
> >                                  unsigned *uaddr2, int val3)
> > {
> >         int cmd = op & FUTEX_CMD_MASK;
> >         int ret = -ENOSYS;
> > #ifdef __NR_futex64
> >         {
> >                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
> >                     (sizeof(time_t) == sizeof(__u64))) {
> >                         ret = syscall(__NR_futex64, uaddr, op, val,
> >                                       val2.timeout, uaddr2, val3);
> >                 }
> >
> >                 if (ret != -ENOSYS && ret != -EPERM)
> >                         return ret;
> >         }
> > #endif
> > #ifdef __NR_futex
> >         {
> >                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
> >                     sizeof(time_t) == sizeof(__kernel_long_t)) {
> >                         ret = syscall(__NR_futex, uaddr, op, val,
> >                                        val2.timeout, uaddr2, val3);
> >                 } else {
> >                         struct __kernel_old_timespec oldtimeout = {
> >                                 .tv_sec  = val2.timeout->tv_sec,
> >                                 .tv_nsec = val2.timeout->tv_nsec,
> >                         };
> >                         ret = syscall(__NR_futex, uaddr, op, val,
> >                                       &oldtimeout, uaddr2, val3);
> >                 }
> >         }
> > #endif
> >         return ret;
> > }
> > #endif /* __KERNEL__
> >
> > With this, applications can be changed to define their own
> >
> > #ifdef __kernel_futex
> > #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
> >       __kernel_futex(uaddr, futex_op, val, val2, uaddr2, val3)
> > #else
> > #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
> >        syscall(__NR_futex, uaddr, futex_op, val, val2, uaddr2, val3)
> > #endif
> >
> >          Arnd
>
> I would like to do a recap on what we have so far.
>
> The goal is to compile perf for RISCV 32 bit with glibc 2.32 which has support
> for 64 bit time_t. The target kernel is 5.10.7 with option COMPAT_32BIT_TIME
> enabled.

I didn't think that COMPAT_32BIT_TIME was even allowed on rv32,
but it seems there is a bug in Kconfig, it should be disabled for any
architecture that does not set __ARCH_WANT_TIME32_SYSCALLS,
but this should not affect the problem at hand.

> The userspace header file bits/syscall.h contains the needed definitions:
> |...
> |#ifdef __NR_futex
> |# define SYS_futex __NR_futex
> |#endif
> |
> |#ifdef __NR_futex_time64
> |# define SYS_futex_time64 __NR_futex_time64
> |#endif
> |...
>
> In case of architecture without __NR_futex definition nor futex() wrapper,
> the syscall function should be called with SYS_futex_time64 instead of the
> classic SYS_futex. This is not backward compatible and also not documented
> as far as I know.
>
> How this problem is solved in real scenarios? For example how can pthread
> library, which uses futex syscall, be compiled correctly in glibc?

The pthread library is built as part of glibc, and it uses its own local
(architecture specific) workaround.

> The solution is quite simple, glibc has a file called sysdep.h which in case of
> RISCV 32 bit contains the following workarounds:
> |...
> |#if __WORDSIZE == 32
> |
> |/* 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
> |...
>
> So, in that case, __NR_futex is defined as __NR_futex_time64 and so SYS_futex is
> defined and usable.
>
> But this file can not be seen and used by userspace application.

Right, this is intentional, redefining __NR_futex would break any
correctly written application that passes a __kernel_old_timespec
structure when using __NR_futex.

> So, what should be the right solution here?
>
> Should the kernel export only __NR_futex and then check the timer struct
> dimension and distinguish the various cases (time32 vs time64 futex)?
>
> Or should the libc implementation take care to define SYS_futex once and for all
> and then use the proper __NR_futex{_time64} based on its definition of time_t?
>
> Considering that this is not only a problem of futex but of many others syscalls
> (e.g. __NR_clock_gettime{64}, __NR_clock_settime{64}, and so on), probably
> I would argue that the problem should be solved in the libc implementation
> in some way.
>
> What do you think?

The fundamental problem is mixing glibc interfaces with kernel interfaces,
which is broken in many ways. You would hit a similar issue when using
__NR_stat with 'struct stat' from glibc instead of 'struct __old_kernel_stat'.

Ideally, an application would just use the kernel types when calling
the kernel interfaces, or use the libc types and call a libc-provided
function (which glibc does not provide for futex).

There is nothing that glibc or kernel could do here to fix existing
applications, they all have to be changed manually to use a correct
interface when built against a time64 glibc or musl but directly
calling low-level syscall interfaces including but not limited to futex.

rv32 is in the unfortunate position of being the first one that has a glibc
port, so it hits a lot of problems that other architectures do not.
If the applications get fixed properly, then at least they will also
work with musl-1.2 and a (will work-in-progress) glibc that will
eventually allow building with 64-bit time_t.

One thing that glibc can do to help would be to #undef all the
time32 SYS_* macros when 64-bit time_t is used, to force a
build error on broken applications that try to use the wrong types,
but this is also problematic since you sometimes want to use
the numbers for something other than calling syscall(), e.g.
in strace or seccomp.

      Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Tue, Jan 19, 2021 at 5:53 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> On Mon, Jan 18, 2021 at 08:58:50PM +0100, Arnd Bergmann wrote:
> > On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
> > <ingrassia@epigenesys.com> wrote:
> > > On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> > > > On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia <ingrassia@epigenesys.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > after some searching I found this patch from Yocto project:
> > > > >
> > > > > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> > > > >
> > > > > I don't know if it's ever been proposed as mainline patch.
> > > > >
> > > > > What do you think about it? Could it be the right solution?
> > > >
> > > > No. While this makes it work on rv32, it does not fix it in a
> > > > way that makes it work on other architectures with a time64
> > > > libc. If you fix it, please do it in a way that works on all architectures.
> > >
> > > Ok, I totally agree with you on this point.
> > >
> > > So, it is right to conclude that the problem is not in perf
> > > implementation but in the libc implementation?
> > > That is, for example, glibc should define SYS_futex as SYS_futex_time64
> > > in case of riscv 32 bit because of its support to 64 bit time_t?
> >
> > Actually, there might be a somewhat less ugly workaround, if we decide
> > to put a correct wrapper into the exported kernel headers, e.g. adding
> > something along these lines to include/uapi/linux/futex.h or a new header
> > from the kernel:
> >
> > #ifndef __KERNEL__
> > #include <linux/futex.h>
> > #include <linux/time_types.h>
> > #include <asm/unistd.h>
> > #include <sys/time.h>
> > #include <unistd.h>
> > #include <errno.h>
> >
> > #define __kernel_futex __kernel_futex
> > typedef union __attribute__ ((__transparent_union__)) {
> >        const struct timespec *timeout;
> >        unsigned int intval;
> > } __kernel_futex_union;
> >
> > struct __kernel_old_timespec {
> >        __kernel_time_t tv_sec;                 /* seconds */
> >        long            tv_nsec;                /* nanoseconds */
> > };
> >
> > static inline int __kernel_futex(int *uaddr, int op, int val,
> >                                  __kernel_futex_union val2,
> >                                  unsigned *uaddr2, int val3)
> > {
> >         int cmd = op & FUTEX_CMD_MASK;
> >         int ret = -ENOSYS;
> > #ifdef __NR_futex64
> >         {
> >                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
> >                     (sizeof(time_t) == sizeof(__u64))) {
> >                         ret = syscall(__NR_futex64, uaddr, op, val,
> >                                       val2.timeout, uaddr2, val3);
> >                 }
> >
> >                 if (ret != -ENOSYS && ret != -EPERM)
> >                         return ret;
> >         }
> > #endif
> > #ifdef __NR_futex
> >         {
> >                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
> >                     sizeof(time_t) == sizeof(__kernel_long_t)) {
> >                         ret = syscall(__NR_futex, uaddr, op, val,
> >                                        val2.timeout, uaddr2, val3);
> >                 } else {
> >                         struct __kernel_old_timespec oldtimeout = {
> >                                 .tv_sec  = val2.timeout->tv_sec,
> >                                 .tv_nsec = val2.timeout->tv_nsec,
> >                         };
> >                         ret = syscall(__NR_futex, uaddr, op, val,
> >                                       &oldtimeout, uaddr2, val3);
> >                 }
> >         }
> > #endif
> >         return ret;
> > }
> > #endif /* __KERNEL__
> >
> > With this, applications can be changed to define their own
> >
> > #ifdef __kernel_futex
> > #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
> >       __kernel_futex(uaddr, futex_op, val, val2, uaddr2, val3)
> > #else
> > #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
> >        syscall(__NR_futex, uaddr, futex_op, val, val2, uaddr2, val3)
> > #endif
> >
> >          Arnd
>
> I would like to do a recap on what we have so far.
>
> The goal is to compile perf for RISCV 32 bit with glibc 2.32 which has support
> for 64 bit time_t. The target kernel is 5.10.7 with option COMPAT_32BIT_TIME
> enabled.

I didn't think that COMPAT_32BIT_TIME was even allowed on rv32,
but it seems there is a bug in Kconfig, it should be disabled for any
architecture that does not set __ARCH_WANT_TIME32_SYSCALLS,
but this should not affect the problem at hand.

> The userspace header file bits/syscall.h contains the needed definitions:
> |...
> |#ifdef __NR_futex
> |# define SYS_futex __NR_futex
> |#endif
> |
> |#ifdef __NR_futex_time64
> |# define SYS_futex_time64 __NR_futex_time64
> |#endif
> |...
>
> In case of architecture without __NR_futex definition nor futex() wrapper,
> the syscall function should be called with SYS_futex_time64 instead of the
> classic SYS_futex. This is not backward compatible and also not documented
> as far as I know.
>
> How this problem is solved in real scenarios? For example how can pthread
> library, which uses futex syscall, be compiled correctly in glibc?

The pthread library is built as part of glibc, and it uses its own local
(architecture specific) workaround.

> The solution is quite simple, glibc has a file called sysdep.h which in case of
> RISCV 32 bit contains the following workarounds:
> |...
> |#if __WORDSIZE == 32
> |
> |/* 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
> |...
>
> So, in that case, __NR_futex is defined as __NR_futex_time64 and so SYS_futex is
> defined and usable.
>
> But this file can not be seen and used by userspace application.

Right, this is intentional, redefining __NR_futex would break any
correctly written application that passes a __kernel_old_timespec
structure when using __NR_futex.

> So, what should be the right solution here?
>
> Should the kernel export only __NR_futex and then check the timer struct
> dimension and distinguish the various cases (time32 vs time64 futex)?
>
> Or should the libc implementation take care to define SYS_futex once and for all
> and then use the proper __NR_futex{_time64} based on its definition of time_t?
>
> Considering that this is not only a problem of futex but of many others syscalls
> (e.g. __NR_clock_gettime{64}, __NR_clock_settime{64}, and so on), probably
> I would argue that the problem should be solved in the libc implementation
> in some way.
>
> What do you think?

The fundamental problem is mixing glibc interfaces with kernel interfaces,
which is broken in many ways. You would hit a similar issue when using
__NR_stat with 'struct stat' from glibc instead of 'struct __old_kernel_stat'.

Ideally, an application would just use the kernel types when calling
the kernel interfaces, or use the libc types and call a libc-provided
function (which glibc does not provide for futex).

There is nothing that glibc or kernel could do here to fix existing
applications, they all have to be changed manually to use a correct
interface when built against a time64 glibc or musl but directly
calling low-level syscall interfaces including but not limited to futex.

rv32 is in the unfortunate position of being the first one that has a glibc
port, so it hits a lot of problems that other architectures do not.
If the applications get fixed properly, then at least they will also
work with musl-1.2 and a (will work-in-progress) glibc that will
eventually allow building with 64-bit time_t.

One thing that glibc can do to help would be to #undef all the
time32 SYS_* macros when 64-bit time_t is used, to force a
build error on broken applications that try to use the wrong types,
but this is also problematic since you sometimes want to use
the numbers for something other than calling syscall(), e.g.
in strace or seccomp.

      Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Tue, Jan 19, 2021 at 08:47:59PM +0100, Arnd Bergmann wrote:
> On Tue, Jan 19, 2021 at 5:53 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > On Mon, Jan 18, 2021 at 08:58:50PM +0100, Arnd Bergmann wrote:
> > > On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
> > > <ingrassia@epigenesys.com> wrote:
> > > > On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> > > > > On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia <ingrassia@epigenesys.com> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > after some searching I found this patch from Yocto project:
> > > > > >
> > > > > > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> > > > > >
> > > > > > I don't know if it's ever been proposed as mainline patch.
> > > > > >
> > > > > > What do you think about it? Could it be the right solution?
> > > > >
> > > > > No. While this makes it work on rv32, it does not fix it in a
> > > > > way that makes it work on other architectures with a time64
> > > > > libc. If you fix it, please do it in a way that works on all architectures.
> > > >
> > > > Ok, I totally agree with you on this point.
> > > >
> > > > So, it is right to conclude that the problem is not in perf
> > > > implementation but in the libc implementation?
> > > > That is, for example, glibc should define SYS_futex as SYS_futex_time64
> > > > in case of riscv 32 bit because of its support to 64 bit time_t?
> > >
> > > Actually, there might be a somewhat less ugly workaround, if we decide
> > > to put a correct wrapper into the exported kernel headers, e.g. adding
> > > something along these lines to include/uapi/linux/futex.h or a new header
> > > from the kernel:
> > >
> > > #ifndef __KERNEL__
> > > #include <linux/futex.h>
> > > #include <linux/time_types.h>
> > > #include <asm/unistd.h>
> > > #include <sys/time.h>
> > > #include <unistd.h>
> > > #include <errno.h>
> > >
> > > #define __kernel_futex __kernel_futex
> > > typedef union __attribute__ ((__transparent_union__)) {
> > >        const struct timespec *timeout;
> > >        unsigned int intval;
> > > } __kernel_futex_union;
> > >
> > > struct __kernel_old_timespec {
> > >        __kernel_time_t tv_sec;                 /* seconds */
> > >        long            tv_nsec;                /* nanoseconds */
> > > };
> > >
> > > static inline int __kernel_futex(int *uaddr, int op, int val,
> > >                                  __kernel_futex_union val2,
> > >                                  unsigned *uaddr2, int val3)
> > > {
> > >         int cmd = op & FUTEX_CMD_MASK;
> > >         int ret = -ENOSYS;
> > > #ifdef __NR_futex64
> > >         {
> > >                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
> > >                     (sizeof(time_t) == sizeof(__u64))) {
> > >                         ret = syscall(__NR_futex64, uaddr, op, val,
> > >                                       val2.timeout, uaddr2, val3);
> > >                 }
> > >
> > >                 if (ret != -ENOSYS && ret != -EPERM)
> > >                         return ret;
> > >         }
> > > #endif
> > > #ifdef __NR_futex
> > >         {
> > >                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
> > >                     sizeof(time_t) == sizeof(__kernel_long_t)) {
> > >                         ret = syscall(__NR_futex, uaddr, op, val,
> > >                                        val2.timeout, uaddr2, val3);
> > >                 } else {
> > >                         struct __kernel_old_timespec oldtimeout = {
> > >                                 .tv_sec  = val2.timeout->tv_sec,
> > >                                 .tv_nsec = val2.timeout->tv_nsec,
> > >                         };
> > >                         ret = syscall(__NR_futex, uaddr, op, val,
> > >                                       &oldtimeout, uaddr2, val3);
> > >                 }
> > >         }
> > > #endif
> > >         return ret;
> > > }
> > > #endif /* __KERNEL__
> > >
> > > With this, applications can be changed to define their own
> > >
> > > #ifdef __kernel_futex
> > > #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
> > >       __kernel_futex(uaddr, futex_op, val, val2, uaddr2, val3)
> > > #else
> > > #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
> > >        syscall(__NR_futex, uaddr, futex_op, val, val2, uaddr2, val3)
> > > #endif
> > >
> > >          Arnd
> >
> > I would like to do a recap on what we have so far.
> >
> > The goal is to compile perf for RISCV 32 bit with glibc 2.32 which has support
> > for 64 bit time_t. The target kernel is 5.10.7 with option COMPAT_32BIT_TIME
> > enabled.
>
> I didn't think that COMPAT_32BIT_TIME was even allowed on rv32,
> but it seems there is a bug in Kconfig, it should be disabled for any
> architecture that does not set __ARCH_WANT_TIME32_SYSCALLS,
> but this should not affect the problem at hand.

Ok, will you submit a patch for this bug?

>
> > The userspace header file bits/syscall.h contains the needed definitions:
> > |...
> > |#ifdef __NR_futex
> > |# define SYS_futex __NR_futex
> > |#endif
> > |
> > |#ifdef __NR_futex_time64
> > |# define SYS_futex_time64 __NR_futex_time64
> > |#endif
> > |...
> >
> > In case of architecture without __NR_futex definition nor futex() wrapper,
> > the syscall function should be called with SYS_futex_time64 instead of the
> > classic SYS_futex. This is not backward compatible and also not documented
> > as far as I know.
> >
> > How this problem is solved in real scenarios? For example how can pthread
> > library, which uses futex syscall, be compiled correctly in glibc?
>
> The pthread library is built as part of glibc, and it uses its own local
> (architecture specific) workaround.
>
> > The solution is quite simple, glibc has a file called sysdep.h which in case of
> > RISCV 32 bit contains the following workarounds:
> > |...
> > |#if __WORDSIZE == 32
> > |
> > |/* 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
> > |...
> >
> > So, in that case, __NR_futex is defined as __NR_futex_time64 and so SYS_futex is
> > defined and usable.
> >
> > But this file can not be seen and used by userspace application.
>
> Right, this is intentional, redefining __NR_futex would break any
> correctly written application that passes a __kernel_old_timespec
> structure when using __NR_futex.
>
> > So, what should be the right solution here?
> >
> > Should the kernel export only __NR_futex and then check the timer struct
> > dimension and distinguish the various cases (time32 vs time64 futex)?
> >
> > Or should the libc implementation take care to define SYS_futex once and for all
> > and then use the proper __NR_futex{_time64} based on its definition of time_t?
> >
> > Considering that this is not only a problem of futex but of many others syscalls
> > (e.g. __NR_clock_gettime{64}, __NR_clock_settime{64}, and so on), probably
> > I would argue that the problem should be solved in the libc implementation
> > in some way.
> >
> > What do you think?
>
> The fundamental problem is mixing glibc interfaces with kernel interfaces,
> which is broken in many ways. You would hit a similar issue when using
> __NR_stat with 'struct stat' from glibc instead of 'struct __old_kernel_stat'.
>
> Ideally, an application would just use the kernel types when calling
> the kernel interfaces, or use the libc types and call a libc-provided
> function (which glibc does not provide for futex).
>
> There is nothing that glibc or kernel could do here to fix existing
> applications, they all have to be changed manually to use a correct
> interface when built against a time64 glibc or musl but directly
> calling low-level syscall interfaces including but not limited to futex.
>

Here is the example futex app corrected w.r.t. all the previous
considerations:

|#include <features.h>
|#if (defined __GLIBC__) && (__GLIBC__ >= 2 && __GLIBC_MINOR__ >= 32)
|#include <asm/unistd.h>
|#include <bits/timesize.h>
|# ifndef __NR_futex
|#  if defined (__NR_futex_time64) && (__TIMESIZE == 64)
|#   define __NR_futex	__NR_futex_time64
|#  endif
|# endif
|#endif
|
|#include <unistd.h>
|#include <sys/syscall.h>
|#include <linux/futex.h>
|#include <stdint.h>
|#include <sys/time.h>
|
|static int
|futex(uint32_t *uaddr, int futex_op, uint32_t val,
|      const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
|{
|	return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3);
|}
|
|int main(void)
|{
|	return futex(NULL, FUTEX_WAKE, 0, NULL, NULL, 0);
|}

In this case, __NR_futex is defined only if __NR_futex_time64 is defined
and if the time_t size is 64 bit. In this way SYS_futex will be defined
only if __NR_futex is defined, without directly defining the libc macro.

This solve the problem in glibc case but is quite ugly.
About musl, the support for riscv 32 bit is on the roadmap without a
prevision about that (https://wiki.musl-libc.org/roadmap.html).

> rv32 is in the unfortunate position of being the first one that has a glibc
> port, so it hits a lot of problems that other architectures do not.
> If the applications get fixed properly, then at least they will also
> work with musl-1.2 and a (will work-in-progress) glibc that will
> eventually allow building with 64-bit time_t.

Ok, so in case of perf we probably should limit the needed patch to
riscv 32 bit architecture, unless we want to solve the problem for all
32 bit architecture with time_t 64 bit support.

How should we proceed?

>
> One thing that glibc can do to help would be to #undef all the
> time32 SYS_* macros when 64-bit time_t is used, to force a
> build error on broken applications that try to use the wrong types,
> but this is also problematic since you sometimes want to use
> the numbers for something other than calling syscall(), e.g.
> in strace or seccomp.
>
>       Arnd

Thank you, best regards.

Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Tue, Jan 19, 2021 at 08:47:59PM +0100, Arnd Bergmann wrote:
> On Tue, Jan 19, 2021 at 5:53 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > On Mon, Jan 18, 2021 at 08:58:50PM +0100, Arnd Bergmann wrote:
> > > On Mon, Jan 18, 2021 at 4:33 PM Emiliano Ingrassia
> > > <ingrassia@epigenesys.com> wrote:
> > > > On Mon, Jan 18, 2021 at 04:19:47PM +0100, Arnd Bergmann wrote:
> > > > > On Mon, Jan 18, 2021 at 3:53 PM Emiliano Ingrassia <ingrassia@epigenesys.com> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > after some searching I found this patch from Yocto project:
> > > > > >
> > > > > > https://www.mail-archive.com/linux-yocto@lists.yoctoproject.org/msg01004.html.
> > > > > >
> > > > > > I don't know if it's ever been proposed as mainline patch.
> > > > > >
> > > > > > What do you think about it? Could it be the right solution?
> > > > >
> > > > > No. While this makes it work on rv32, it does not fix it in a
> > > > > way that makes it work on other architectures with a time64
> > > > > libc. If you fix it, please do it in a way that works on all architectures.
> > > >
> > > > Ok, I totally agree with you on this point.
> > > >
> > > > So, it is right to conclude that the problem is not in perf
> > > > implementation but in the libc implementation?
> > > > That is, for example, glibc should define SYS_futex as SYS_futex_time64
> > > > in case of riscv 32 bit because of its support to 64 bit time_t?
> > >
> > > Actually, there might be a somewhat less ugly workaround, if we decide
> > > to put a correct wrapper into the exported kernel headers, e.g. adding
> > > something along these lines to include/uapi/linux/futex.h or a new header
> > > from the kernel:
> > >
> > > #ifndef __KERNEL__
> > > #include <linux/futex.h>
> > > #include <linux/time_types.h>
> > > #include <asm/unistd.h>
> > > #include <sys/time.h>
> > > #include <unistd.h>
> > > #include <errno.h>
> > >
> > > #define __kernel_futex __kernel_futex
> > > typedef union __attribute__ ((__transparent_union__)) {
> > >        const struct timespec *timeout;
> > >        unsigned int intval;
> > > } __kernel_futex_union;
> > >
> > > struct __kernel_old_timespec {
> > >        __kernel_time_t tv_sec;                 /* seconds */
> > >        long            tv_nsec;                /* nanoseconds */
> > > };
> > >
> > > static inline int __kernel_futex(int *uaddr, int op, int val,
> > >                                  __kernel_futex_union val2,
> > >                                  unsigned *uaddr2, int val3)
> > > {
> > >         int cmd = op & FUTEX_CMD_MASK;
> > >         int ret = -ENOSYS;
> > > #ifdef __NR_futex64
> > >         {
> > >                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
> > >                     (sizeof(time_t) == sizeof(__u64))) {
> > >                         ret = syscall(__NR_futex64, uaddr, op, val,
> > >                                       val2.timeout, uaddr2, val3);
> > >                 }
> > >
> > >                 if (ret != -ENOSYS && ret != -EPERM)
> > >                         return ret;
> > >         }
> > > #endif
> > > #ifdef __NR_futex
> > >         {
> > >                 if (((cmd != FUTEX_WAIT) && (cmd != FUTEX_WAIT_BITSET)) ||
> > >                     sizeof(time_t) == sizeof(__kernel_long_t)) {
> > >                         ret = syscall(__NR_futex, uaddr, op, val,
> > >                                        val2.timeout, uaddr2, val3);
> > >                 } else {
> > >                         struct __kernel_old_timespec oldtimeout = {
> > >                                 .tv_sec  = val2.timeout->tv_sec,
> > >                                 .tv_nsec = val2.timeout->tv_nsec,
> > >                         };
> > >                         ret = syscall(__NR_futex, uaddr, op, val,
> > >                                       &oldtimeout, uaddr2, val3);
> > >                 }
> > >         }
> > > #endif
> > >         return ret;
> > > }
> > > #endif /* __KERNEL__
> > >
> > > With this, applications can be changed to define their own
> > >
> > > #ifdef __kernel_futex
> > > #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
> > >       __kernel_futex(uaddr, futex_op, val, val2, uaddr2, val3)
> > > #else
> > > #define futex(uaddr, futex_op, val, val2, uaddr2, val3) \
> > >        syscall(__NR_futex, uaddr, futex_op, val, val2, uaddr2, val3)
> > > #endif
> > >
> > >          Arnd
> >
> > I would like to do a recap on what we have so far.
> >
> > The goal is to compile perf for RISCV 32 bit with glibc 2.32 which has support
> > for 64 bit time_t. The target kernel is 5.10.7 with option COMPAT_32BIT_TIME
> > enabled.
>
> I didn't think that COMPAT_32BIT_TIME was even allowed on rv32,
> but it seems there is a bug in Kconfig, it should be disabled for any
> architecture that does not set __ARCH_WANT_TIME32_SYSCALLS,
> but this should not affect the problem at hand.

Ok, will you submit a patch for this bug?

>
> > The userspace header file bits/syscall.h contains the needed definitions:
> > |...
> > |#ifdef __NR_futex
> > |# define SYS_futex __NR_futex
> > |#endif
> > |
> > |#ifdef __NR_futex_time64
> > |# define SYS_futex_time64 __NR_futex_time64
> > |#endif
> > |...
> >
> > In case of architecture without __NR_futex definition nor futex() wrapper,
> > the syscall function should be called with SYS_futex_time64 instead of the
> > classic SYS_futex. This is not backward compatible and also not documented
> > as far as I know.
> >
> > How this problem is solved in real scenarios? For example how can pthread
> > library, which uses futex syscall, be compiled correctly in glibc?
>
> The pthread library is built as part of glibc, and it uses its own local
> (architecture specific) workaround.
>
> > The solution is quite simple, glibc has a file called sysdep.h which in case of
> > RISCV 32 bit contains the following workarounds:
> > |...
> > |#if __WORDSIZE == 32
> > |
> > |/* 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
> > |...
> >
> > So, in that case, __NR_futex is defined as __NR_futex_time64 and so SYS_futex is
> > defined and usable.
> >
> > But this file can not be seen and used by userspace application.
>
> Right, this is intentional, redefining __NR_futex would break any
> correctly written application that passes a __kernel_old_timespec
> structure when using __NR_futex.
>
> > So, what should be the right solution here?
> >
> > Should the kernel export only __NR_futex and then check the timer struct
> > dimension and distinguish the various cases (time32 vs time64 futex)?
> >
> > Or should the libc implementation take care to define SYS_futex once and for all
> > and then use the proper __NR_futex{_time64} based on its definition of time_t?
> >
> > Considering that this is not only a problem of futex but of many others syscalls
> > (e.g. __NR_clock_gettime{64}, __NR_clock_settime{64}, and so on), probably
> > I would argue that the problem should be solved in the libc implementation
> > in some way.
> >
> > What do you think?
>
> The fundamental problem is mixing glibc interfaces with kernel interfaces,
> which is broken in many ways. You would hit a similar issue when using
> __NR_stat with 'struct stat' from glibc instead of 'struct __old_kernel_stat'.
>
> Ideally, an application would just use the kernel types when calling
> the kernel interfaces, or use the libc types and call a libc-provided
> function (which glibc does not provide for futex).
>
> There is nothing that glibc or kernel could do here to fix existing
> applications, they all have to be changed manually to use a correct
> interface when built against a time64 glibc or musl but directly
> calling low-level syscall interfaces including but not limited to futex.
>

Here is the example futex app corrected w.r.t. all the previous
considerations:

|#include <features.h>
|#if (defined __GLIBC__) && (__GLIBC__ >= 2 && __GLIBC_MINOR__ >= 32)
|#include <asm/unistd.h>
|#include <bits/timesize.h>
|# ifndef __NR_futex
|#  if defined (__NR_futex_time64) && (__TIMESIZE == 64)
|#   define __NR_futex	__NR_futex_time64
|#  endif
|# endif
|#endif
|
|#include <unistd.h>
|#include <sys/syscall.h>
|#include <linux/futex.h>
|#include <stdint.h>
|#include <sys/time.h>
|
|static int
|futex(uint32_t *uaddr, int futex_op, uint32_t val,
|      const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
|{
|	return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3);
|}
|
|int main(void)
|{
|	return futex(NULL, FUTEX_WAKE, 0, NULL, NULL, 0);
|}

In this case, __NR_futex is defined only if __NR_futex_time64 is defined
and if the time_t size is 64 bit. In this way SYS_futex will be defined
only if __NR_futex is defined, without directly defining the libc macro.

This solve the problem in glibc case but is quite ugly.
About musl, the support for riscv 32 bit is on the roadmap without a
prevision about that (https://wiki.musl-libc.org/roadmap.html).

> rv32 is in the unfortunate position of being the first one that has a glibc
> port, so it hits a lot of problems that other architectures do not.
> If the applications get fixed properly, then at least they will also
> work with musl-1.2 and a (will work-in-progress) glibc that will
> eventually allow building with 64-bit time_t.

Ok, so in case of perf we probably should limit the needed patch to
riscv 32 bit architecture, unless we want to solve the problem for all
32 bit architecture with time_t 64 bit support.

How should we proceed?

>
> One thing that glibc can do to help would be to #undef all the
> time32 SYS_* macros when 64-bit time_t is used, to force a
> build error on broken applications that try to use the wrong types,
> but this is also problematic since you sometimes want to use
> the numbers for something other than calling syscall(), e.g.
> in strace or seccomp.
>
>       Arnd

Thank you, best regards.

Emiliano

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Wed, Jan 20, 2021 at 3:44 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> On Tue, Jan 19, 2021 at 08:47:59PM +0100, Arnd Bergmann wrote:
> > On Tue, Jan 19, 2021 at 5:53 PM Emiliano Ingrassia
> >
> > I didn't think that COMPAT_32BIT_TIME was even allowed on rv32,
> > but it seems there is a bug in Kconfig, it should be disabled for any
> > architecture that does not set __ARCH_WANT_TIME32_SYSCALLS,
> > but this should not affect the problem at hand.
>
> Ok, will you submit a patch for this bug?

Sure, will do.
>
> Here is the example futex app corrected w.r.t. all the previous
> considerations:
>
> |#include <features.h>
> |#if (defined __GLIBC__) && (__GLIBC__ >= 2 && __GLIBC_MINOR__ >= 32)
> |#include <asm/unistd.h>
> |#include <bits/timesize.h>
> |# ifndef __NR_futex
> |#  if defined (__NR_futex_time64) && (__TIMESIZE == 64)
> |#   define __NR_futex  __NR_futex_time64
> |#  endif
> |# endif
> |#endif

This is broken on musl (which does not define __GLIBC__), on kernels
running with CONFIG_COMPAT_32BIT_TIME disabled (which may
define __NR_futex, but always returns -ENOSYS), and on any future glibc
with 64-bit time_t on anything other all existing architectures (which
define __NR_futex but expect an incompatible argument). It may also
be broken on rv32 if we end up adding support for futex() in the future,
as has been discussed as a possible workaround.

> > rv32 is in the unfortunate position of being the first one that has a glibc
> > port, so it hits a lot of problems that other architectures do not.
> > If the applications get fixed properly, then at least they will also
> > work with musl-1.2 and a (will work-in-progress) glibc that will
> > eventually allow building with 64-bit time_t.
>
> Ok, so in case of perf we probably should limit the needed patch to
> riscv 32 bit architecture, unless we want to solve the problem for all
> 32 bit architecture with time_t 64 bit support.

No, whatever you do should be written to work on all 32-bit
architectures. It would be crazy to add a special case for one
architecture when it's easier to fix it properly.

What was wrong with the helper function I suggested?

       Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Wed, Jan 20, 2021 at 3:44 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> On Tue, Jan 19, 2021 at 08:47:59PM +0100, Arnd Bergmann wrote:
> > On Tue, Jan 19, 2021 at 5:53 PM Emiliano Ingrassia
> >
> > I didn't think that COMPAT_32BIT_TIME was even allowed on rv32,
> > but it seems there is a bug in Kconfig, it should be disabled for any
> > architecture that does not set __ARCH_WANT_TIME32_SYSCALLS,
> > but this should not affect the problem at hand.
>
> Ok, will you submit a patch for this bug?

Sure, will do.
>
> Here is the example futex app corrected w.r.t. all the previous
> considerations:
>
> |#include <features.h>
> |#if (defined __GLIBC__) && (__GLIBC__ >= 2 && __GLIBC_MINOR__ >= 32)
> |#include <asm/unistd.h>
> |#include <bits/timesize.h>
> |# ifndef __NR_futex
> |#  if defined (__NR_futex_time64) && (__TIMESIZE == 64)
> |#   define __NR_futex  __NR_futex_time64
> |#  endif
> |# endif
> |#endif

This is broken on musl (which does not define __GLIBC__), on kernels
running with CONFIG_COMPAT_32BIT_TIME disabled (which may
define __NR_futex, but always returns -ENOSYS), and on any future glibc
with 64-bit time_t on anything other all existing architectures (which
define __NR_futex but expect an incompatible argument). It may also
be broken on rv32 if we end up adding support for futex() in the future,
as has been discussed as a possible workaround.

> > rv32 is in the unfortunate position of being the first one that has a glibc
> > port, so it hits a lot of problems that other architectures do not.
> > If the applications get fixed properly, then at least they will also
> > work with musl-1.2 and a (will work-in-progress) glibc that will
> > eventually allow building with 64-bit time_t.
>
> Ok, so in case of perf we probably should limit the needed patch to
> riscv 32 bit architecture, unless we want to solve the problem for all
> 32 bit architecture with time_t 64 bit support.

No, whatever you do should be written to work on all 32-bit
architectures. It would be crazy to add a special case for one
architecture when it's easier to fix it properly.

What was wrong with the helper function I suggested?

       Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Wed, Jan 20, 2021 at 04:29:02PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 20, 2021 at 3:44 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > On Tue, Jan 19, 2021 at 08:47:59PM +0100, Arnd Bergmann wrote:
> > > On Tue, Jan 19, 2021 at 5:53 PM Emiliano Ingrassia
> > >
> > > I didn't think that COMPAT_32BIT_TIME was even allowed on rv32,
> > > but it seems there is a bug in Kconfig, it should be disabled for any
> > > architecture that does not set __ARCH_WANT_TIME32_SYSCALLS,
> > > but this should not affect the problem at hand.
> >
> > Ok, will you submit a patch for this bug?
>
> Sure, will do.
> >
> > Here is the example futex app corrected w.r.t. all the previous
> > considerations:
> >
> > |#include <features.h>
> > |#if (defined __GLIBC__) && (__GLIBC__ >= 2 && __GLIBC_MINOR__ >= 32)
> > |#include <asm/unistd.h>
> > |#include <bits/timesize.h>
> > |# ifndef __NR_futex
> > |#  if defined (__NR_futex_time64) && (__TIMESIZE == 64)
> > |#   define __NR_futex  __NR_futex_time64
> > |#  endif
> > |# endif
> > |#endif
>
> This is broken on musl (which does not define __GLIBC__), on kernels
> running with CONFIG_COMPAT_32BIT_TIME disabled (which may
> define __NR_futex, but always returns -ENOSYS), and on any future glibc
> with 64-bit time_t on anything other all existing architectures (which
> define __NR_futex but expect an incompatible argument). It may also
> be broken on rv32 if we end up adding support for futex() in the future,
> as has been discussed as a possible workaround.

I agree on this. It was just an example on howto get __NR_futex defined
in this specific case. Sure is broken in general.

>
> > > rv32 is in the unfortunate position of being the first one that has a glibc
> > > port, so it hits a lot of problems that other architectures do not.
> > > If the applications get fixed properly, then at least they will also
> > > work with musl-1.2 and a (will work-in-progress) glibc that will
> > > eventually allow building with 64-bit time_t.
> >
> > Ok, so in case of perf we probably should limit the needed patch to
> > riscv 32 bit architecture, unless we want to solve the problem for all
> > 32 bit architecture with time_t 64 bit support.
>
> No, whatever you do should be written to work on all 32-bit
> architectures. It would be crazy to add a special case for one
> architecture when it's easier to fix it properly.
>
> What was wrong with the helper function I suggested?

Sorry, I was just trying to have a more clear picture of the problem.

I'm totally fine with the helper function you proposed. About that,
should such approach be used for every time64 syscall for which is known
that there is no libc wrapper?

>
>        Arnd

Thank you, best regards.

Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Wed, Jan 20, 2021 at 04:29:02PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 20, 2021 at 3:44 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > On Tue, Jan 19, 2021 at 08:47:59PM +0100, Arnd Bergmann wrote:
> > > On Tue, Jan 19, 2021 at 5:53 PM Emiliano Ingrassia
> > >
> > > I didn't think that COMPAT_32BIT_TIME was even allowed on rv32,
> > > but it seems there is a bug in Kconfig, it should be disabled for any
> > > architecture that does not set __ARCH_WANT_TIME32_SYSCALLS,
> > > but this should not affect the problem at hand.
> >
> > Ok, will you submit a patch for this bug?
>
> Sure, will do.
> >
> > Here is the example futex app corrected w.r.t. all the previous
> > considerations:
> >
> > |#include <features.h>
> > |#if (defined __GLIBC__) && (__GLIBC__ >= 2 && __GLIBC_MINOR__ >= 32)
> > |#include <asm/unistd.h>
> > |#include <bits/timesize.h>
> > |# ifndef __NR_futex
> > |#  if defined (__NR_futex_time64) && (__TIMESIZE == 64)
> > |#   define __NR_futex  __NR_futex_time64
> > |#  endif
> > |# endif
> > |#endif
>
> This is broken on musl (which does not define __GLIBC__), on kernels
> running with CONFIG_COMPAT_32BIT_TIME disabled (which may
> define __NR_futex, but always returns -ENOSYS), and on any future glibc
> with 64-bit time_t on anything other all existing architectures (which
> define __NR_futex but expect an incompatible argument). It may also
> be broken on rv32 if we end up adding support for futex() in the future,
> as has been discussed as a possible workaround.

I agree on this. It was just an example on howto get __NR_futex defined
in this specific case. Sure is broken in general.

>
> > > rv32 is in the unfortunate position of being the first one that has a glibc
> > > port, so it hits a lot of problems that other architectures do not.
> > > If the applications get fixed properly, then at least they will also
> > > work with musl-1.2 and a (will work-in-progress) glibc that will
> > > eventually allow building with 64-bit time_t.
> >
> > Ok, so in case of perf we probably should limit the needed patch to
> > riscv 32 bit architecture, unless we want to solve the problem for all
> > 32 bit architecture with time_t 64 bit support.
>
> No, whatever you do should be written to work on all 32-bit
> architectures. It would be crazy to add a special case for one
> architecture when it's easier to fix it properly.
>
> What was wrong with the helper function I suggested?

Sorry, I was just trying to have a more clear picture of the problem.

I'm totally fine with the helper function you proposed. About that,
should such approach be used for every time64 syscall for which is known
that there is no libc wrapper?

>
>        Arnd

Thank you, best regards.

Emiliano

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Wed, Jan 20, 2021 at 4:45 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> > > > rv32 is in the unfortunate position of being the first one that has a glibc
> > > > port, so it hits a lot of problems that other architectures do not.
> > > > If the applications get fixed properly, then at least they will also
> > > > work with musl-1.2 and a (will work-in-progress) glibc that will
> > > > eventually allow building with 64-bit time_t.
> > >
> > > Ok, so in case of perf we probably should limit the needed patch to
> > > riscv 32 bit architecture, unless we want to solve the problem for all
> > > 32 bit architecture with time_t 64 bit support.
> >
> > No, whatever you do should be written to work on all 32-bit
> > architectures. It would be crazy to add a special case for one
> > architecture when it's easier to fix it properly.
> >
> > What was wrong with the helper function I suggested?
>
> Sorry, I was just trying to have a more clear picture of the problem.
>
> I'm totally fine with the helper function you proposed. About that,
> should such approach be used for every time64 syscall for which is known
> that there is no libc wrapper?

We have previously discussed adding generated wrappers for every
syscall, but my suggestion to have an inline function in one of the kernel
headers was not very popular at the time.

I think futex() is very much a special case here because this is the
only[1] thing that gets called directly by a number of applications through
syscall(). All the other time32 system calls have wrappers that are
provided by any C library, which will just use the correct syscall number
to match the arguments, or do a conversion if needed.

Most of the packages that use the other __NR_* macros need them
for something other than calling them (e.g. strace), so a wrapper does
not help.

      Arnd

[1] technically, there is also io_{p,}getevents, but that is usually abstracted
through libaio, and only that needs to be fixed. I checked
https://pagure.io/libaio/commits/master, which seems to be the upstream
but I do not see a fix in there, so this version is currently broken on 32-bit
architectures with musl-1.2 as well as on rv32 with glibc. Distros may
have already fixed this in their downstream copies.

        Arnd

Re: perf tools build broken for RISCV 32 bit

[Arnd Bergmann]

On Wed, Jan 20, 2021 at 4:45 PM Emiliano Ingrassia
<ingrassia@epigenesys.com> wrote:
> > > > rv32 is in the unfortunate position of being the first one that has a glibc
> > > > port, so it hits a lot of problems that other architectures do not.
> > > > If the applications get fixed properly, then at least they will also
> > > > work with musl-1.2 and a (will work-in-progress) glibc that will
> > > > eventually allow building with 64-bit time_t.
> > >
> > > Ok, so in case of perf we probably should limit the needed patch to
> > > riscv 32 bit architecture, unless we want to solve the problem for all
> > > 32 bit architecture with time_t 64 bit support.
> >
> > No, whatever you do should be written to work on all 32-bit
> > architectures. It would be crazy to add a special case for one
> > architecture when it's easier to fix it properly.
> >
> > What was wrong with the helper function I suggested?
>
> Sorry, I was just trying to have a more clear picture of the problem.
>
> I'm totally fine with the helper function you proposed. About that,
> should such approach be used for every time64 syscall for which is known
> that there is no libc wrapper?

We have previously discussed adding generated wrappers for every
syscall, but my suggestion to have an inline function in one of the kernel
headers was not very popular at the time.

I think futex() is very much a special case here because this is the
only[1] thing that gets called directly by a number of applications through
syscall(). All the other time32 system calls have wrappers that are
provided by any C library, which will just use the correct syscall number
to match the arguments, or do a conversion if needed.

Most of the packages that use the other __NR_* macros need them
for something other than calling them (e.g. strace), so a wrapper does
not help.

      Arnd

[1] technically, there is also io_{p,}getevents, but that is usually abstracted
through libaio, and only that needs to be fixed. I checked
https://pagure.io/libaio/commits/master, which seems to be the upstream
but I do not see a fix in there, so this version is currently broken on 32-bit
architectures with musl-1.2 as well as on rv32 with glibc. Distros may
have already fixed this in their downstream copies.

        Arnd

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Wed, Jan 20, 2021 at 05:03:30PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 20, 2021 at 4:45 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > > > > rv32 is in the unfortunate position of being the first one that has a glibc
> > > > > port, so it hits a lot of problems that other architectures do not.
> > > > > If the applications get fixed properly, then at least they will also
> > > > > work with musl-1.2 and a (will work-in-progress) glibc that will
> > > > > eventually allow building with 64-bit time_t.
> > > >
> > > > Ok, so in case of perf we probably should limit the needed patch to
> > > > riscv 32 bit architecture, unless we want to solve the problem for all
> > > > 32 bit architecture with time_t 64 bit support.
> > >
> > > No, whatever you do should be written to work on all 32-bit
> > > architectures. It would be crazy to add a special case for one
> > > architecture when it's easier to fix it properly.
> > >
> > > What was wrong with the helper function I suggested?
> >
> > Sorry, I was just trying to have a more clear picture of the problem.
> >
> > I'm totally fine with the helper function you proposed. About that,
> > should such approach be used for every time64 syscall for which is known
> > that there is no libc wrapper?
>
> We have previously discussed adding generated wrappers for every
> syscall, but my suggestion to have an inline function in one of the kernel
> headers was not very popular at the time.
>
> I think futex() is very much a special case here because this is the
> only[1] thing that gets called directly by a number of applications through
> syscall(). All the other time32 system calls have wrappers that are
> provided by any C library, which will just use the correct syscall number
> to match the arguments, or do a conversion if needed.
>
> Most of the packages that use the other __NR_* macros need them
> for something other than calling them (e.g. strace), so a wrapper does
> not help.
>
>       Arnd
>
> [1] technically, there is also io_{p,}getevents, but that is usually abstracted
> through libaio, and only that needs to be fixed. I checked
> https://pagure.io/libaio/commits/master, which seems to be the upstream
> but I do not see a fix in there, so this version is currently broken on 32-bit
> architectures with musl-1.2 as well as on rv32 with glibc. Distros may
> have already fixed this in their downstream copies.
>
>         Arnd

That's OK with me.

Please, when you'll propose the patch cc me so I can test and give you my review.
About this, will you also patch perf to use __kernel_futex()?

Thank you, best regards.

Emiliano

Re: perf tools build broken for RISCV 32 bit

[Emiliano Ingrassia]

Hi Arnd,

On Wed, Jan 20, 2021 at 05:03:30PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 20, 2021 at 4:45 PM Emiliano Ingrassia
> <ingrassia@epigenesys.com> wrote:
> > > > > rv32 is in the unfortunate position of being the first one that has a glibc
> > > > > port, so it hits a lot of problems that other architectures do not.
> > > > > If the applications get fixed properly, then at least they will also
> > > > > work with musl-1.2 and a (will work-in-progress) glibc that will
> > > > > eventually allow building with 64-bit time_t.
> > > >
> > > > Ok, so in case of perf we probably should limit the needed patch to
> > > > riscv 32 bit architecture, unless we want to solve the problem for all
> > > > 32 bit architecture with time_t 64 bit support.
> > >
> > > No, whatever you do should be written to work on all 32-bit
> > > architectures. It would be crazy to add a special case for one
> > > architecture when it's easier to fix it properly.
> > >
> > > What was wrong with the helper function I suggested?
> >
> > Sorry, I was just trying to have a more clear picture of the problem.
> >
> > I'm totally fine with the helper function you proposed. About that,
> > should such approach be used for every time64 syscall for which is known
> > that there is no libc wrapper?
>
> We have previously discussed adding generated wrappers for every
> syscall, but my suggestion to have an inline function in one of the kernel
> headers was not very popular at the time.
>
> I think futex() is very much a special case here because this is the
> only[1] thing that gets called directly by a number of applications through
> syscall(). All the other time32 system calls have wrappers that are
> provided by any C library, which will just use the correct syscall number
> to match the arguments, or do a conversion if needed.
>
> Most of the packages that use the other __NR_* macros need them
> for something other than calling them (e.g. strace), so a wrapper does
> not help.
>
>       Arnd
>
> [1] technically, there is also io_{p,}getevents, but that is usually abstracted
> through libaio, and only that needs to be fixed. I checked
> https://pagure.io/libaio/commits/master, which seems to be the upstream
> but I do not see a fix in there, so this version is currently broken on 32-bit
> architectures with musl-1.2 as well as on rv32 with glibc. Distros may
> have already fixed this in their downstream copies.
>
>         Arnd

That's OK with me.

Please, when you'll propose the patch cc me so I can test and give you my review.
About this, will you also patch perf to use __kernel_futex()?

Thank you, best regards.

Emiliano

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv