Correct timespec implementation [BZ #26232]

  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?

Thanks.
  

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
>
  

* H. J. Lu via Libc-alpha:

> -/* 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
>     or underflows, respectively.  */
> -long
> +time_t
>  support_timespec_ns (struct timespec time)
>  {

Why not use long long int as the type?

Thanks,
Florian
  

On Tue, Jul 14, 2020 at 4:16 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > -/* 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
> >     or underflows, respectively.  */
> > -long
> > +time_t
> >  support_timespec_ns (struct timespec time)
> >  {
>
> Why not use long long int as the type?
>

Using time_t has the least impact since most of the targets have time_t == long.
I am checking in this patch and will post a followup patch with long long.
  

On Tue, Jul 14, 2020 at 4:42 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 4:16 AM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * H. J. Lu via Libc-alpha:
> >
> > > -/* 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
> > >     or underflows, respectively.  */
> > > -long
> > > +time_t
> > >  support_timespec_ns (struct timespec time)
> > >  {
> >
> > Why not use long long int as the type?
> >
>
> Using time_t has the least impact since most of the targets have time_t == long.
> I am checking in this patch and will post a followup patch with long long.
>

There are

time_t
support_timespec_ns (struct timespec time)
{
  time_t time_ns;
  if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
    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(time_t) : TYPE_MAXIMUM(time_t);
  return time_ns;
}

Even if support_timespec_ns is changed to return long long, we still may need to
keep

 time_t time_ns;

for

   if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))

and

  if (INT_ADD_WRAPV(time_ns, time.tv_nsec, &time_ns))

It looks odd to return long long here.
  

* H. J. Lu:

> There are
>
> time_t
> support_timespec_ns (struct timespec time)
> {
>   time_t time_ns;
>   if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
>     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(time_t) : TYPE_MAXIMUM(time_t);
>   return time_ns;
> }
>
> Even if support_timespec_ns is changed to return long long, we still
> may need to keep
>
>  time_t time_ns;
>
> for
>
>    if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
>
> and
>
>   if (INT_ADD_WRAPV(time_ns, time.tv_nsec, &time_ns))
>
> It looks odd to return long long here.

Why?

You can use the GCC built-ins for checked arithmetic, they support mixed
types.

Thanks,
Florian
  

Hi H.J. Lu,
Thanks for working on this. Overall the patch seams right to me. Just
one comment on style down there.

> From 67ff24ce5b9b294089776563d656ee7678c6d076 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.
> 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).
> ---
>  support/Makefile       |  7 +++
>  support/timespec.c     | 18 +++-----
>  support/timespec.h     |  2 +-
>  support/tst-timespec.c | 98 ++++++++++++++++++++++++------------------
>  4 files changed, 71 insertions(+), 54 deletions(-)
> 
> diff --git a/support/Makefile b/support/Makefile
> index e74e0dd519..93faafddf9 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -196,6 +196,13 @@ CFLAGS-support_paths.c = \
>  		-DROOTSBINDIR_PATH=\"$(rootsbindir)\" \
>  		-DCOMPLOCALEDIR_PATH=\"$(complocaledir)\"
>  
> +# 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.
> +CFLAGS-timespec.c += -fexcess-precision=standard
> +
OK.

>  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
>     or underflows, respectively.  */
> -long
> +time_t
Ok.

>  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);
>    return time_ns;
>  }
>  
Ok.

> @@ -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;
>    expected_norm = support_timespec_ns (expected);
>    /* Don't divide by zero  */
>    assert(expected_norm != 0);
Ok.

> 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);
>  
>  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>
>  
>  #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)
> +
>  /* 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,
>    }
>  };
>  

Please don't mix styles here.
> @@ -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)},
>    }
>  };
>  
OK.

> @@ -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,
>    },
>  };
>  
> @@ -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);
>        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);
>        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);
OK.

---
Lucas A. M. Magalhães
  

On Tue, Jul 14, 2020 at 5:18 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > There are
> >
> > time_t
> > support_timespec_ns (struct timespec time)
> > {
> >   time_t time_ns;
> >   if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
> >     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(time_t) : TYPE_MAXIMUM(time_t);
> >   return time_ns;
> > }
> >
> > Even if support_timespec_ns is changed to return long long, we still
> > may need to keep
> >
> >  time_t time_ns;
> >
> > for
> >
> >    if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
> >
> > and
> >
> >   if (INT_ADD_WRAPV(time_ns, time.tv_nsec, &time_ns))
> >
> > It looks odd to return long long here.
>
> Why?
>
> You can use the GCC built-ins for checked arithmetic, they support mixed
> types.
>

This doesn't work on i686:

diff --git a/support/timespec.c b/support/timespec.c
index edbdb165ec..c35e8a8201 100644
--- a/support/timespec.c
+++ b/support/timespec.c
@@ -63,10 +63,10 @@ test_timespec_equal_or_after_impl (const char
*file, int line,
 /* Convert TIME to nanoseconds stored in a time_t.
    Returns time_t maximum or minimum if the conversion overflows
    or underflows, respectively.  */
