[v3,11/12] selftests/nolibc: add new gettimeofday test cases

  These 2 test cases are added to cover the normal using scenes of
gettimeofday().

They have been used to trigger and fix up such issue with nolibc:

    nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'

This issue happens while there is no "unsigned int" conversion in the
coming new clock_gettime / clock_gettime64 syscall path of
gettimeofday():

    tv->tv_usec = ts.tv_nsec / 1000;

Suggested-by: Thomas Weißschuh <linux@weissschuh.net>
Link: https://lore.kernel.org/linux-riscv/280867a8-7601-4a96-9b85-87668e1f1282@t-8ch.de/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
 1 file changed, 4 insertions(+)
  

On 2023-06-03 16:16:07+0800, Zhangjin Wu wrote:
> These 2 test cases are added to cover the normal using scenes of
> gettimeofday().
> 
> They have been used to trigger and fix up such issue with nolibc:
> 
>     nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'
> 
> This issue happens while there is no "unsigned int" conversion in the
> coming new clock_gettime / clock_gettime64 syscall path of
> gettimeofday():
> 
>     tv->tv_usec = ts.tv_nsec / 1000;

As mentioned before this looks to me like an issue in the build setup.
Could you provide reproduction steps?

Nevertheless I guess the tests themselves are fine to have.

> Suggested-by: Thomas Weißschuh <linux@weissschuh.net>
> Link: https://lore.kernel.org/linux-riscv/280867a8-7601-4a96-9b85-87668e1f1282@t-8ch.de/
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index bf63fc66e486..e68c5692ec54 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -533,6 +533,8 @@ static int test_stat_timestamps(void)
>   */
>  int run_syscall(int min, int max)
>  {
> +	struct timeval tv;
> +	struct timezone tz;
>  	struct stat stat_buf;
>  	int euid0;
>  	int proc;
> @@ -588,6 +590,8 @@ int run_syscall(int min, int max)
>  		CASE_TEST(getdents64_root);   EXPECT_SYSNE(1, test_getdents64("/"), -1); break;
>  		CASE_TEST(getdents64_null);   EXPECT_SYSER(1, test_getdents64("/dev/null"), -1, ENOTDIR); break;
>  		CASE_TEST(gettimeofday_null); EXPECT_SYSZR(1, gettimeofday(NULL, NULL)); break;
> +		CASE_TEST(gettimeofday_tv);   EXPECT_SYSZR(1, gettimeofday(&tv, NULL)); break;
> +		CASE_TEST(gettimeofday_tv_tz);EXPECT_SYSZR(1, gettimeofday(&tv, &tz)); break;
>  		CASE_TEST(getpagesize);       EXPECT_SYSZR(1, test_getpagesize()); break;
>  		CASE_TEST(ioctl_tiocinq);     EXPECT_SYSZR(1, ioctl(0, TIOCINQ, &tmp)); break;
>  		CASE_TEST(ioctl_tiocinq);     EXPECT_SYSZR(1, ioctl(0, TIOCINQ, &tmp)); break;
> -- 
> 2.25.1
>
  

On Sun, Jun 4, 2023, at 10:29, 吴章金 wrote:
>
> Sorry for missing part of your feedbacks, I will check if -nostdlib 
> stops the linking of libgcc_s or my own separated test script forgot 
> linking the libgcc_s manually.

According to the gcc documentation, -nostdlib drops libgcc.a, but
adding -lgcc is the recommended way to bring it back.

> And as suggestion from Thomas' reply,
>
>>> Perhaps we really need to add the missing __divdi3 and __aeabi_ldivmod and the
>>> ones for the other architectures, or get one from lib/math/div64.c.
>
>>No, these ones come from the compiler via libgcc_s, we must not try to
> reimplement them. And we should do our best to avoid depending on them
> to avoid the error you got above.
>
> So, the explicit conversion is used instead in the patch.

I think a cast to a 32-bit type is ideal when converting the
clock_gettime() result into microseconds, since the kernel guarantees
that the timespec value is normalized, with all zeroes in the
upper 34 bits. Going through __aeabi_ldivmod would make the
conversion much slower.

For user supplied non-normalized timeval values, it's not obvious
whether we need the full 64-bit division

     Arnd
  

On Sun, Jun 04, 2023 at 11:24:39AM +0200, Arnd Bergmann wrote:
> On Sun, Jun 4, 2023, at 10:29, ??? wrote:
> >
> > Sorry for missing part of your feedbacks, I will check if -nostdlib 
> > stops the linking of libgcc_s or my own separated test script forgot 
> > linking the libgcc_s manually.
> 
> According to the gcc documentation, -nostdlib drops libgcc.a, but
> adding -lgcc is the recommended way to bring it back.
> 
> > And as suggestion from Thomas' reply,
> >
> >>> Perhaps we really need to add the missing __divdi3 and __aeabi_ldivmod and the
> >>> ones for the other architectures, or get one from lib/math/div64.c.
> >
> >>No, these ones come from the compiler via libgcc_s, we must not try to
> > reimplement them. And we should do our best to avoid depending on them
> > to avoid the error you got above.
> >
> > So, the explicit conversion is used instead in the patch.
> 
> I think a cast to a 32-bit type is ideal when converting the
> clock_gettime() result into microseconds, since the kernel guarantees
> that the timespec value is normalized, with all zeroes in the
> upper 34 bits. Going through __aeabi_ldivmod would make the
> conversion much slower.
> 
> For user supplied non-normalized timeval values, it's not obvious
> whether we need the full 64-bit division

We don't have to care about these here for the microsecond part,
because for decades these were exclusively 32-bit. Also the only
one consuming this field would have been settimeofday() and it's
already documented as returning EINVAL if tv_usec is not within
the expected 0..999999 range.

And when in doubt we should keep in mind that nolibc's purpose is not
to become a yet-another full-blown libc alternative but just a small
piece of software allowing to produce portable and compact binaries
for testing or booting. Being a bit stricter than other libcs for the
sake of code compactness is better here. Originally for example it was
necessary to always pass the 3 arguments to open(). Over time we managed
to make simple code compile with both glibc and nolibc, but when it
comes at the cost of adding size and burden for the developers, such
as forcing them to add libgcc, I prefer that we slightly limit the
domain of application instead.

Thanks!
Willy
  

On Sun, Jun 4, 2023, at 13:27, Willy Tarreau wrote:
> On Sun, Jun 04, 2023 at 11:24:39AM +0200, Arnd Bergmann wrote:
>> 
>> For user supplied non-normalized timeval values, it's not obvious
>> whether we need the full 64-bit division
>
> We don't have to care about these here for the microsecond part,
> because for decades these were exclusively 32-bit. Also the only
> one consuming this field would have been settimeofday() and it's
> already documented as returning EINVAL if tv_usec is not within
> the expected 0..999999 range.

Right

> Over time we managed
> to make simple code compile with both glibc and nolibc, but when it
> comes at the cost of adding size and burden for the developers, such
> as forcing them to add libgcc, I prefer that we slightly limit the
> domain of application instead.

Good point. This also reminds me that the compilers I build for
https://mirrors.edge.kernel.org/pub/tools/crosstool/ don't always
have every version of libgcc that may be needed, for instance
the mips compilers only provide a big-endian libgcc and the
arm compilers only provide a little-endian one, even though
the compilers can build code both ways with the right flags.

      Arnd
  

On Sun, Jun 04, 2023 at 01:38:39PM +0200, Arnd Bergmann wrote:
> > Over time we managed
> > to make simple code compile with both glibc and nolibc, but when it
> > comes at the cost of adding size and burden for the developers, such
> > as forcing them to add libgcc, I prefer that we slightly limit the
> > domain of application instead.
> 
> Good point. This also reminds me that the compilers I build for
> https://mirrors.edge.kernel.org/pub/tools/crosstool/ don't always
> have every version of libgcc that may be needed, for instance
> the mips compilers only provide a big-endian libgcc and the
> arm compilers only provide a little-endian one, even though
> the compilers can build code both ways with the right flags.

That reminds me something indeed, I know that MIPS is a great platform
for testing portability due to libgcc and/or atomics not always being
complete depending how it's built. At work when I double-check that
haproxy still builds and starts on my EdgeRouter-X, then it will build
everywhere ;-)

