diff mbox series

[1/2] selftests/nolibc: add new gettimeofday test cases

Message ID bfc3dba52300dcce03ae1c7c41f2bb8984cf459b.1685428087.git.falcon@tinylab.org (mailing list archive)
State New
Headers show
Series nolibc: add part3 of support for rv32 | expand

Commit Message

Zhangjin Wu May 30, 2023, 6:37 a.m. UTC
These 3 test cases are added to cover the normal using scenes of
gettimeofday().

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

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

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

    tv->tv_usec = ts.tv_nsec / 1000;

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Thomas Weißschuh May 30, 2023, 10:59 a.m. UTC | #1
On 2023-05-30 14:37:49+0800, Zhangjin Wu wrote:
> These 3 test cases are added to cover the normal using scenes of
> gettimeofday().
> 
> They have been used to trigger and fix up such issue:
> 
>     nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'
> 
> This issue happens while there is no "unsigned int" conversion in the
> new clock_gettime / clock_gettime64 syscall path of gettimeofday():
> 
>     tv->tv_usec = ts.tv_nsec / 1000;
> 
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/testing/selftests/nolibc/nolibc-test.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 8ba8c2fc71a0..20d184da9a2b 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,9 @@ 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_tz);   EXPECT_SYSZR(1, gettimeofday(NULL, &tz)); break;

Calling gettimeofday(NULL, ...) will actually segfault on glibc.
It works when calling through the VDSO, but not the logic in glibc
itself, which is guess is allowed by POSIX.

I propose to avoid doing it :-)

Either we gate the existing test in #ifdef NOLIBC or we remove it.

> +		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
>
Zhangjin Wu May 30, 2023, 11:28 a.m. UTC | #2
> On 2023-05-30 14:37:49+0800, Zhangjin Wu wrote:
> > These 3 test cases are added to cover the normal using scenes of
> > gettimeofday().
> > 
> > They have been used to trigger and fix up such issue:
> > 
> >     nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'
> > 
> > This issue happens while there is no "unsigned int" conversion in the
> > new clock_gettime / clock_gettime64 syscall path of gettimeofday():
> > 
> >     tv->tv_usec = ts.tv_nsec / 1000;
> > 
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/testing/selftests/nolibc/nolibc-test.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index 8ba8c2fc71a0..20d184da9a2b 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,9 @@ 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_tz);   EXPECT_SYSZR(1, gettimeofday(NULL, &tz)); break;
> 
> Calling gettimeofday(NULL, ...) will actually segfault on glibc.
> It works when calling through the VDSO, but not the logic in glibc
> itself, which is guess is allowed by POSIX.
>

In low level gettimeofday syscall, it should be ok, will check the non-vdso
code of glibc later. in musl, it returns 0 when it is NULL
(src/time/gettimeofday.c).

> I propose to avoid doing it :-)

Just checked it again, these two cases can also detect the issues (1):

    CASE_TEST(gettimeofday_tv);   EXPECT_SYSZR(1, gettimeofday(&tv, NULL)); break;
    CASE_TEST(gettimeofday_tv_tz);EXPECT_SYSZR(1, gettimeofday(&tv, &tz)); break;

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

we can directly remove the second one ;-)

> 
> Either we gate the existing test in #ifdef NOLIBC or we remove it.

so, we still need to check the original gettimeofday_null test case for glibc?

Thanks,
Zhangjin

> 
> > +		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;
Thomas Weißschuh May 30, 2023, 11:54 a.m. UTC | #3
On 2023-05-30 19:28:06+0800, Zhangjin Wu wrote:
> > On 2023-05-30 14:37:49+0800, Zhangjin Wu wrote:
> > > These 3 test cases are added to cover the normal using scenes of
> > > gettimeofday().
> > > 
> > > They have been used to trigger and fix up such issue:
> > > 
> > >     nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'
> > > 
> > > This issue happens while there is no "unsigned int" conversion in the
> > > new clock_gettime / clock_gettime64 syscall path of gettimeofday():
> > > 
> > >     tv->tv_usec = ts.tv_nsec / 1000;
> > > 
> > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > ---
> > >  tools/testing/selftests/nolibc/nolibc-test.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > index 8ba8c2fc71a0..20d184da9a2b 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,9 @@ 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_tz);   EXPECT_SYSZR(1, gettimeofday(NULL, &tz)); break;
> > 
> > Calling gettimeofday(NULL, ...) will actually segfault on glibc.
> > It works when calling through the VDSO, but not the logic in glibc
> > itself, which is guess is allowed by POSIX.
> >
> 
> In low level gettimeofday syscall, it should be ok, will check the non-vdso
> code of glibc later. in musl, it returns 0 when it is NULL
> (src/time/gettimeofday.c).

It's fine for the Linux syscall, vdso, musl and nolibc.
It's not fine for glibc and gnulib.

> > I propose to avoid doing it :-)
> 
> Just checked it again, these two cases can also detect the issues (1):
> 
>     CASE_TEST(gettimeofday_tv);   EXPECT_SYSZR(1, gettimeofday(&tv, NULL)); break;
>     CASE_TEST(gettimeofday_tv_tz);EXPECT_SYSZR(1, gettimeofday(&tv, &tz)); break;
> 
>     (1) nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'

To be honest this does not look like a nolibc issue, more a compiler
setup problem. GCC generates calls to a function in libgcc that is not
linked for some reason.

Can you provide reproduction steps?

Also I guess a testcase that tests at runtime that the compilation has
worked is of limited use :-)
It either works or it never executes.

> we can directly remove the second one ;-)
> 
> > 
> > Either we gate the existing test in #ifdef NOLIBC or we remove it.
> 
> so, we still need to check the original gettimeofday_null test case for glibc?