-time_t
+long long
 support_timespec_ns (struct timespec time)
 {
-  time_t time_ns;
+  long long time_ns;
   if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
     return time.tv_sec < 0 ? TYPE_MINIMUM(time_t) : TYPE_MAXIMUM(time_t);
   if (INT_ADD_WRAPV(time_ns, time.tv_nsec, &time_ns))
diff --git a/support/timespec.h b/support/timespec.h
index 1a6775a882..68055f42c3 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);

-time_t support_timespec_ns (struct timespec time);
+long long support_timespec_ns (struct timespec time);

 struct timespec support_timespec_normalize (struct timespec time);

This works:

diff --git a/support/timespec.c b/support/timespec.c
index edbdb165ec..b117e7c0d2 100644
--- a/support/timespec.c
+++ b/support/timespec.c
@@ -63,7 +63,7 @@ test_timespec_equal_or_after_impl (const char *file, int line,
 /* Convert TIME to nanoseconds stored in a time_t.
    Returns time_t maximum or minimum if the conversion overflows
    or underflows, respectively.  */
-time_t
+long long
 support_timespec_ns (struct timespec time)
 {
   time_t time_ns;
diff --git a/support/timespec.h b/support/timespec.h
index 1a6775a882..68055f42c3 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);

-time_t support_timespec_ns (struct timespec time);
+long long support_timespec_ns (struct timespec time);

 struct timespec support_timespec_normalize (struct timespec time);

What is the advantage of long long over time_t?
  

* H. J. Lu:

> diff --git a/support/timespec.c b/support/timespec.c
> index edbdb165ec..c35e8a8201 100644
> --- a/support/timespec.c
> +++ b/support/timespec.c
> @@ -63,10 +63,10 @@ test_timespec_equal_or_after_impl (const char
> *file, int line,
>  /* Convert TIME to nanoseconds stored in a time_t.
>     Returns time_t maximum or minimum if the conversion overflows
>     or underflows, respectively.  */
> -time_t
> +long long
>  support_timespec_ns (struct timespec time)
>  {
> -  time_t time_ns;
> +  long long time_ns;
>    if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
>      return time.tv_sec < 0 ? TYPE_MINIMUM(time_t) : TYPE_MAXIMUM(time_t);
>    if (INT_ADD_WRAPV(time_ns, time.tv_nsec, &time_ns))

I suspect you need to use LONG_LONG_MIN and LONG_LONG_MAX here.

Thanks,
Florian
  

On Tue, Jul 14, 2020 at 6:15 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > diff --git a/support/timespec.c b/support/timespec.c
> > index edbdb165ec..c35e8a8201 100644
> > --- a/support/timespec.c
> > +++ b/support/timespec.c
> > @@ -63,10 +63,10 @@ test_timespec_equal_or_after_impl (const char
> > *file, int line,
> >  /* Convert TIME to nanoseconds stored in a time_t.
> >     Returns time_t maximum or minimum if the conversion overflows
> >     or underflows, respectively.  */
> > -time_t
> > +long long
> >  support_timespec_ns (struct timespec time)
> >  {
> > -  time_t time_ns;
> > +  long long time_ns;
> >    if (INT_MULTIPLY_WRAPV(time.tv_sec, TIMESPEC_HZ, &time_ns))
> >      return time.tv_sec < 0 ? TYPE_MINIMUM(time_t) : TYPE_MAXIMUM(time_t);
> >    if (INT_ADD_WRAPV(time_ns, time.tv_nsec, &time_ns))
>
> I suspect you need to use LONG_LONG_MIN and LONG_LONG_MAX here.
>

No, it doesn't work on i686.
  

On 7/14/20 6:17 AM, H.J. Lu via Libc-alpha wrote:
> No, it doesn't work on i686.

Could you give more details about why it doesn't work? Is it a GCC bug (and if 
so, which version and what flags did you use)? Is it a bug in 
INT_MULTIPLY_WRAPV? in INT_ADD_WRAPV? What code is generated and why is it 
wrong? That sort of thing. If there's a bug in INT_*_WRAPV I'd like to fix it.

An advantage to using 'long long' over 'time_t' is that insulates the code from 
possible future changes where time_t is unsigned (which POSIX allows). 
Admittedly this is unlikely.
  

On Wed, Jul 15, 2020 at 12:38 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 7/14/20 6:17 AM, H.J. Lu via Libc-alpha wrote:
> > No, it doesn't work on i686.
>
> Could you give more details about why it doesn't work? Is it a GCC bug (and if
> so, which version and what flags did you use)? Is it a bug in
> INT_MULTIPLY_WRAPV? in INT_ADD_WRAPV? What code is generated and why is it
> wrong? That sort of thing. If there's a bug in INT_*_WRAPV I'd like to fix it.

Test failed on i686.  Ping me in a couple weeks when I have some spare cycles.

> An advantage to using 'long long' over 'time_t' is that insulates the code from
> possible future changes where time_t is unsigned (which POSIX allows).
> Admittedly this is unlikely.