[RFC PATCH-for-5.1?] tests/tcg/multiarch/linux-test: Skip test if nanosleep missing (Travis)

[RFC PATCH-for-5.1?] tests/tcg/multiarch/linux-test: Skip test if nanosleep missing (Travis)

[Philippe Mathieu-Daudé]

The time test sometimes fails on Travis-CI [*]:

    TEST    linux-test on aarch64
  tests/tcg/multiarch/linux-test.c:237: nanosleep
  make[2]: *** [run-linux-test] Error 1
  make: *** [run-tcg-tests-aarch64-linux-user] Error 2

As this seems due to a container limitation on Travis-CI,
simply skip the test there.

[*] https://travis-ci.org/github/qemu/qemu/jobs/710005078#L3706

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because per Laurent Vivier we are not using the correct libc
    while cross-linking the test (maybe change in the container
    packages?)
---
 tests/tcg/multiarch/linux-test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
index 8a7c15cd31..c7dfdec9ec 100644
--- a/tests/tcg/multiarch/linux-test.c
+++ b/tests/tcg/multiarch/linux-test.c
@@ -233,8 +233,13 @@ static void test_time(void)
     ts.tv_sec = 0;
     ts.tv_nsec = 20 * 1000000;
     chk_error(nanosleep(&ts, &rem));
-    if (rem.tv_sec != 1)
+    if (rem.tv_sec != 1) {
+        if (getenv("TRAVIS_ARCH")) {
+            printf("nanosleep missing? skipping 'time' test\n");
+            return;
+        }
         error("nanosleep");
+    }
     chk_error(gettimeofday(&tv2, NULL));
     ti = tv2.tv_sec - tv.tv_sec;
     if (ti >= 2)
-- 
2.21.3

Re: [RFC PATCH-for-5.1?] tests/tcg/multiarch/linux-test: Skip test if nanosleep missing (Travis)

[Laurent Vivier]

Le 21/07/2020 à 10:57, Philippe Mathieu-Daudé a écrit :
> The time test sometimes fails on Travis-CI [*]:
> 
>     TEST    linux-test on aarch64
>   tests/tcg/multiarch/linux-test.c:237: nanosleep
>   make[2]: *** [run-linux-test] Error 1
>   make: *** [run-tcg-tests-aarch64-linux-user] Error 2
> 
> As this seems due to a container limitation on Travis-CI,
> simply skip the test there.
> 
> [*] https://travis-ci.org/github/qemu/qemu/jobs/710005078#L3706
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because per Laurent Vivier we are not using the correct libc
>     while cross-linking the test (maybe change in the container
>     packages?)
> ---
>  tests/tcg/multiarch/linux-test.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
> index 8a7c15cd31..c7dfdec9ec 100644
> --- a/tests/tcg/multiarch/linux-test.c
> +++ b/tests/tcg/multiarch/linux-test.c
> @@ -233,8 +233,13 @@ static void test_time(void)
>      ts.tv_sec = 0;
>      ts.tv_nsec = 20 * 1000000;
>      chk_error(nanosleep(&ts, &rem));
> -    if (rem.tv_sec != 1)
> +    if (rem.tv_sec != 1) {
> +        if (getenv("TRAVIS_ARCH")) {
> +            printf("nanosleep missing? skipping 'time' test\n");
> +            return;
> +        }
>          error("nanosleep");
> +    }
>      chk_error(gettimeofday(&tv2, NULL));
>      ti = tv2.tv_sec - tv.tv_sec;
>      if (ti >= 2)
> 

Well, in the end I think the problem is in linux-user:

We copy the "rem" structure even if there is no error, so "1" is
overwritten.

We don't have the problem on all architectures because some use
nanosleep() syscall (that is correct) others use clock_nanosleep()
syscall that is not correct.

