From: Stafford Horne via Libc-alpha <libc-alpha@sourceware.org>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: GLIBC patches <libc-alpha@sourceware.org>
Subject: Re: [PATCH] linux: Define STAT64_IS_KERNEL_STAT64 by default
Date: Thu, 25 Nov 2021 06:29:15 +0900 [thread overview]
Message-ID: <YZ6uq0APvPHZT1LL@antec> (raw)
In-Reply-To: <b790b9fc-32ac-c7fe-50ac-3006e7076240@linaro.org>
On Wed, Nov 24, 2021 at 12:50:40PM -0300, Adhemerval Zanella wrote:
>
>
> On 23/11/2021 18:39, Stafford Horne wrote:
> > On Mon, Nov 22, 2021 at 10:00:31AM -0300, Adhemerval Zanella wrote:
> >>
> >>
> >> On 20/11/2021 18:09, Stafford Horne via Libc-alpha wrote:
> >>> In commit 36260d5035 ("linux: Set default kernel_stat.h to LFS") the
> >>> default for STAT64_IS_KERNEL_STAT64 was removed. This patch adds it
> >>> back.
> >>>
> >>> For architectures that want to used the default kernel_stat.h and do not
> >>> have __NR_newfstatat, STAT64_IS_KERNEL_STAT64 needs to be defined. Set
> >>> the default as 1 as modern port's stat64 struct should match the kernel
> >>> stat64 layout.
> >>>
> >>> I tested this on the OpenRISC port and it seems to work fine. Currently,
> >>> all archs that use the default kernel_stat.h define __NR_newfstatat so
> >>> they will not use the STAT64_IS_KERNEL_STAT64 macro. However, arc seems
> >>> to be an outlier it uses the default kernel_stat.h, but does not define
> >>> __NR_newfstatat or __NR_fstatat64 I am not clear how arc works here.
> >>
> >> arc and usually newer 32-bit ports will only use __NR_statx:
> >>
> >> 138 #if (__WORDSIZE == 32 \
> >> 139 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \
> >> 140 || defined STAT_HAS_TIME32
> >> 141 # define FSTATAT_USE_STATX 1
> >> 142 #else
> >> 143 # define FSTATAT_USE_STATX 0
> >> 144 #endif
> >>
> >> The patch looks ok, but it seems strange that ork1 requires it since it
> >> setting minimum required kernel to 5.10. I would expect that __ASSUME_STATX
> >> would be defined and only fstatat64_time64_statx would be used.
> >
> > Right,
> >
> > In that case maybe another ifdef is needed in fstatat64.c? I don't see
> > fstatat64_time64_stat is actually getting compiled into the libc.so binary. But
> > if I don't define STAT64_IS_KERNEL_STAT64 I get the below compile error.
> >
> > I added a pragma to output the value of FSTATAT_USE_STATX and __ASSUME_STATX in
> > the compile unit and I get the below which looks right:
> >
> > * __ASSUME_STATX: 1
> > * FSTATAT_USE_STATX: 1
> >
> > Error:
> >
> > or1k-glibc-linux-gnu-gcc ../sysdeps/unix/sysv/linux/fstatat64.c -c -std=gnu11 -fgnu89-inline -g -O2 -Wall -Wwrite-strings -Wundef -Werror -fmerge-all-constants -frounding-math -fno-stack-protector -fno-common -Wstrict-prototypes -Wold-style-definition -fmath-errno -ftls-model=initial-exec -I../include -I/home/shorne/work/gnu-toolchain/build-many/build/glibcs/or1k-linux-gnu-soft/glibc/io -I/home/shorne/work/gnu-toolchain/build-many/build/glibcs/or1k-linux-gnu-soft/glibc -I../sysdeps/unix/sysv/linux/or1k -I../sysdeps/or1k/nptl -I../sysdeps/unix/sysv/linux/generic/wordsize-32 -I../sysdeps/unix/sysv/linux/generic -I../sysdeps/unix/sysv/linux/include -I../sysdeps/unix/sysv/linux -I../sysdeps/nptl -I../sysdeps/pthread -I../sysdeps/gnu -I../sysdeps/unix/inet -I../sysdeps/unix/sysv -I../sysdeps/unix -I../sysdeps/posix -I../sysdeps/or1k/nofpu -I../sysdeps/ieee754/soft-fp -I../sysdeps/or1k -I../sysdeps/wordsize-32 -I../sysdeps/ieee754/dbl-64 -I../sysdeps/ieee754/flt-32 -I../sysdeps/ieee754 -I../sysdeps/generic -I.. -I../libio -I. -D_LIBC_REENTRANT -include /home/shorne/work/gnu-toolchain/build-many/build/glibcs/or1k-linux-gnu-soft/glibc/libc-modules.h -DMODULE_NAME=libc -include ../include/libc-symbols.h -DTOP_NAMESPACE=glibc -o /home/shorne/work/gnu-toolchain/build-many/build/glibcs/or1k-linux-gnu-soft/glibc/io/fstatat64.o -MD -MP -MF /home/shorne/work/gnu-toolchain/build-many/build/glibcs/or1k-linux-gnu-soft/glibc/io/fstatat64.o.dt -MT /home/shorne/work/gnu-toolchain/build-many/build/glibcs/or1k-linux-gnu-soft/glibc/io/fstatat64.o
> > ../sysdeps/unix/sysv/linux/fstatat64.c: In function ‘fstatat64_time64_stat’:
> > ../sysdeps/unix/sysv/linux/fstatat64.c:89:7: error: "STAT64_IS_KERNEL_STAT64" is not defined, evaluates to 0 [-Werror=undef]
> > 89 | # if STAT64_IS_KERNEL_STAT64
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > ../sysdeps/unix/sysv/linux/fstatat64.c:94:24: error: storage size of ‘kst64’ isn’t known
> > 94 | struct kernel_stat64 kst64;
> > | ^~~~~
> > ../sysdeps/unix/sysv/linux/fstatat64.c:97:5: error: implicit declaration of function ‘__cp_stat64_kstat64’ [-Werror=implicit-function-declaration]
> > 97 | __cp_stat64_kstat64 (buf, &kst64);
> > | ^~~~~~~~~~~~~~~~~~~
> > ../sysdeps/unix/sysv/linux/fstatat64.c:94:24: error: unused variable ‘kst64’ [-Werror=unused-variable]
> > 94 | struct kernel_stat64 kst64;
> > | ^~~~~
> > ../sysdeps/unix/sysv/linux/fstatat64.c: At top level:
> > ../sysdeps/unix/sysv/linux/fstatat64.c:149:9: note: ‘#pragma message: The value of FSTATAT_USE_STATX: 1’
> > 149 | #pragma message "The value of FSTATAT_USE_STATX: " XSTR(FSTATAT_USE_STATX)
> > | ^~~~~~~
> > ../sysdeps/unix/sysv/linux/fstatat64.c:150:9: note: ‘#pragma message: The value of __ASSUME_STATX: 1’
> > 150 | #pragma message "The value of __ASSUME_STATX: " XSTR(__ASSUME_STATX)
> > | ^~~~~~~
> >
> > Again I am not sure how arc avoids this error I shall try to compile it too.
>
> The problem is 'fstatat64_time64_stat' should not be build for 32-bit architectures
> with __ASSUME_STATX (since only statx provides 64-bit timestamps). Other architecture
> might get an improved codegen using older syscalls (specially 64-bit ones), so statx
> should be used only when really required.
>
> The patch below fixes it:
Right, this was what I was thinking too. However, I am still not sure why or1k
needs it and arc does not. I tested the arc build and confirmed with the same
version of GCC the arc build passes but or1k fails with the above error. So as
you said, maybe there is better codegen somewhere.
> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c
> index f968e4ef05..50ae5ad748 100644
> --- a/sysdeps/unix/sysv/linux/fstatat64.c
> +++ b/sysdeps/unix/sysv/linux/fstatat64.c
> @@ -74,6 +74,17 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
> return r;
> }
>
> +#if (__WORDSIZE == 32 \
> + && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \
> + || defined STAT_HAS_TIME32
> +# define FSTATAT_USE_STATX 1
> +#else
> +# define FSTATAT_USE_STATX 0
> +#endif
> +
> +/* Only statx supports 64-bit timestamps for 32-bit architectures with
> + __ASSUME_STATX, so there is no point in building the fallback. */
> +#if !FSTATAT_USE_STATX || (FSTATAT_USE_STATX && !defined __ASSUME_STATX)
> static inline int
> fstatat64_time64_stat (int fd, const char *file, struct __stat64_t64 *buf,
> int flag)
> @@ -134,13 +145,6 @@ fstatat64_time64_stat (int fd, const char *file, struct __stat64_t64 *buf,
>
> return r;
> }
> -
> -#if (__WORDSIZE == 32 \
> - && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) \
> - || defined STAT_HAS_TIME32
> -# define FSTATAT_USE_STATX 1
> -#else
> -# define FSTATAT_USE_STATX 0
> #endif
>
> int
I tested this patch and it works.
-Stafford
next prev parent reply other threads:[~2021-11-24 21:29 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-20 21:09 [PATCH] linux: Define STAT64_IS_KERNEL_STAT64 by default Stafford Horne via Libc-alpha
2021-11-22 13:00 ` Adhemerval Zanella via Libc-alpha
2021-11-23 21:39 ` Stafford Horne via Libc-alpha
2021-11-24 15:50 ` Adhemerval Zanella via Libc-alpha
2021-11-24 21:29 ` Stafford Horne via Libc-alpha [this message]
2021-11-25 12:53 ` Adhemerval Zanella via Libc-alpha
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/libc/involved.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YZ6uq0APvPHZT1LL@antec \
--to=libc-alpha@sourceware.org \
--cc=adhemerval.zanella@linaro.org \
--cc=shorne@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).