From fce8577f730be891f0e9798f6fcb1f1b41a5c920 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Mon, 10 Apr 2023 17:06:01 +0800 Subject: [PATCH 1/8] try to reproduce random assert fail Signed-off-by: Jerry Yu --- tests/suites/test_suite_platform.function | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_platform.function b/tests/suites/test_suite_platform.function index 54ddd42c3..1d661b26a 100644 --- a/tests/suites/test_suite_platform.function +++ b/tests/suites/test_suite_platform.function @@ -84,7 +84,8 @@ void time_delay_seconds(int delay_secs) sleep_ms(delay_secs * 1000); elapsed_secs = mbedtls_time(NULL) - current; - TEST_ASSERT(elapsed_secs >= delay_secs && elapsed_secs < 4 + delay_secs); + TEST_ASSERT(elapsed_secs >= delay_secs); + TEST_ASSERT(elapsed_secs < 4 + delay_secs); /* This goto is added to avoid warnings from the generated code. */ goto exit; } From c9c3e62b3ed28dd18775fb3d733a086a98196031 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Tue, 11 Apr 2023 14:08:23 +0800 Subject: [PATCH 2/8] workaround the assert fail with tollerance Signed-off-by: Jerry Yu --- tests/suites/test_suite_platform.function | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_platform.function b/tests/suites/test_suite_platform.function index 1d661b26a..f4e12bff3 100644 --- a/tests/suites/test_suite_platform.function +++ b/tests/suites/test_suite_platform.function @@ -84,7 +84,12 @@ void time_delay_seconds(int delay_secs) sleep_ms(delay_secs * 1000); elapsed_secs = mbedtls_time(NULL) - current; - TEST_ASSERT(elapsed_secs >= delay_secs); + + /* Built-in mbedtls_time function returns the number of seconds since the + * Epoch. That is affected by discontinuous jumps and cause test fail. + * Workaround it with 1 seconds tollerance. + */ + TEST_ASSERT(elapsed_secs >= delay_secs - 1); TEST_ASSERT(elapsed_secs < 4 + delay_secs); /* This goto is added to avoid warnings from the generated code. */ goto exit; From 2f1e85f47ea0d3cac78bd92dcdc92b226335d143 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Mon, 17 Apr 2023 16:47:50 +0800 Subject: [PATCH 3/8] fix comments issues Signed-off-by: Jerry Yu --- tests/suites/test_suite_platform.function | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_platform.function b/tests/suites/test_suite_platform.function index f4e12bff3..2584d3dfc 100644 --- a/tests/suites/test_suite_platform.function +++ b/tests/suites/test_suite_platform.function @@ -86,8 +86,11 @@ void time_delay_seconds(int delay_secs) elapsed_secs = mbedtls_time(NULL) - current; /* Built-in mbedtls_time function returns the number of seconds since the - * Epoch. That is affected by discontinuous jumps and cause test fail. - * Workaround it with 1 seconds tollerance. + * Epoch. That is affected by discontinuous jumps. And `nanosleep` use + * CLOCK_MONOTONIC(monotonically-increasing time source), That will cause + * negative elapsed time difference. + * + * Workaround it with 1 second tolerance. */ TEST_ASSERT(elapsed_secs >= delay_secs - 1); TEST_ASSERT(elapsed_secs < 4 + delay_secs); From 4852bb823f247a8f0d5f180c5a9669b99677fc49 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Tue, 18 Apr 2023 13:50:24 +0800 Subject: [PATCH 4/8] remove time delay tests See #1517. They often failed on the CI. Signed-off-by: Jerry Yu --- tests/suites/test_suite_platform.data | 6 --- tests/suites/test_suite_platform.function | 61 ----------------------- 2 files changed, 67 deletions(-) diff --git a/tests/suites/test_suite_platform.data b/tests/suites/test_suite_platform.data index 557b586eb..4276b8fb7 100644 --- a/tests/suites/test_suite_platform.data +++ b/tests/suites/test_suite_platform.data @@ -4,9 +4,3 @@ time_get_milliseconds: Time: get seconds time_get_seconds: - -Time: delay milliseconds -time_delay_milliseconds:1000 - -Time: delay seconds -time_delay_seconds:1 diff --git a/tests/suites/test_suite_platform.function b/tests/suites/test_suite_platform.function index 2584d3dfc..aadcc39c2 100644 --- a/tests/suites/test_suite_platform.function +++ b/tests/suites/test_suite_platform.function @@ -10,28 +10,6 @@ #if defined(MBEDTLS_HAVE_TIME) #include "mbedtls/platform_time.h" -#ifdef WIN32 -#include -#elif _POSIX_C_SOURCE >= 199309L -#include -#else -#include -#endif -void sleep_ms(int milliseconds) -{ -#ifdef WIN32 - Sleep(milliseconds); -#elif _POSIX_C_SOURCE >= 199309L - struct timespec ts; - ts.tv_sec = milliseconds / 1000; - ts.tv_nsec = (milliseconds % 1000) * 1000000; - nanosleep(&ts, NULL); -#else - usleep(milliseconds * 1000); -#endif -} -#endif - /* END_HEADER */ /* BEGIN_DEPENDENCIES */ @@ -59,42 +37,3 @@ void time_get_seconds() goto exit; } /* END_CASE */ - -/* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */ -void time_delay_milliseconds(int delay_ms) -{ - mbedtls_ms_time_t current = mbedtls_ms_time(); - mbedtls_ms_time_t elapsed_ms; - - sleep_ms(delay_ms); - - elapsed_ms = mbedtls_ms_time() - current; - TEST_ASSERT(elapsed_ms >= delay_ms && elapsed_ms < 4000 + delay_ms); - /* This goto is added to avoid warnings from the generated code. */ - goto exit; -} -/* END_CASE */ - -/* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */ -void time_delay_seconds(int delay_secs) -{ - mbedtls_time_t current = mbedtls_time(NULL); - mbedtls_time_t elapsed_secs; - - sleep_ms(delay_secs * 1000); - - elapsed_secs = mbedtls_time(NULL) - current; - - /* Built-in mbedtls_time function returns the number of seconds since the - * Epoch. That is affected by discontinuous jumps. And `nanosleep` use - * CLOCK_MONOTONIC(monotonically-increasing time source), That will cause - * negative elapsed time difference. - * - * Workaround it with 1 second tolerance. - */ - TEST_ASSERT(elapsed_secs >= delay_secs - 1); - TEST_ASSERT(elapsed_secs < 4 + delay_secs); - /* This goto is added to avoid warnings from the generated code. */ - goto exit; -} -/* END_CASE */ From d1190a5af3f8fbdbaa154799d440c3c53e151209 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Tue, 18 Apr 2023 16:29:02 +0800 Subject: [PATCH 5/8] Update comments and remove delay seconds test Signed-off-by: Jerry Yu --- tests/suites/test_suite_platform.data | 3 + tests/suites/test_suite_platform.function | 69 ++++++++++++++++++++++- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_platform.data b/tests/suites/test_suite_platform.data index 4276b8fb7..39b423f3d 100644 --- a/tests/suites/test_suite_platform.data +++ b/tests/suites/test_suite_platform.data @@ -4,3 +4,6 @@ time_get_milliseconds: Time: get seconds time_get_seconds: + +Time: delay milliseconds +time_delay_milliseconds:1000 diff --git a/tests/suites/test_suite_platform.function b/tests/suites/test_suite_platform.function index aadcc39c2..54d394470 100644 --- a/tests/suites/test_suite_platform.function +++ b/tests/suites/test_suite_platform.function @@ -10,14 +10,34 @@ #if defined(MBEDTLS_HAVE_TIME) #include "mbedtls/platform_time.h" +#ifdef WIN32 +#include +#elif _POSIX_C_SOURCE >= 199309L +#include +#else +#include +#endif +void sleep_ms(int milliseconds) +{ +#ifdef WIN32 + Sleep(milliseconds); +#elif _POSIX_C_SOURCE >= 199309L + struct timespec ts; + ts.tv_sec = milliseconds / 1000; + ts.tv_nsec = (milliseconds % 1000) * 1000000; + nanosleep(&ts, NULL); +#else + usleep(milliseconds * 1000); +#endif +} +#endif + /* END_HEADER */ /* BEGIN_DEPENDENCIES */ /* END_DEPENDENCIES */ - - /* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */ void time_get_milliseconds() { @@ -37,3 +57,48 @@ void time_get_seconds() goto exit; } /* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */ +void time_delay_milliseconds(int delay_ms) +{ + mbedtls_ms_time_t current = mbedtls_ms_time(); + mbedtls_ms_time_t elapsed_ms; + + sleep_ms(delay_ms); + + elapsed_ms = mbedtls_ms_time() - current; + TEST_ASSERT(elapsed_ms >= delay_ms && elapsed_ms < 4000 + delay_ms); + /* This goto is added to avoid warnings from the generated code. */ + goto exit; +} +/* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */ +void time_delay_seconds(int delay_secs) +{ + mbedtls_time_t current = mbedtls_time(NULL); + mbedtls_time_t elapsed_secs; + + sleep_ms(delay_secs * 1000); + + elapsed_secs = mbedtls_time(NULL) - current; + + /* + * `mbedtls_time` was defined as c99 function `time`, it uses + * CLOCK_REALTIME and returns the number of seconds since the Epoch. And + * `nanosleep` uses CLOCK_MONOTONIC. The time sources are out of sync. + * + * If CLOCK_MONOTONIC is faster than CLOCK_REALTIME and `nanosleep` exits at + * the end of a second, `elapsed_secs` will be less than `delay_secs`. + * + * Workaround it with 1 second tolerance. + */ + TEST_ASSERT(elapsed_secs >= delay_secs - 1); + /* If CLOCK_MONOTONIC is slower than CLOCK_REALTIME or nanosleep does not + * exit on time. + */ + TEST_ASSERT(elapsed_secs < 4 + delay_secs); + /* This goto is added to avoid warnings from the generated code. */ + goto exit; +} +/* END_CASE */ From ed9b9a7579b6aec7406acd2af5bdc45efd507789 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Tue, 18 Apr 2023 17:09:03 +0800 Subject: [PATCH 6/8] Add warning to reserve the reason The test has some issues we can not avoid. Put it in code to avoid it is re-inroduced again Signed-off-by: Jerry Yu --- tests/suites/test_suite_platform.function | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/suites/test_suite_platform.function b/tests/suites/test_suite_platform.function index 54d394470..8dd4098f3 100644 --- a/tests/suites/test_suite_platform.function +++ b/tests/suites/test_suite_platform.function @@ -74,6 +74,13 @@ void time_delay_milliseconds(int delay_ms) /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */ + +/* + * WARNING: DONOT ENABLE THIS TEST. RESERVE IT HERE TO KEEP THE REASON. + * + * The test often failed on the CI. See #1517. CI failures cannot be + * completely avoided due to out-of-sync clock sources. + */ void time_delay_seconds(int delay_secs) { mbedtls_time_t current = mbedtls_time(NULL); From d3c7d538f1aa41518924f2dc138942970aaf11d0 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Wed, 19 Apr 2023 14:07:59 +0800 Subject: [PATCH 7/8] Improve comments about the time_delay test. Signed-off-by: Jerry Yu --- tests/suites/test_suite_platform.function | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/suites/test_suite_platform.function b/tests/suites/test_suite_platform.function index 8dd4098f3..3d0f52bd1 100644 --- a/tests/suites/test_suite_platform.function +++ b/tests/suites/test_suite_platform.function @@ -91,20 +91,21 @@ void time_delay_seconds(int delay_secs) elapsed_secs = mbedtls_time(NULL) - current; /* - * `mbedtls_time` was defined as c99 function `time`, it uses - * CLOCK_REALTIME and returns the number of seconds since the Epoch. And - * `nanosleep` uses CLOCK_MONOTONIC. The time sources are out of sync. + * `mbedtls_time()` was defined as c99 function `time()`, returns the number + * of seconds since the Epoch. And it is affected by discontinuous changes + * from automatic drift adjustment or time setting system call. The POSIX.1 + * specification for clock_settime says that discontinuous changes in + * CLOCK_REALTIME should not affect `nanosleep()`. * - * If CLOCK_MONOTONIC is faster than CLOCK_REALTIME and `nanosleep` exits at - * the end of a second, `elapsed_secs` will be less than `delay_secs`. + * If discontinuous changes occur during `nanosleep()`, we will get + * `elapsed_secs < delay_secs` for backward and `elapsed_secs > delay_secs` + * for forward. * - * Workaround it with 1 second tolerance. + * The following tolerance windows cannot be guaranteed. + * PLEASE DO NOT ENABLE IT IN CI TEST. */ - TEST_ASSERT(elapsed_secs >= delay_secs - 1); - /* If CLOCK_MONOTONIC is slower than CLOCK_REALTIME or nanosleep does not - * exit on time. - */ - TEST_ASSERT(elapsed_secs < 4 + delay_secs); + TEST_ASSERT(elapsed_secs - delay_secs >= -1 && + elapsed_secs - delay_secs < 4); /* This goto is added to avoid warnings from the generated code. */ goto exit; } From ad2091d9c224bfa96433106ad281ed93c1c42b2a Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Thu, 20 Apr 2023 10:01:42 +0800 Subject: [PATCH 8/8] fix grammar issues Signed-off-by: Jerry Yu --- tests/suites/test_suite_platform.function | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_platform.function b/tests/suites/test_suite_platform.function index 3d0f52bd1..7453c32c9 100644 --- a/tests/suites/test_suite_platform.function +++ b/tests/suites/test_suite_platform.function @@ -76,7 +76,8 @@ void time_delay_milliseconds(int delay_ms) /* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */ /* - * WARNING: DONOT ENABLE THIS TEST. RESERVE IT HERE TO KEEP THE REASON. + * WARNING: DO NOT ENABLE THIS TEST. We keep the code here to document the + * reason. * * The test often failed on the CI. See #1517. CI failures cannot be * completely avoided due to out-of-sync clock sources. @@ -98,7 +99,7 @@ void time_delay_seconds(int delay_secs) * CLOCK_REALTIME should not affect `nanosleep()`. * * If discontinuous changes occur during `nanosleep()`, we will get - * `elapsed_secs < delay_secs` for backward and `elapsed_secs > delay_secs` + * `elapsed_secs < delay_secs` for backward or `elapsed_secs > delay_secs` * for forward. * * The following tolerance windows cannot be guaranteed.