Re: things I don't understand in src/env/time.c + bugs + suggested patch

On 01/28/2017 01:31 PM, David Monniaux wrote:
> Over glpk-4.61
> 
> src/env/time.c contains:
> 
> double glp_time(void)
> {     struct timeval tv;
>       struct tm *tm;
>       int j;
>       double t;
>       gettimeofday(&tv, NULL);
>       tm = gmtime(&tv.tv_sec);
>       j = jday(tm->tm_mday, tm->tm_mon + 1, 1900 + tm->tm_year);
>       xassert(j >= 0);
>       t = ((((double)(j - EPOCH) * 24.0 + (double)tm->tm_hour) * 60.0 +
>          (double)tm->tm_min) * 60.0 + (double)tm->tm_sec) * 1000.0 +
>          (double)(tv.tv_usec / 1000);
>       return t;
> }
> 
> This code suffers from several problems:
> - The division tv.tv_usec / 1000 is integer division, then converted to
> double; why not simply multiply by 1E-3, which is more precise?
> - It uses gmtime, which is not reentrant (as opposed to gmtime_r). In
> fact, my attention was called to this function because of error reports
> by Valgrind's DRD race condition analyser ran on code using glpk from
> different threads.

It is a shame that glibc does not define _tmbuf as thread local memory.

gmtime_r is not C standard but POSIX specific.
The relevant C11 function is gmtime_s.

It should be supported with GCC 4.7 and later.
So probably we should check for gmtime_s in configure.ac.

> 
> I also do not understand why it is useful to use such a function,
> calling jday() to convert Julian days, whereas the following returns the
> same result:
> 
> double glp_time(void)
> {     struct timeval tv;
>        gettimeofday(&tv, NULL);
>       return tv.tv_sec + 1E3 * tv.tv_usec * 1E-3;

1E3 * 1E-3 = 1.
You wouldn't add seconds and microseconds without scaling.

tv_sec is counting from 0 to 59 (or 60 if you have leap second).
Why do you want to return the same values for 11:01:00 and 11:02:00?

> }
> 
> What am I missing?
> 
> If I'm missing nothing, this patch simplifies the code:
> 
> --- glpk-4.61-orig/src/env/time.c    2017-01-22 08:00:00.000000000 +0100
> +++ glpk-4.61-time-patched/src/env/time.c    2017-01-28
> 13:09:07.367464607 +0100
> @@ -53,17 +53,8 @@
>  
>  double glp_time(void)
>  {     struct timeval tv;
> -      struct tm *tm;
> -      int j;
> -      double t;
>        gettimeofday(&tv, NULL);
> -      tm = gmtime(&tv.tv_sec);
> -      j = jday(tm->tm_mday, tm->tm_mon + 1, 1900 + tm->tm_year);
> -      xassert(j >= 0);
> -      t = ((((double)(j - EPOCH) * 24.0 + (double)tm->tm_hour) * 60.0 +
> -         (double)tm->tm_min) * 60.0 + (double)tm->tm_sec) * 1000.0 +
> -         (double)(tv.tv_usec / 1000);
> -      return t;
> +      return tv.tv_sec + 1E3 * tv.tv_usec * 1E-3;
>  }
>  
>  /* MS Windows version *************************************************/
> @@ -92,17 +83,7 @@
>  #include <time.h>
>  
>  double glp_time(void)
> -{     time_t timer;
> -      struct tm *tm;
> -      int j;
> -      double t;
> -      timer = time(NULL);
> -      tm = gmtime(&timer);
> -      j = jday(tm->tm_mday, tm->tm_mon + 1, 1900 + tm->tm_year);
> -      xassert(j >= 0);
> -      t = ((((double)(j - EPOCH) * 24.0 + (double)tm->tm_hour) * 60.0 +
> -         (double)tm->tm_min) * 60.0 + (double)tm->tm_sec) * 1000.0;
> -      return t;
> +{      return time(NULL) * 1E3;
>  }
>  
>  #endif
> 
> Regards
> 
> 
> 
> _______________________________________________
> Help-glpk mailing list
> [email protected]
> https://lists.gnu.org/mailman/listinfo/help-glpk
>