[PATCH] Correct timespec implementation [BZ #26232]
Carlos O'Donell
carlos@redhat.com
Tue Jul 14 02:35:41 GMT 2020
On 7/13/20 7:30 PM, H.J. Lu wrote:
> On Sat, Jul 11, 2020 at 9:31 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Sat, Jul 11, 2020 at 7:45 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>
>>> On Fri, Jul 10, 2020 at 4:10 PM Tulio Magno Quites Machado Filho
>>> <tuliom@ascii.art.br> wrote:
>>>>
>>>> Carlos O'Donell via Libc-alpha <libc-alpha@sourceware.org> writes:
>>>>
>>>>> OK for master. I'd like to see this fixed in glibc 2.32.
>>>>> Thank you for fixing the test case!
>>>>>
>>>>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>>>>
>>>> Pushed as 04deeaa9ea74b0679dfc9d9155a37b6425f19a9f.
>>>>
>>>
>>> support/tst-timespec failed on i686 and x32:
>>>
>>> Test case 10
>>> tst-timespec.c:312: numeric comparison failure
>>> left: 0 (0x0); from: support_timespec_check_in_range
>>> (check_cases[i].expected, check_cases[i].observed,
>>> check_cases[i].lower_bound, check_cases[i].upper_bound)
>>> right: 1 (0x1); from: check_cases[i].result
>>> Test case 11
>>>
>>
>> Usage of long may be the problem.
>>
>
> commit 04deeaa9ea74b0679dfc9d9155a37b6425f19a9f
> Author: Lucas A. M. Magalhaes <lamm@linux.ibm.com>
> Date: Fri Jul 10 19:41:06 2020 -0300
>
> Fix time/tst-cpuclock1 intermitent failures
>
> has 2 issues:
>
> 1. It assumes time_t == long which is false on x32.
> 2. tst-timespec.c is compiled without -fexcess-precision=standard which
> generates incorrect results on i686 in support_timespec_check_in_range:
>
> double ratio = (double)observed_norm / expected_norm;
> return (lower_bound <= ratio && ratio <= upper_bound);
>
> This patch does
>
> 1. Compile tst-timespec.c with -fexcess-precision=standard.
> 2. Replace long with time_t.
> 3. Replace LONG_MIN and LONG_MAX with TYPE_MINIMUM (time_t) and
> TYPE_MAXIMUM (time_t).
>
> OK for master?
OK for 2.32 with comment.
Reivewed-by: Carlos O'Donell <carlos@redhat.com>
> From 3a3003d3a429035d7cfb8485c64a22cb9517ec48 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Mon, 13 Jul 2020 16:15:56 -0700
> Subject: [PATCH] Correct timespec implementation [BZ #26232]
>
> commit 04deeaa9ea74b0679dfc9d9155a37b6425f19a9f
> Author: Lucas A. M. Magalhaes <lamm@linux.ibm.com>
> Date: Fri Jul 10 19:41:06 2020 -0300
>
> Fix time/tst-cpuclock1 intermitent failures
>
> has 2 issues:
>
> 1. It assumes time_t == long which is false on x32.
Thanks. I had no considered x32 during the review.
> 2. tst-timespec.c is compiled without -fexcess-precision=standard which
> generates incorrect results on i686 in support_timespec_check_in_range:
>
> double ratio = (double)observed_norm / expected_norm;
> return (lower_bound <= ratio && ratio <= upper_bound);
Yes, some of the tests have bounds which are quite tight. We could either
loosen the bounds or compile as you indicate. I'm OK either way. I'll
ack this patch.
> This patch does
>
> 1. Compile tst-timespec.c with -fexcess-precision=standard.
> 2. Replace long with time_t.
> 3. Replace LONG_MIN and LONG_MAX with TYPE_MINIMUM (time_t) and
> TYPE_MAXIMUM (time_t).
> ---
> support/Makefile | 2 +
> support/timespec.c | 18 +++-----
> support/timespec.h | 2 +-
> support/tst-timespec.c | 98 ++++++++++++++++++++++++------------------
> 4 files changed, 66 insertions(+), 54 deletions(-)
>
> diff --git a/support/Makefile b/support/Makefile
> index e74e0dd519..94f84e01eb 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -196,6 +196,8 @@ CFLAGS-support_paths.c = \
> -DROOTSBINDIR_PATH=\"$(rootsbindir)\" \
> -DCOMPLOCALEDIR_PATH=\"$(complocaledir)\"
>
> +CFLAGS-timespec.c += -fexcess-precision=standard
OK with this comment:
# In support_timespec_check_in_range we may be passed a very
# tight range for which we should produce a correct result
# for expected being withing the observed range. The code
# uses double internally in support_timespec_check_in_range
# and for that computation we use -fexcess-precision=standard.
> +
> ifeq (,$(CXX))
> LINKS_DSO_PROGRAM = links-dso-program-c
> else
> diff --git a/support/timespec.c b/support/timespec.c
> index 9f5449e49e..edbdb165ec 100644
> --- a/support/timespec.c
> +++ b/support/timespec.c
> @@ -60,21 +60,17 @@ test_timespec_equal_or_after_impl (const char *file, int line,
> }
> }
>
> -/* Convert TIME to nanoseconds stored in a long.
> - Returns long maximum or minimum if the conversion overflows
> +/* Convert TIME to nanoseconds stored in a time_t.
> + Returns time_t maximum or minimum if the conversion overflows
OK.
> or underflows, respectively. */
> -long
> +time_t
> support_timespec_ns (struct timespec time)
> {
> - long time_ns;
> + time_t time_ns;
> if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
> - {
> - return (time.tv_sec < 0) ? TYPE_MINIMUM(long): TYPE_MAXIMUM(long);
> - }
> + return time.tv_sec < 0 ? TYPE_MINIMUM(time_t) : TYPE_MAXIMUM(time_t);
> if (INT_ADD_WRAPV(time_ns, time.tv_nsec, &time_ns))
> - {
> - return (time.tv_nsec < 0) ? TYPE_MINIMUM(long): TYPE_MAXIMUM(long);
> - }
> + return time.tv_nsec < 0 ? TYPE_MINIMUM(time_t) : TYPE_MAXIMUM(time_t);
OK.
> return time_ns;
> }
>
> @@ -113,7 +109,7 @@ support_timespec_check_in_range (struct timespec expected, struct timespec obser
> double lower_bound, double upper_bound)
> {
> assert (upper_bound >= lower_bound);
> - long expected_norm, observed_norm;
> + time_t expected_norm, observed_norm;
OK.
> expected_norm = support_timespec_ns (expected);
> /* Don't divide by zero */
> assert(expected_norm != 0);
> diff --git a/support/timespec.h b/support/timespec.h
> index fd5466745d..1a6775a882 100644
> --- a/support/timespec.h
> +++ b/support/timespec.h
> @@ -48,7 +48,7 @@ void test_timespec_equal_or_after_impl (const char *file, int line,
> const struct timespec left,
> const struct timespec right);
>
> -long support_timespec_ns (struct timespec time);
> +time_t support_timespec_ns (struct timespec time);
OK.
>
> struct timespec support_timespec_normalize (struct timespec time);
>
> diff --git a/support/tst-timespec.c b/support/tst-timespec.c
> index 71423555a9..ac5ed228ba 100644
> --- a/support/tst-timespec.c
> +++ b/support/tst-timespec.c
> @@ -19,13 +19,14 @@
> #include <support/timespec.h>
> #include <support/check.h>
> #include <limits.h>
> +#include <intprops.h>
OK.
>
> #define TIMESPEC_HZ 1000000000
>
> struct timespec_ns_test_case
> {
> struct timespec time;
> - long time_ns;
> + time_t time_ns;
OK.
> };
>
> struct timespec_norm_test_case
> @@ -43,6 +44,9 @@ struct timespec_test_case
> int result;
> };
>
> +#define TIME_T_MIN TYPE_MINIMUM (time_t)
> +#define TIME_T_MAX TYPE_MAXIMUM (time_t)
OK.
> +
> /* Test cases for timespec_ns */
> struct timespec_ns_test_case ns_cases[] = {
> {.time = {.tv_sec = 0, .tv_nsec = 0},
> @@ -73,36 +77,42 @@ struct timespec_ns_test_case ns_cases[] = {
> .time_ns = -TIMESPEC_HZ + 1,
> },
> /* Overflow bondary by 2 */
> - {.time = {.tv_sec = LONG_MAX / TIMESPEC_HZ, .tv_nsec = LONG_MAX%TIMESPEC_HZ - 1},
> - .time_ns = LONG_MAX - 1,
> + {.time = {.tv_sec = TIME_T_MAX / TIMESPEC_HZ,
> + .tv_nsec = TIME_T_MAX % TIMESPEC_HZ - 1},
> + .time_ns = TIME_T_MAX - 1,
> },
> /* Overflow bondary */
> - {.time = {.tv_sec = LONG_MAX / TIMESPEC_HZ, .tv_nsec = LONG_MAX%TIMESPEC_HZ},
> - .time_ns = LONG_MAX,
> + {.time = {.tv_sec = TIME_T_MAX / TIMESPEC_HZ,
> + .tv_nsec = TIME_T_MAX % TIMESPEC_HZ},
> + .time_ns = TIME_T_MAX,
> },
> /* Underflow bondary by 1 */
> - {.time = {.tv_sec = LONG_MIN / TIMESPEC_HZ, .tv_nsec = LONG_MIN%TIMESPEC_HZ + 1},
> - .time_ns = LONG_MIN + 1,
> + {.time = {.tv_sec = TIME_T_MIN / TIMESPEC_HZ,
> + .tv_nsec = TIME_T_MIN % TIMESPEC_HZ + 1},
> + .time_ns = TIME_T_MIN + 1,
> },
> /* Underflow bondary */
> - {.time = {.tv_sec = LONG_MIN / TIMESPEC_HZ, .tv_nsec = LONG_MIN%TIMESPEC_HZ},
> - .time_ns = LONG_MIN,
> + {.time = {.tv_sec = TIME_T_MIN / TIMESPEC_HZ,
> + .tv_nsec = TIME_T_MIN % TIMESPEC_HZ},
> + .time_ns = TIME_T_MIN,
> },
> /* Multiplication overflow */
> - {.time = {.tv_sec = LONG_MAX / TIMESPEC_HZ + 1, .tv_nsec = 1},
> - .time_ns = LONG_MAX,
> + {.time = {.tv_sec = TIME_T_MAX / TIMESPEC_HZ + 1, .tv_nsec = 1},
> + .time_ns = TIME_T_MAX,
> },
> /* Multiplication underflow */
> - {.time = {.tv_sec = LONG_MIN / TIMESPEC_HZ - 1, .tv_nsec = -1},
> - .time_ns = LONG_MIN,
> + {.time = {.tv_sec = TIME_T_MIN / TIMESPEC_HZ - 1, .tv_nsec = -1},
> + .time_ns = TIME_T_MIN,
> },
> /* Sum overflows */
> - {.time = {.tv_sec = LONG_MAX / TIMESPEC_HZ, .tv_nsec = LONG_MAX%TIMESPEC_HZ + 1},
> - .time_ns = LONG_MAX,
> + {.time = {.tv_sec = TIME_T_MAX / TIMESPEC_HZ,
> + .tv_nsec = TIME_T_MAX % TIMESPEC_HZ + 1},
> + .time_ns = TIME_T_MAX,
> },
> /* Sum underflow */
> - {.time = {.tv_sec = LONG_MIN / TIMESPEC_HZ, .tv_nsec = LONG_MIN%TIMESPEC_HZ - 1},
> - .time_ns = LONG_MIN,
> + {.time = {.tv_sec = TIME_T_MIN / TIMESPEC_HZ,
> + .tv_nsec = TIME_T_MIN % TIMESPEC_HZ - 1},
> + .time_ns = TIME_T_MIN,
> }
> };
>
> @@ -144,28 +154,28 @@ struct timespec_norm_test_case norm_cases[] = {
> .norm = {.tv_sec = -2, .tv_nsec = -1}
> },
> /* Overflow bondary by 2 */
> - {.time = {.tv_sec = LONG_MAX - 2, .tv_nsec = TIMESPEC_HZ + 1},
> - .norm = {.tv_sec = LONG_MAX - 1, 1},
> + {.time = {.tv_sec = TIME_T_MAX - 2, .tv_nsec = TIMESPEC_HZ + 1},
> + .norm = {.tv_sec = TIME_T_MAX - 1, 1},
> },
> /* Overflow bondary by 1 */
> - {.time = {.tv_sec = LONG_MAX - 1, .tv_nsec = TIMESPEC_HZ + 1},
> - .norm = {.tv_sec = LONG_MAX, .tv_nsec = 1},
> + {.time = {.tv_sec = TIME_T_MAX - 1, .tv_nsec = TIMESPEC_HZ + 1},
> + .norm = {.tv_sec = TIME_T_MAX, .tv_nsec = 1},
> },
> /* Underflow bondary by 2 */
> - {.time = {.tv_sec = LONG_MIN + 2, .tv_nsec = -TIMESPEC_HZ - 1},
> - .norm = {.tv_sec = LONG_MIN + 1, -1},
> + {.time = {.tv_sec = TIME_T_MIN + 2, .tv_nsec = -TIMESPEC_HZ - 1},
> + .norm = {.tv_sec = TIME_T_MIN + 1, -1},
> },
> /* Underflow bondary by 1 */
> - {.time = {.tv_sec = LONG_MIN + 1, .tv_nsec = -TIMESPEC_HZ - 1},
> - .norm = {.tv_sec = LONG_MIN, .tv_nsec = -1},
> + {.time = {.tv_sec = TIME_T_MIN + 1, .tv_nsec = -TIMESPEC_HZ - 1},
> + .norm = {.tv_sec = TIME_T_MIN, .tv_nsec = -1},
> },
> /* SUM overflow */
> - {.time = {.tv_sec = LONG_MAX, .tv_nsec = TIMESPEC_HZ},
> - .norm = {.tv_sec = LONG_MAX, .tv_nsec = TIMESPEC_HZ - 1},
> + {.time = {.tv_sec = TIME_T_MAX, .tv_nsec = TIMESPEC_HZ},
> + .norm = {.tv_sec = TIME_T_MAX, .tv_nsec = TIMESPEC_HZ - 1},
> },
> /* SUM underflow */
> - {.time = {.tv_sec = LONG_MIN, .tv_nsec = -TIMESPEC_HZ},
> - .norm = {.tv_sec = LONG_MIN, .tv_nsec = -1 * (TIMESPEC_HZ - 1)},
> + {.time = {.tv_sec = TIME_T_MIN, .tv_nsec = -TIMESPEC_HZ},
> + .norm = {.tv_sec = TIME_T_MIN, .tv_nsec = -1 * (TIMESPEC_HZ - 1)},
> }
> };
>
> @@ -243,39 +253,41 @@ struct timespec_test_case check_cases[] = {
> },
> /* Maximum/Minimum long values */
> /* 14 */
> - {.expected = {.tv_sec = LONG_MAX, .tv_nsec = TIMESPEC_HZ - 1},
> - .observed = {.tv_sec = LONG_MAX, .tv_nsec = TIMESPEC_HZ - 2},
> + {.expected = {.tv_sec = TIME_T_MAX, .tv_nsec = TIMESPEC_HZ - 1},
> + .observed = {.tv_sec = TIME_T_MAX, .tv_nsec = TIMESPEC_HZ - 2},
> .upper_bound = 1, .lower_bound = .9, .result = 1,
> },
> /* 15 - support_timespec_ns overflow */
> - {.expected = {.tv_sec = LONG_MAX, .tv_nsec = TIMESPEC_HZ},
> - .observed = {.tv_sec = LONG_MAX, .tv_nsec = TIMESPEC_HZ},
> + {.expected = {.tv_sec = TIME_T_MAX, .tv_nsec = TIMESPEC_HZ},
> + .observed = {.tv_sec = TIME_T_MAX, .tv_nsec = TIMESPEC_HZ},
> .upper_bound = 1, .lower_bound = 1, .result = 1,
> },
> /* 16 - support_timespec_ns overflow + underflow */
> - {.expected = {.tv_sec = LONG_MAX, .tv_nsec = TIMESPEC_HZ},
> - .observed = {.tv_sec = LONG_MIN, .tv_nsec = -TIMESPEC_HZ},
> + {.expected = {.tv_sec = TIME_T_MAX, .tv_nsec = TIMESPEC_HZ},
> + .observed = {.tv_sec = TIME_T_MIN, .tv_nsec = -TIMESPEC_HZ},
> .upper_bound = 1, .lower_bound = 1, .result = 0,
> },
> /* 17 - support_timespec_ns underflow */
> - {.expected = {.tv_sec = LONG_MIN, .tv_nsec = -TIMESPEC_HZ},
> - .observed = {.tv_sec = LONG_MIN, .tv_nsec = -TIMESPEC_HZ},
> + {.expected = {.tv_sec = TIME_T_MIN, .tv_nsec = -TIMESPEC_HZ},
> + .observed = {.tv_sec = TIME_T_MIN, .tv_nsec = -TIMESPEC_HZ},
> .upper_bound = 1, .lower_bound = 1, .result = 1,
> },
> /* 18 - support_timespec_ns underflow + overflow */
> - {.expected = {.tv_sec = LONG_MIN, .tv_nsec = -TIMESPEC_HZ},
> - .observed = {.tv_sec = LONG_MAX, .tv_nsec = TIMESPEC_HZ},
> + {.expected = {.tv_sec = TIME_T_MIN, .tv_nsec = -TIMESPEC_HZ},
> + .observed = {.tv_sec = TIME_T_MAX, .tv_nsec = TIMESPEC_HZ},
> .upper_bound = 1, .lower_bound = 1, .result = 0,
> },
> /* 19 - Biggest division */
> - {.expected = {.tv_sec = LONG_MAX / TIMESPEC_HZ , .tv_nsec = TIMESPEC_HZ - 1},
> + {.expected = {.tv_sec = TIME_T_MAX / TIMESPEC_HZ,
> + .tv_nsec = TIMESPEC_HZ - 1},
> .observed = {.tv_sec = 0, .tv_nsec = 1},
> .upper_bound = 1, .lower_bound = 1.0842021724855044e-19, .result = 1,
> },
> /* 20 - Lowest division */
> {.expected = {.tv_sec = 0, .tv_nsec = 1},
> - .observed = {.tv_sec = LONG_MAX / TIMESPEC_HZ , .tv_nsec = TIMESPEC_HZ - 1},
> - .upper_bound = LONG_MAX, .lower_bound = 1, .result = 1,
> + .observed = {.tv_sec = TIME_T_MAX / TIMESPEC_HZ,
> + .tv_nsec = TIMESPEC_HZ - 1},
> + .upper_bound = TIME_T_MAX, .lower_bound = 1, .result = 1,
OK.
> },
> };
>
> @@ -288,6 +300,7 @@ do_test (void)
> printf("Testing support_timespec_ns\n");
> for (i = 0; i < ntests; i++)
> {
> + printf("Test case %d\n", i);
OK.
> TEST_COMPARE (support_timespec_ns (ns_cases[i].time),
> ns_cases[i].time_ns);
> }
> @@ -297,6 +310,7 @@ do_test (void)
> printf("Testing support_timespec_normalize\n");
> for (i = 0; i < ntests; i++)
> {
> + printf("Test case %d\n", i);
OK.
> result = support_timespec_normalize (norm_cases[i].time);
> TEST_COMPARE (norm_cases[i].norm.tv_sec, result.tv_sec);
> TEST_COMPARE (norm_cases[i].norm.tv_nsec, result.tv_nsec);
> --
> 2.26.2
>
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list