[Bug fortran/56919] New: [4.6/4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[Bug fortran/56919] New: [4.6/4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

             Bug #: 56919
           Summary: [4.6/4.7/4.8/4.9 Regression] Wrong result for
                    SYSTEM_CLOCK on Cygwin
    Classification: Unclassified
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: burnus@gcc.gnu.org
                CC: janus@gcc.gnu.org, jb@gcc.gnu.org


The regression has been caused by Janne's patch, Rev.
http://gcc.gnu.org/r169449 - which was checked in 2011-01-31  (= GCC 4.6)


See Angelo Graziosi's bug report at
http://gcc.gnu.org/ml/fortran/2013-04/msg00085.html


The problem is that Cygwin does not support CLOCK_MONOTONIC; using it will
return always return 0.

Note that Cygwin not only defines CLOCK_MONOTONIC but since Cygwin-1.7.6 also 
"_POSIX_MONOTONIC_CLOCK 200112L" [1]. Thus, _POSIX_MONOTONIC_CLOCK [2] can also
not be used.


Currently, libgfortran uses (cf.libgfortran/intrinsics/system_clock.c):


/* POSIX states that CLOCK_REALTIME must be present if clock_gettime
   is available, others are optional.  */
#ifdef CLOCK_MONOTONIC
#define GF_CLOCK_MONOTONIC CLOCK_MONOTONIC
#else
#define GF_CLOCK_MONOTONIC CLOCK_REALTIME
#endif

#ifdef HAVE_CLOCK_GETTIME
  err = clock_gettime (GF_CLOCK_MONOTONIC, &ts);
#else
#if defined(HAVE_CLOCK_GETTIME_LIBRT) && SUPPORTS_WEAK && GTHREAD_USE_WEAK
  if (weak_gettime)
    err = clock_gettime (GF_CLOCK_MONOTONIC, &ts);
#endif
  err = gf_gettime (secs, nanosecs);
#endif


And gf_gettime (time_1.h) uses: gettimeofday - or if not available:
"clock_gettime (CLOCK_REALTIME" - or a simple "time(NULL)".


Nick suggests to simply call gf_gettime unconditionally. [3]


[1] Cf. http://cygwin.com/ml/cygwin-announce/2010-08/msg00019.html ,
http://sourceware.org/ml/newlib/2010/msg00454.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap02.html
[3] http://gcc.gnu.org/ml/fortran/2013-04/msg00086.html

[Bug fortran/56919] [4.6/4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #1 from Tobias Burnus <burnus at gcc dot gnu.org> 2013-04-11 13:53:26 UTC ---
Seemingly, it works with Cygwin 1.7.17, and the upcoming Cygwin 1.7.18,
http://gcc.gnu.org/ml/fortran/2013-04/msg00089.html

[Bug fortran/56919] [4.6/4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[jb at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

--- Comment #2 from Janne Blomqvist <jb at gcc dot gnu.org> 2013-04-11 18:32:19 UTC ---
(In reply to comment #0)
> The problem is that Cygwin does not support CLOCK_MONOTONIC; using it will
> return always return 0.
> 
> Note that Cygwin not only defines CLOCK_MONOTONIC but since Cygwin-1.7.6 also 
> "_POSIX_MONOTONIC_CLOCK 200112L" [1]. Thus, _POSIX_MONOTONIC_CLOCK [2] can also
> not be used.

Aagh, one wonders what demented logic caused them to claim they support it and
then not actually do so in any kind of useful manner.. *sigh*

> Nick suggests to simply call gf_gettime unconditionally. [3]

I wonder what, if any, specific issues there actually are..

Anyways, clock_gettime works fine on Linux, and IMHO it would be an annoying
regression to replace the current high-resolution monotonic clock with a lower
resolution non-monotonic clock, particularly since with recent glibc it should
be available to all users and not only those who link in librt (e.g. implicitly
via -fopenmp).

I don't know how cygwin implements CLOCK_MONOTONIC, but anyway, AFAIK on
windows a robust monotonic clock is available via the GetTickCount (and on
Vista/2008 and up, GetTickCount64 as well) function. Thus I'd suggest that both
MINGW and CYGWIN version should use those functions in the implementation of
the SYSTEM_CLOCK intrinsic. 

An overview of various timing functions can be found e.g. at
http://www.python.org/dev/peps/pep-0418/


> [3] http://gcc.gnu.org/ml/fortran/2013-04/msg00086.html

[Bug fortran/56919] [4.6/4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> 2013-04-11 20:14:13 UTC ---
First, I think we made a thinko with the random_seed example at
  http://gcc.gnu.org/onlinedocs/gfortran/RANDOM_005fSEED.html
it uses (as does Angelo's code):

            CALL SYSTEM_CLOCK(COUNT=clock)
            seed = clock + 37 * (/ (i - 1, i = 1, n) /)
            CALL RANDOM_SEED(PUT = seed)

If clock_gettime uses the seconds since the epoch (or since system start),
everything is fine. However, under Cygwin (cf. comment 1) the time since
process start seems to be used. Result: The timing might be quite similar.

Actually, at least with the testing of Angelo,
http://gcc.gnu.org/ml/fortran/2013-04/msg00092.html +
http://gcc.gnu.org/ml/fortran/2013-04/msg00084.html, it even seems as if the
clock rate is that slow that one can do several system calls in no time (= 0
nanoseconds).

Also gettimeofday and clock_gettime(CLOCK_REALTIME,...) show the same number of
nanoseconds with Cygwin. (Contrary to Linux.)


Thus, we should really update the RANDOM_SEED example to use something
wall-time based.

 * * *

(In reply to comment #2)
> Aagh, one wonders what demented logic caused them to claim they support it and
> then not actually do so in any kind of useful manner.. *sigh*

If one looks at newlib, one finds in src/newlib/libc/include/time.h:

#define CLOCK_REALTIME (clockid_t)1
#define CLOCK_PROCESS_CPUTIME_ID (clockid_t)2
#define CLOCK_THREAD_CPUTIME_ID (clockid_t)3
#define CLOCK_MONOTONIC (clockid_t)4

And src/newlib/libc/sys/linux/clock_gettime.c does not handle CLOCK_MONOTONIC:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/newlib/libc/sys/linux/clock_gettime.c?rev=1.1&content-type=text/x-cvsweb-markup&cvsroot=src


If one looks at the code, that should return "-1" as status code, if
CLOCK_MONOTONIC has failed.

I think we should change system_clock to use:

/* Newlib defines CLOCK_MONOTONIC but doesn't support it. Thus, if
   clock_gettime failed, fall through to gf_gettime.  */
if (return_value == 0)
  return;
...

That way, all those embedded systems which use newlib will get a proper result.
(gfortran has embedded users, at least one user on VxWorks.)

* * *

> > Nick suggests to simply call gf_gettime unconditionally. [3]
> I wonder what, if any, specific issues there actually are...

I wonder as well - at least for newlib it is clear why it fails.


> I don't know how cygwin implements CLOCK_MONOTONIC

See:
http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/cygwin/times.cc?cvsroot=src
It uses QueryPerformanceCounter (cf. hires_ns::nsecs, called by clock_gettime).
I have not yet fully understand why it is 0.

Cf.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms644904%28v=vs.85%29.aspx

> but anyway, AFAIK on
> windows a robust monotonic clock is available via the GetTickCount (and on
> Vista/2008 and up, GetTickCount64 as well) function. Thus I'd suggest that
> both MINGW and CYGWIN version should use those functions in the
> implementation of the SYSTEM_CLOCK intrinsic.

Looking through the list of functions at the URL below, I think calling
QueryPerformanceCounter makes most sense - which is what Cygwin does:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms644900(v=vs.85).aspx

 * * *

In summary:

- Cygwin is probably okay - it just starts from 0 with the first call to
system_clock(monotonic,...) which is fine but different to Linux.

- Newlib's system_clock handling is completely broken, but as it returns -1,
one could fall through to gf_gettime

- gfortran's random_seed example is bad if the system_clock(monotonic,...)
behaves as with Cygwin - or everything else based on the timing of the process
instead of the system up time (like GetTickCount - with overflow issues) or
ticks since the epoch (as system_clock on Linux).

- There seems to be a design issue with system_clock according to Nick, but we
don't know which one it is.

[Bug fortran/56919] [4.6/4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

--- Comment #4 from Tobias Burnus <burnus at gcc dot gnu.org> 2013-04-11 21:18:43 UTC ---
(In reply to comment #3)
> In summary:
> - Cygwin is probably okay - it just starts from 0 with the first call to
> system_clock(monotonic,...) which is fine but different to Linux.

That has now been fixed, cf.
http://gcc.gnu.org/ml/fortran/2013-04/msg00097.html

> - Newlib's system_clock handling is completely broken, but as it returns -1,
> one could fall through to gf_gettime

Actually, it only has  _POSIX_MONOTONIC_CLOCK set on RTEMS+Cygwin, which
support it.  Thus, it probably makes to check:
  #if defined(_POSIX_MONOTONIC_CLOCK) && _POSIX_MONOTONIC_CLOCK >= 0
in addition to check only for MONOTONIC_CLOCK is required to be in time.h
according to POSIX.
Cf. http://gcc.gnu.org/ml/fortran/2013-04/msg00099.html

> - gfortran's random_seed example is bad if the system_clock(monotonic,...)
> behaves as with Cygwin - or everything else based on the timing of the process
> instead of the system up time (like GetTickCount - with overflow issues) or
> ticks since the epoch (as system_clock on Linux).

The question is what we do here? Leave it? Modify it? Or add a note?


(In reply to comment #2)
> > Nick suggests to simply call gf_gettime unconditionally. [3]
> I wonder what, if any, specific issues there actually are..

Seems to be just difficult to implement: It has to be monotonic and can it
cannot be based on clock ticks - unless they have a well-defined frequency.
Thus, one can either query the clock - or try to play tricks which does not
work well.

Examples: GetTickCount (overflows after 49.7 days), Cygwin (used the process
start time not, e.g., system start time as base) - and there are presumably
tons of other clever ways which are broken in one or the other way.

[Bug fortran/56919] [4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[jb at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

Janne Blomqvist <jb at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://gcc.gnu.org/ml/gcc-p
                   |                            |atches/2013-04/msg00835.htm
                   |                            |l

--- Comment #5 from Janne Blomqvist <jb at gcc dot gnu.org> 2013-04-12 18:49:24 UTC ---
Patch at http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00835.html

[Bug fortran/56919] [4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

--- Comment #6 from Tobias Burnus <burnus at gcc dot gnu.org> 2013-04-15 12:57:51 UTC ---
Author: jb
Date: Mon Apr 15 12:43:15 2013
New Revision: 197968

URL: http://gcc.gnu.org/viewcvs?rev=197968&root=gcc&view=rev
Log:
PR 56919 Improve SYSTEM_CLOCK intrinsic on Windows.

frontend ChangeLog:

2013-04-15  Janne Blomqvist  <jb@gcc.gnu.org>

    PR fortran/56919
    * intrinsics.texi (SYSTEM_CLOCK): Update documentation.


libgfortran ChangeLog:

2013-04-15  Janne Blomqvist  <jb@gcc.gnu.org>

    PR fortran/56919
    * intrinsics/time_1.h: Check __CYGWIN__ in addition to
    __MINGW32__.
    * intrinsics/system_clock.c (GF_CLOCK_MONOTONIC): Check
    _POSIX_MONOTONIC_CLOCK as well.
    (system_clock_4): Use GetTickCount on Windows.
    (system_clock_8): Use QueryPerformanceCounter and
    QueryPerformanceCounterFrequency on Windows.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/intrinsic.texi
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/intrinsics/system_clock.c
    trunk/libgfortran/intrinsics/time_1.h

Author: burnus
Date: Mon Apr 15 12:51:20 2013
New Revision: 197970

URL: http://gcc.gnu.org/viewcvs?rev=197970&root=gcc&view=rev
Log:
2013-04-15  Tobias Burnus  <burnus@net-b.de>

        * intrinsic.texi (SYSTEM_CLOCK): Recommend kind=8.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/intrinsic.texi

[Bug fortran/56919] [4.7/4.8/4.9 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4
   Target Milestone|---                         |4.7.4

[Bug fortran/56919] [4.7/4.8 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[jb at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

Janne Blomqvist <jb at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2013-05-15
           Assignee|unassigned at gcc dot gnu.org      |jb at gcc dot gnu.org
            Summary|[4.7/4.8/4.9 Regression]    |[4.7/4.8 Regression] Wrong
                   |Wrong result for            |result for SYSTEM_CLOCK on
                   |SYSTEM_CLOCK on Cygwin      |Cygwin
     Ever confirmed|0                           |1

--- Comment #7 from Janne Blomqvist <jb at gcc dot gnu.org> ---
Issue is fixed on trunk (4.9). 

The question is whether it's useful to backport to 4.7 and 4.8. The previous
cygwin behavior, while perhaps surprising, isn't wrong per se and while the new
behavior is an improvement (IMHO) it's a (subtle) change in behavior which
might be surprising in the middle of a release series.

Maybe just backport the _POSIX_MONOTONIC_CLOCK part to fix(?) newlib targets,
and leave the windows stuff as is?

[Bug fortran/56919] [4.7/4.8 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[jb at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

--- Comment #8 from Janne Blomqvist <jb at gcc dot gnu.org> ---
Author: jb
Date: Sat May  3 20:56:26 2014
New Revision: 210041

URL: http://gcc.gnu.org/viewcvs?rev=210041&root=gcc&view=rev
Log:
PR 56919 Check _POSIX_MONOTONIC_CLOCK.

2014-05-03  Janne Blomqvist  <jb@gcc.gnu.org>

    Backport from 4.9
    PR libfortran/56919
    * intrinsics/system_clock.c (GF_CLOCK_MONOTONIC): Check
    _POSIX_MONOTONIC_CLOCK as well.

Modified:
    branches/gcc-4_8-branch/libgfortran/ChangeLog
    branches/gcc-4_8-branch/libgfortran/intrinsics/system_clock.c

[Bug fortran/56919] [4.7/4.8 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[jb at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

--- Comment #9 from Janne Blomqvist <jb at gcc dot gnu.org> ---
Author: jb
Date: Sat May  3 21:03:10 2014
New Revision: 210042

URL: http://gcc.gnu.org/viewcvs?rev=210042&root=gcc&view=rev
Log:
PR 56919 Check _POSIX_MONOTONIC_CLOCK.

2014-05-04  Janne Blomqvist  <jb@gcc.gnu.org>

    Backport from 4.9
    PR libfortran/56919
    * intrinsics/system_clock.c (GF_CLOCK_MONOTONIC): Check
    _POSIX_MONOTONIC_CLOCK as well.

Modified:
    branches/gcc-4_7-branch/libgfortran/ChangeLog
    branches/gcc-4_7-branch/libgfortran/intrinsics/system_clock.c

[Bug fortran/56919] [4.7/4.8 Regression] Wrong result for SYSTEM_CLOCK on Cygwin

[jb at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919

Janne Blomqvist <jb at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |FIXED

--- Comment #10 from Janne Blomqvist <jb at gcc dot gnu.org> ---
Backported the _POSIX_MONOTONIC_CLOCK check to 4.7/4.8, closing as fixed.