This should fix the problem:

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1211e759c26c..130005716ece 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11831,7 +11831,7 @@ static abi_long do_syscall1(void *cpu_env, int
num, abi_long arg1,
         target_to_host_timespec(&ts, arg3);
         ret = get_errno(safe_clock_nanosleep(arg1, arg2,
                                              &ts, arg4 ? &ts : NULL));
-        if (arg4)
+        if (is_error(ret) && arg4)
             host_to_target_timespec(arg4, &ts);

 #if defined(TARGET_PPC)

Thanks,
Laurent

Re: [RFC PATCH-for-5.1?] tests/tcg/multiarch/linux-test: Skip test if nanosleep missing (Travis)

[Laurent Vivier]

Le 21/07/2020 à 13:38, Laurent Vivier a écrit :
> Le 21/07/2020 à 10:57, Philippe Mathieu-Daudé a écrit :
>> The time test sometimes fails on Travis-CI [*]:
>>
>>     TEST    linux-test on aarch64
>>   tests/tcg/multiarch/linux-test.c:237: nanosleep
>>   make[2]: *** [run-linux-test] Error 1
>>   make: *** [run-tcg-tests-aarch64-linux-user] Error 2
>>
>> As this seems due to a container limitation on Travis-CI,
>> simply skip the test there.
>>
>> [*] https://travis-ci.org/github/qemu/qemu/jobs/710005078#L3706
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> RFC because per Laurent Vivier we are not using the correct libc
>>     while cross-linking the test (maybe change in the container
>>     packages?)
>> ---
>>  tests/tcg/multiarch/linux-test.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
>> index 8a7c15cd31..c7dfdec9ec 100644
>> --- a/tests/tcg/multiarch/linux-test.c
>> +++ b/tests/tcg/multiarch/linux-test.c
>> @@ -233,8 +233,13 @@ static void test_time(void)
>>      ts.tv_sec = 0;
>>      ts.tv_nsec = 20 * 1000000;
>>      chk_error(nanosleep(&ts, &rem));
>> -    if (rem.tv_sec != 1)
>> +    if (rem.tv_sec != 1) {
>> +        if (getenv("TRAVIS_ARCH")) {
>> +            printf("nanosleep missing? skipping 'time' test\n");
>> +            return;
>> +        }
>>          error("nanosleep");
>> +    }
>>      chk_error(gettimeofday(&tv2, NULL));
>>      ti = tv2.tv_sec - tv.tv_sec;
>>      if (ti >= 2)
>>
> 
> Well, in the end I think the problem is in linux-user:
> 
> We copy the "rem" structure even if there is no error, so "1" is
> overwritten.
> 
> We don't have the problem on all architectures because some use
> nanosleep() syscall (that is correct) others use clock_nanosleep()
> syscall that is not correct.
> 
> This should fix the problem:
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1211e759c26c..130005716ece 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11831,7 +11831,7 @@ static abi_long do_syscall1(void *cpu_env, int
> num, abi_long arg1,
>          target_to_host_timespec(&ts, arg3);
>          ret = get_errno(safe_clock_nanosleep(arg1, arg2,
>                                               &ts, arg4 ? &ts : NULL));
> -        if (arg4)
> +        if (is_error(ret) && arg4)
>              host_to_target_timespec(arg4, &ts);
> 
>  #if defined(TARGET_PPC)

According to clock_nanosleep(2) it should be in fact:

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1211e759c26c..63e7cd8947e5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11831,8 +11831,9 @@ static abi_long do_syscall1(void *cpu_env, int
num, abi_long arg1,
         target_to_host_timespec(&ts, arg3);
         ret = get_errno(safe_clock_nanosleep(arg1, arg2,
                                              &ts, arg4 ? &ts : NULL));
-        if (arg4)
+        if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME) {
             host_to_target_timespec(arg4, &ts);
+        }

 #if defined(TARGET_PPC)
         /* clock_nanosleep is odd in that it returns positive errno values.

Re: [RFC PATCH-for-5.1?] tests/tcg/multiarch/linux-test: Skip test if nanosleep missing (Travis)

[Philippe Mathieu-Daudé]

On 7/21/20 2:24 PM, Laurent Vivier wrote:
> Le 21/07/2020 à 13:38, Laurent Vivier a écrit :
>> Le 21/07/2020 à 10:57, Philippe Mathieu-Daudé a écrit :
>>> The time test sometimes fails on Travis-CI [*]:
>>>
>>>     TEST    linux-test on aarch64
>>>   tests/tcg/multiarch/linux-test.c:237: nanosleep
>>>   make[2]: *** [run-linux-test] Error 1
>>>   make: *** [run-tcg-tests-aarch64-linux-user] Error 2
>>>
>>> As this seems due to a container limitation on Travis-CI,
>>> simply skip the test there.
>>>
>>> [*] https://travis-ci.org/github/qemu/qemu/jobs/710005078#L3706
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> RFC because per Laurent Vivier we are not using the correct libc
>>>     while cross-linking the test (maybe change in the container
>>>     packages?)
>>> ---
>>>  tests/tcg/multiarch/linux-test.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
>>> index 8a7c15cd31..c7dfdec9ec 100644
>>> --- a/tests/tcg/multiarch/linux-test.c
>>> +++ b/tests/tcg/multiarch/linux-test.c
>>> @@ -233,8 +233,13 @@ static void test_time(void)
>>>      ts.tv_sec = 0;
>>>      ts.tv_nsec = 20 * 1000000;
>>>      chk_error(nanosleep(&ts, &rem));
>>> -    if (rem.tv_sec != 1)
>>> +    if (rem.tv_sec != 1) {
>>> +        if (getenv("TRAVIS_ARCH")) {
>>> +            printf("nanosleep missing? skipping 'time' test\n");
>>> +            return;
>>> +        }
>>>          error("nanosleep");
>>> +    }
>>>      chk_error(gettimeofday(&tv2, NULL));
>>>      ti = tv2.tv_sec - tv.tv_sec;
>>>      if (ti >= 2)
>>>
>>
>> Well, in the end I think the problem is in linux-user:
>>
>> We copy the "rem" structure even if there is no error, so "1" is
>> overwritten.
>>
>> We don't have the problem on all architectures because some use
>> nanosleep() syscall (that is correct) others use clock_nanosleep()
>> syscall that is not correct.
>>
>> This should fix the problem:
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 1211e759c26c..130005716ece 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -11831,7 +11831,7 @@ static abi_long do_syscall1(void *cpu_env, int
>> num, abi_long arg1,
>>          target_to_host_timespec(&ts, arg3);
>>          ret = get_errno(safe_clock_nanosleep(arg1, arg2,
>>                                               &ts, arg4 ? &ts : NULL));
>> -        if (arg4)
>> +        if (is_error(ret) && arg4)
>>              host_to_target_timespec(arg4, &ts);
>>
>>  #if defined(TARGET_PPC)
> 
> According to clock_nanosleep(2) it should be in fact:
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1211e759c26c..63e7cd8947e5 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11831,8 +11831,9 @@ static abi_long do_syscall1(void *cpu_env, int
> num, abi_long arg1,
>          target_to_host_timespec(&ts, arg3);
>          ret = get_errno(safe_clock_nanosleep(arg1, arg2,
>                                               &ts, arg4 ? &ts : NULL));
> -        if (arg4)
> +        if (ret == -TARGET_EINTR && arg4 && arg2 != TIMER_ABSTIME) {
>              host_to_target_timespec(arg4, &ts);
> +        }

I tested this hunk and couldn't reproduce it on Travis-CI. Since it is
intermittent, that doesn't mean much, but still if you send a proper
path you can add "Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>".

> 
>  #if defined(TARGET_PPC)
>          /* clock_nanosleep is odd in that it returns positive errno values.
> 
> 

Re: [RFC PATCH-for-5.1?] tests/tcg/multiarch/linux-test: Skip test if nanosleep missing (Travis)

[Alex Bennée]

Laurent Vivier <laurent@vivier.eu> writes:

> Le 21/07/2020 à 10:57, Philippe Mathieu-Daudé a écrit :
>> The time test sometimes fails on Travis-CI [*]:
>> 
>>     TEST    linux-test on aarch64
>>   tests/tcg/multiarch/linux-test.c:237: nanosleep
>>   make[2]: *** [run-linux-test] Error 1
>>   make: *** [run-tcg-tests-aarch64-linux-user] Error 2
>> 
>> As this seems due to a container limitation on Travis-CI,
>> simply skip the test there.
>> 
>> [*] https://travis-ci.org/github/qemu/qemu/jobs/710005078#L3706
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> RFC because per Laurent Vivier we are not using the correct libc
>>     while cross-linking the test (maybe change in the container
>>     packages?)
>> ---
>>  tests/tcg/multiarch/linux-test.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
>> index 8a7c15cd31..c7dfdec9ec 100644
>> --- a/tests/tcg/multiarch/linux-test.c
>> +++ b/tests/tcg/multiarch/linux-test.c
>> @@ -233,8 +233,13 @@ static void test_time(void)
>>      ts.tv_sec = 0;
>>      ts.tv_nsec = 20 * 1000000;
>>      chk_error(nanosleep(&ts, &rem));
>> -    if (rem.tv_sec != 1)
>> +    if (rem.tv_sec != 1) {
>> +        if (getenv("TRAVIS_ARCH")) {
>> +            printf("nanosleep missing? skipping 'time' test\n");
>> +            return;
>> +        }
>>          error("nanosleep");
>> +    }
>>      chk_error(gettimeofday(&tv2, NULL));
>>      ti = tv2.tv_sec - tv.tv_sec;
>>      if (ti >= 2)
>> 
>
> Well, in the end I think the problem is in linux-user:
>
> We copy the "rem" structure even if there is no error, so "1" is
> overwritten.
>
> We don't have the problem on all architectures because some use
> nanosleep() syscall (that is correct) others use clock_nanosleep()
> syscall that is not correct.
>
> This should fix the problem:
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1211e759c26c..130005716ece 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -11831,7 +11831,7 @@ static abi_long do_syscall1(void *cpu_env, int
> num, abi_long arg1,
>          target_to_host_timespec(&ts, arg3);
>          ret = get_errno(safe_clock_nanosleep(arg1, arg2,
>                                               &ts, arg4 ? &ts : NULL));
> -        if (arg4)
> +        if (is_error(ret) && arg4)
>              host_to_target_timespec(arg4, &ts);

So the testcase worked and caught something :-)

I don't know why I couldn't trigger it reliably on my own dev setting though...

>
>  #if defined(TARGET_PPC)
>
> Thanks,
> Laurent


-- 
Alex Bennée