Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-441-ga3ab13cd6d-fm-20230517.001-ga3ab13cd
Mime-Version: 1.0
Message-Id: <9e4064fc-f0c5-4dd3-941f-344d2150e1cd@app.fastmail.com>
In-Reply-To: <20230528082509.293250-1-falcon@tinylab.org>
References: <83ab9f47-e1ed-463c-a717-26aad6bf2b71@app.fastmail.com>
 <20230528082509.293250-1-falcon@tinylab.org>
Date:   Sun, 28 May 2023 10:48:21 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Zhangjin Wu" <falcon@tinylab.org>,
        =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <thomas@t-8ch.de>,
        "Willy Tarreau" <w@1wt.eu>
Cc:     linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-riscv@lists.infradead.org,
        "Palmer Dabbelt" <palmer@dabbelt.com>,
        "Paul Walmsley" <paul.walmsley@sifive.com>
Subject: Re: [PATCH 09/13] tools/nolibc: sys_poll: riscv: use __NR_ppoll_time64 for
 rv32
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, May 28, 2023, at 10:25, Zhangjin Wu wrote:
>> On Fri, May 26, 2023, at 09:15, Thomas Wei=3DC3=3D9Fschuh wrote:

> * Use __NR_*time64 for all 32bit platforms
> * Use __NR_pselect6/ppoll/clock_gettime only for 64bit platforms
> * New sizeof tests added to verify off_t, time_t, timespec, itimerspec=
...
>
>    	CASE_TEST(sizeof_time_t);           EXPECT_EQ(1, 8,  =20
> sizeof(time_t)); break;
>     	CASE_TEST(sizeof_timespec);         EXPECT_EQ(1, 16, =20
> sizeof(struct timespec)); break;
>     #ifdef NOLIBC
>     	CASE_TEST(sizeof_itimerspec);       EXPECT_EQ(1, 32, =20
> sizeof(struct itimerspec)); break;
>     #endif
>     	CASE_TEST(sizeof_timeval);          EXPECT_EQ(1, 16, =20
> sizeof(struct timeval)); break;
>     	CASE_TEST(sizeof_itimerval);        EXPECT_EQ(1, 32, =20
> sizeof(struct itimerval)); break;
>     	CASE_TEST(sizeof_off_t);            EXPECT_EQ(1, 8,  =20
> sizeof(off_t)); break;
>
>
> @Arnd, the above timeval/itimerval definitions are used to override th=
e ones
> from linux/time.h to avoid such error:
>
>     error: redefinition of =E2=80=98struct timeval=E2=80=99
>
>     nolibc/sysroot/riscv/include/types.h:225:8: error: redefinition of=20
> =E2=80=98struct timeval=E2=80=99
>       225 | struct timeval {
>           |        ^~~~~~~
>     In file included from nolibc/sysroot/riscv/include/types.h:11,
>                      from nolibc/sysroot/riscv/include/nolibc.h:98,
>                      from nolibc/sysroot/riscv/include/errno.h:26,
>                      from nolibc/sysroot/riscv/include/stdio.h:14,
>                      from=20
> tools/testing/selftests/nolibc/nolibc-test.c:12:
>     nolibc/sysroot/riscv/include/linux/time.h:16:8: note: originally=20
> defined here
>        16 | struct timeval {
>
> @Arnd, As you commented in another reply, is it time for us to update
> include/uapi/linux/time.h together and let it provide time64 timeval/i=
timerval
> instead of the old ones? perhaps some libc's are still using them.

It's hard to know if anything is using it until we try. On the other
hand, we also know that anything still relying on it is going to be
increasingly broken on 32-bit architectures over as we get closer to
y2038, so we can just try removing these here to see what happens.

> Or perhaps we can add a switch like __ARCH_WANT_TIME32_SYSCALLS, add a
> __ARCH_WANT_TIME32_STRUCTS and simply bind it with __ARCH_WANT_TIME32_=
SYSCALLS?

I don't like that idea: __ARCH_WANT_TIME32_SYSCALLS tells us that
an architeture still provides those syscalls for compatibility, so
that is architecture specific, but __ARCH_WANT_TIME32_STRUCTS is not
something that is an architecture specific decision at all, it's
only needed for old source code.

> About the above ugly override code, What's your suggestion in v2? ;-)

Can you maybe just remove the "#include <linux/time.h>" from all
include/uapi/ and nolibc headers so it just never gets seen?

Unfortunately I see the header contains a few other definitions,
which might have to get moved out of the way, possibly to
linux/time_types.h.

       Arnd