The original testcase also needs adaption, yes.

> Thanks,
> Zhangjin
> 
> > 
> > > +		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;
Willy Tarreau May 30, 2023, 12:05 p.m. UTC | #4
On Tue, May 30, 2023 at 12:59:31PM +0200, Thomas Weißschuh wrote:
> On 2023-05-30 14:37:49+0800, Zhangjin Wu wrote:
> > These 3 test cases are added to cover the normal using scenes of
> > gettimeofday().
> > 
> > They have been used to trigger and fix up such issue:
> > 
> >     nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'
> > 
> > This issue happens while there is no "unsigned int" conversion in the
> > new clock_gettime / clock_gettime64 syscall path of gettimeofday():
> > 
> >     tv->tv_usec = ts.tv_nsec / 1000;
> > 
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/testing/selftests/nolibc/nolibc-test.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index 8ba8c2fc71a0..20d184da9a2b 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,9 @@ 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_tz);   EXPECT_SYSZR(1, gettimeofday(NULL, &tz)); break;
> 
> Calling gettimeofday(NULL, ...) will actually segfault on glibc.
> It works when calling through the VDSO, but not the logic in glibc
> itself, which is guess is allowed by POSIX.

Then that's shocking, because the man page says:

       If either tv or tz is NULL, the corresponding structure is not  set  or
       returned.   (However, compilation warnings will result if tv is NULL.)

I'd expect glibc to at least support what'd documented in the man
page :-/

> I propose to avoid doing it :-)

If you're certain that's the case, then I agree.

> Either we gate the existing test in #ifdef NOLIBC or we remove it.

Better not keep tests specific to nolibc if they aim at verifying some
compatibliity.

Willy
Andreas Schwab May 30, 2023, 12:31 p.m. UTC | #5
On Mai 30 2023, Willy Tarreau wrote:

> On Tue, May 30, 2023 at 12:59:31PM +0200, Thomas Weißschuh wrote:
>> On 2023-05-30 14:37:49+0800, Zhangjin Wu wrote:
>> > These 3 test cases are added to cover the normal using scenes of
>> > gettimeofday().
>> > 
>> > They have been used to trigger and fix up such issue:
>> > 
>> >     nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'
>> > 
>> > This issue happens while there is no "unsigned int" conversion in the
>> > new clock_gettime / clock_gettime64 syscall path of gettimeofday():
>> > 
>> >     tv->tv_usec = ts.tv_nsec / 1000;
>> > 
>> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
>> > ---
>> >  tools/testing/selftests/nolibc/nolibc-test.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> > 
>> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
>> > index 8ba8c2fc71a0..20d184da9a2b 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,9 @@ 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_tz);   EXPECT_SYSZR(1, gettimeofday(NULL, &tz)); break;
>> 
>> Calling gettimeofday(NULL, ...) will actually segfault on glibc.
>> It works when calling through the VDSO, but not the logic in glibc
>> itself, which is guess is allowed by POSIX.
>
> Then that's shocking, because the man page says:
>
>        If either tv or tz is NULL, the corresponding structure is not  set  or
>        returned.   (However, compilation warnings will result if tv is NULL.)
>
> I'd expect glibc to at least support what'd documented in the man
> page :-/

The manual page is not part of glibc.  Neither the glibc documentation
nor the POSIX spec has ever supported NULL for the first argument.  The
generic POSIX implementation in glibc originally returned EINVAL for
that case, though.
Thomas Weißschuh May 30, 2023, 12:35 p.m. UTC | #6
On 2023-05-30 14:05:00+0200, Willy Tarreau wrote:
> On Tue, May 30, 2023 at 12:59:31PM +0200, Thomas Weißschuh wrote:
> > On 2023-05-30 14:37:49+0800, Zhangjin Wu wrote:
> > > These 3 test cases are added to cover the normal using scenes of
> > > gettimeofday().
> > > 
> > > They have been used to trigger and fix up such issue:
> > > 
> > >     nolibc-test.c:(.text.gettimeofday+0x54): undefined reference to `__aeabi_ldivmod'
> > > 
> > > This issue happens while there is no "unsigned int" conversion in the
> > > new clock_gettime / clock_gettime64 syscall path of gettimeofday():
> > > 
> > >     tv->tv_usec = ts.tv_nsec / 1000;
> > > 
> > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > ---
> > >  tools/testing/selftests/nolibc/nolibc-test.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > index 8ba8c2fc71a0..20d184da9a2b 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,9 @@ 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_tz);   EXPECT_SYSZR(1, gettimeofday(NULL, &tz)); break;
> > 
> > Calling gettimeofday(NULL, ...) will actually segfault on glibc.
> > It works when calling through the VDSO, but not the logic in glibc
> > itself, which is guess is allowed by POSIX.
> 
> Then that's shocking, because the man page says:
> 
>        If either tv or tz is NULL, the corresponding structure is not  set  or
>        returned.   (However, compilation warnings will result if tv is NULL.)
> 
> I'd expect glibc to at least support what'd documented in the man
> page :-/

That is gettimeofday(2), which comes from the Linux man-pages project.

gettimeofday(3p) which specifies the posix function does not have that wording.
It also specifies that there is no way to indicate errors...

Seems to be a weird situation.

> > I propose to avoid doing it :-)
> 
> If you're certain that's the case, then I agree.
> 
> > Either we gate the existing test in #ifdef NOLIBC or we remove it.
> 
> Better not keep tests specific to nolibc if they aim at verifying some
> compatibliity.

Thomas
diff mbox series

Patch

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 8ba8c2fc71a0..20d184da9a2b 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,9 @@  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_tz);   EXPECT_SYSZR(1, gettimeofday(NULL, &tz)); 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;