Willy
  

> On Sun, Jun 04, 2023 at 11:24:39AM +0200, Arnd Bergmann wrote:
> > On Sun, Jun 4, 2023, at 10:29, ??? wrote:
> > >
> > > Sorry for missing part of your feedbacks, I will check if -nostdlib 
> > > stops the linking of libgcc_s or my own separated test script forgot 
> > > linking the libgcc_s manually.
> > 
> > According to the gcc documentation, -nostdlib drops libgcc.a, but
> > adding -lgcc is the recommended way to bring it back.
> > 
> > > And as suggestion from Thomas' reply,
> > >
> > >>> Perhaps we really need to add the missing __divdi3 and __aeabi_ldivmod and the
> > >>> ones for the other architectures, or get one from lib/math/div64.c.
> > >
> > >>No, these ones come from the compiler via libgcc_s, we must not try to
> > > reimplement them. And we should do our best to avoid depending on them
> > > to avoid the error you got above.
> > >
> > > So, the explicit conversion is used instead in the patch.
> > 
> > I think a cast to a 32-bit type is ideal when converting the
> > clock_gettime() result into microseconds, since the kernel guarantees
> > that the timespec value is normalized, with all zeroes in the
> > upper 34 bits. Going through __aeabi_ldivmod would make the
> > conversion much slower.
> > 

Perfectly, this message is really required to be added to the coming
clock_gettime/time64 patches, I did worry about the (unsigned int)
conversion may lose the upper bits, thanks Arnd.

> > For user supplied non-normalized timeval values, it's not obvious
> > whether we need the full 64-bit division
> 
> We don't have to care about these here for the microsecond part,
> because for decades these were exclusively 32-bit. Also the only
> one consuming this field would have been settimeofday() and it's
> already documented as returning EINVAL if tv_usec is not within
> the expected 0..999999 range.
> 

And this one, thanks Willy.

> And when in doubt we should keep in mind that nolibc's purpose is not
> to become a yet-another full-blown libc alternative but just a small
> piece of software allowing to produce portable and compact binaries
> for testing or booting. Being a bit stricter than other libcs for the
> sake of code compactness is better here. Originally for example it was
> necessary to always pass the 3 arguments to open(). Over time we managed
> to make simple code compile with both glibc and nolibc, but when it
> comes at the cost of adding size and burden for the developers, such
> as forcing them to add libgcc, I prefer that we slightly limit the
> domain of application instead.

This explains why it is 'no' libc ;-)

Best regards,
Zhangjin

> 
> Thanks!
> Willy