* [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
@ 2017-02-27 11:15 Janne Blomqvist
2017-03-12 17:27 ` NightStrike
0 siblings, 1 reply; 8+ messages in thread
From: Janne Blomqvist @ 2017-02-27 11:15 UTC (permalink / raw)
To: GCC Patches, Fortran List
Don't try to use rand_s on CYGWIN
CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is
defined even though rand_s is not available. Thus add an extra check
for __CYGWIN__.
Thanks to Tim Prince and Nightstrike for bringing this issue to my attention.
Committed as r245755.
2017-02-27 Janne Blomqvist <jb@gcc.gnu.org>
* intrinsics/random.c (getosrandom): Don't try to use rand_s on
CYGWIN.
--
Janne Blomqvist
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
2017-02-27 11:15 [Patch, libgfortran, committed] Don't use rand_s on CYGWIN Janne Blomqvist
@ 2017-03-12 17:27 ` NightStrike
2017-03-12 20:11 ` NightStrike
2017-03-12 20:46 ` Janne Blomqvist
0 siblings, 2 replies; 8+ messages in thread
From: NightStrike @ 2017-03-12 17:27 UTC (permalink / raw)
To: Janne Blomqvist; +Cc: GCC Patches, Fortran List, JonY
On Mon, Feb 27, 2017 at 6:15 AM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> Don't try to use rand_s on CYGWIN
>
> CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is
> defined even though rand_s is not available. Thus add an extra check
> for __CYGWIN__.
>
> Thanks to Tim Prince and Nightstrike for bringing this issue to my attention.
>
> Committed as r245755.
>
> 2017-02-27 Janne Blomqvist <jb@gcc.gnu.org>
>
> * intrinsics/random.c (getosrandom): Don't try to use rand_s on
> CYGWIN.
1) There was no patch attached to the email.
2) As mentioned on IRC, I don't think this is the right fix. The real
problem is that time_1.h includes windows.h on CYGWIN, which it
shouldn't be doing. This then pollutes the translation unit with all
sorts of MINGW-isms that aren't exactly appropriate for cygwin.
Removing the include in time_1.h and then adjusting a few ifdefs in
system_clock.c that also had the same bug fixes the problem. The
testsuite is running right now on this.
See the following diff for reference:
Index: ../gccsvn/libgfortran/intrinsics/random.c
===================================================================
--- ../gccsvn/libgfortran/intrinsics/random.c (revision 246075)
+++ ../gccsvn/libgfortran/intrinsics/random.c (working copy)
@@ -304,7 +304,7 @@
getosrandom (void *buf, size_t buflen)
{
/* rand_s is available in MinGW-w64 but not plain MinGW. */
-#if defined(__MINGW64_VERSION_MAJOR) && !defined(__CYGWIN__)
+#ifdef __MINGW64_VERSION_MAJOR
unsigned int* b = buf;
for (unsigned i = 0; i < buflen / sizeof (unsigned int); i++)
rand_s (&b[i]);
Index: ../gccsvn/libgfortran/intrinsics/system_clock.c
===================================================================
--- ../gccsvn/libgfortran/intrinsics/system_clock.c (revision 246075)
+++ ../gccsvn/libgfortran/intrinsics/system_clock.c (working copy)
@@ -29,7 +29,7 @@
#include "time_1.h"
-#if !defined(__MINGW32__) && !defined(__CYGWIN__)
+#if !defined(__MINGW32__)
/* POSIX states that CLOCK_REALTIME must be present if clock_gettime
is available, others are optional. */
@@ -121,7 +121,7 @@
system_clock_4 (GFC_INTEGER_4 *count, GFC_INTEGER_4 *count_rate,
GFC_INTEGER_4 *count_max)
{
-#if defined(__MINGW32__) || defined(__CYGWIN__)
+#if defined(__MINGW32__)
if (count)
{
/* Use GetTickCount here as the resolution and range is
@@ -174,7 +174,7 @@
system_clock_8 (GFC_INTEGER_8 *count, GFC_INTEGER_8 *count_rate,
GFC_INTEGER_8 *count_max)
{
-#if defined(__MINGW32__) || defined(__CYGWIN__)
+#if defined(__MINGW32__)
LARGE_INTEGER cnt;
LARGE_INTEGER freq;
bool fail = false;
Index: ../gccsvn/libgfortran/intrinsics/time_1.h
===================================================================
--- ../gccsvn/libgfortran/intrinsics/time_1.h (revision 246075)
+++ ../gccsvn/libgfortran/intrinsics/time_1.h (working copy)
@@ -101,7 +101,7 @@
CPU_TIME intrinsics. Returns 0 for success or -1 if no
CPU time could be computed. */
-#if defined(__MINGW32__) || defined(__CYGWIN__)
+#if defined(__MINGW32__)
#define WIN32_LEAN_AND_MEAN
#include <windows.h>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
2017-03-12 17:27 ` NightStrike
@ 2017-03-12 20:11 ` NightStrike
2017-03-12 20:46 ` Janne Blomqvist
1 sibling, 0 replies; 8+ messages in thread
From: NightStrike @ 2017-03-12 20:11 UTC (permalink / raw)
To: Janne Blomqvist; +Cc: GCC Patches, Fortran List, JonY
On Sun, Mar 12, 2017 at 1:26 PM, NightStrike <nightstrike@gmail.com> wrote:
> On Mon, Feb 27, 2017 at 6:15 AM, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> Don't try to use rand_s on CYGWIN
>>
>> CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is
>> defined even though rand_s is not available. Thus add an extra check
>> for __CYGWIN__.
>>
>> Thanks to Tim Prince and Nightstrike for bringing this issue to my attention.
>>
>> Committed as r245755.
>>
>> 2017-02-27 Janne Blomqvist <jb@gcc.gnu.org>
>>
>> * intrinsics/random.c (getosrandom): Don't try to use rand_s on
>> CYGWIN.
>
> 1) There was no patch attached to the email.
>
> 2) As mentioned on IRC, I don't think this is the right fix. The real
> problem is that time_1.h includes windows.h on CYGWIN, which it
> shouldn't be doing. This then pollutes the translation unit with all
> sorts of MINGW-isms that aren't exactly appropriate for cygwin.
> Removing the include in time_1.h and then adjusting a few ifdefs in
> system_clock.c that also had the same bug fixes the problem. The
> testsuite is running right now on this.
Testsuite run completed with no change in results:
=== gfortran Summary ===
# of expected passes 44633
# of unexpected failures 55
# of unexpected successes 6
# of expected failures 85
# of unsupported tests 187
/tmp/build/gcc/testsuite/gfortran/../../gfortran version 7.0.1
20170312 (experimental) (GCC)
make[1]: Leaving directory '/tmp/build/gcc'
Those 55 unexpected failures are unrelated to this change, and occur
before and after.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
2017-03-12 17:27 ` NightStrike
2017-03-12 20:11 ` NightStrike
@ 2017-03-12 20:46 ` Janne Blomqvist
2017-03-13 9:01 ` Janne Blomqvist
1 sibling, 1 reply; 8+ messages in thread
From: Janne Blomqvist @ 2017-03-12 20:46 UTC (permalink / raw)
To: NightStrike; +Cc: GCC Patches, Fortran List, JonY
On Sun, Mar 12, 2017 at 7:26 PM, NightStrike <nightstrike@gmail.com> wrote:
> On Mon, Feb 27, 2017 at 6:15 AM, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> Don't try to use rand_s on CYGWIN
>>
>> CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is
>> defined even though rand_s is not available. Thus add an extra check
>> for __CYGWIN__.
>>
>> Thanks to Tim Prince and Nightstrike for bringing this issue to my attention.
>>
>> Committed as r245755.
>>
>> 2017-02-27 Janne Blomqvist <jb@gcc.gnu.org>
>>
>> * intrinsics/random.c (getosrandom): Don't try to use rand_s on
>> CYGWIN.
>
> 1) There was no patch attached to the email.
Oh, sorry. Well, you can see it at
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=245755
> 2) As mentioned on IRC, I don't think this is the right fix. The real
> problem is that time_1.h includes windows.h on CYGWIN, which it
> shouldn't be doing. This then pollutes the translation unit with all
> sorts of MINGW-isms that aren't exactly appropriate for cygwin.
> Removing the include in time_1.h and then adjusting a few ifdefs in
> system_clock.c that also had the same bug fixes the problem. The
> testsuite is running right now on this.
It used to be something like that, but IIRC there were some complaints
about SYSTEM_CLOCK on cygwin that were due to the cygwin
clock_gettime() or something like that, and after some discussion with
someone who knows something about cygwin/mingw (since you apparently
have no memory of it, I guess it was Kai), it was decided to use
GetTickCount & QPC also on cygwin.
--
Janne Blomqvist
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
2017-03-12 20:46 ` Janne Blomqvist
@ 2017-03-13 9:01 ` Janne Blomqvist
2017-03-14 4:32 ` NightStrike
0 siblings, 1 reply; 8+ messages in thread
From: Janne Blomqvist @ 2017-03-13 9:01 UTC (permalink / raw)
To: NightStrike; +Cc: GCC Patches, Fortran List, JonY
On Sun, Mar 12, 2017 at 10:46 PM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Sun, Mar 12, 2017 at 7:26 PM, NightStrike <nightstrike@gmail.com> wrote:
>> On Mon, Feb 27, 2017 at 6:15 AM, Janne Blomqvist
>> <blomqvist.janne@gmail.com> wrote:
>>> Don't try to use rand_s on CYGWIN
>>>
>>> CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is
>>> defined even though rand_s is not available. Thus add an extra check
>>> for __CYGWIN__.
>>>
>>> Thanks to Tim Prince and Nightstrike for bringing this issue to my attention.
>>>
>>> Committed as r245755.
>>>
>>> 2017-02-27 Janne Blomqvist <jb@gcc.gnu.org>
>>>
>>> * intrinsics/random.c (getosrandom): Don't try to use rand_s on
>>> CYGWIN.
>>
>> 1) There was no patch attached to the email.
>
> Oh, sorry. Well, you can see it at
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=245755
>
>> 2) As mentioned on IRC, I don't think this is the right fix. The real
>> problem is that time_1.h includes windows.h on CYGWIN, which it
>> shouldn't be doing. This then pollutes the translation unit with all
>> sorts of MINGW-isms that aren't exactly appropriate for cygwin.
>> Removing the include in time_1.h and then adjusting a few ifdefs in
>> system_clock.c that also had the same bug fixes the problem. The
>> testsuite is running right now on this.
>
> It used to be something like that, but IIRC there were some complaints
> about SYSTEM_CLOCK on cygwin that were due to the cygwin
> clock_gettime() or something like that, and after some discussion with
> someone who knows something about cygwin/mingw (since you apparently
> have no memory of it, I guess it was Kai), it was decided to use
> GetTickCount & QPC also on cygwin.
I searched a bit, and it seems the culprit is the thread starting at
https://gcc.gnu.org/ml/fortran/2013-04/msg00033.html
So it turned out that clock_gettime(CLOCK_MONOTONIC, ...) always
returned 0 on cygwin, hence the code was changed to use the windows
API functions GetTickCount and QPC also on cygwin at
https://gcc.gnu.org/ml/fortran/2013-04/msg00124.html
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919
--
Janne Blomqvist
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
2017-03-13 9:01 ` Janne Blomqvist
@ 2017-03-14 4:32 ` NightStrike
2017-03-14 8:05 ` Janne Blomqvist
0 siblings, 1 reply; 8+ messages in thread
From: NightStrike @ 2017-03-14 4:32 UTC (permalink / raw)
To: Janne Blomqvist; +Cc: GCC Patches, Fortran List, JonY
On Mon, Mar 13, 2017 at 5:01 AM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Sun, Mar 12, 2017 at 10:46 PM, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> On Sun, Mar 12, 2017 at 7:26 PM, NightStrike <nightstrike@gmail.com> wrote:
>>> On Mon, Feb 27, 2017 at 6:15 AM, Janne Blomqvist
>>> <blomqvist.janne@gmail.com> wrote:
>>>> Don't try to use rand_s on CYGWIN
>>>>
>>>> CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is
>>>> defined even though rand_s is not available. Thus add an extra check
>>>> for __CYGWIN__.
>>>>
>>>> Thanks to Tim Prince and Nightstrike for bringing this issue to my attention.
>>>>
>>>> Committed as r245755.
>>>>
>>>> 2017-02-27 Janne Blomqvist <jb@gcc.gnu.org>
>>>>
>>>> * intrinsics/random.c (getosrandom): Don't try to use rand_s on
>>>> CYGWIN.
>>>
>>> 1) There was no patch attached to the email.
>>
>> Oh, sorry. Well, you can see it at
>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=245755
Thanks. You know, this is actually better than attaching a patch (as
a general thing for emails sent after things are already committed),
as there can be no difference between what was emailed and what was
committed.
>>> 2) As mentioned on IRC, I don't think this is the right fix. The real
>>> problem is that time_1.h includes windows.h on CYGWIN, which it
>>> shouldn't be doing. This then pollutes the translation unit with all
>>> sorts of MINGW-isms that aren't exactly appropriate for cygwin.
>>> Removing the include in time_1.h and then adjusting a few ifdefs in
>>> system_clock.c that also had the same bug fixes the problem. The
>>> testsuite is running right now on this.
>>
>> It used to be something like that, but IIRC there were some complaints
>> about SYSTEM_CLOCK on cygwin that were due to the cygwin
>> clock_gettime() or something like that, and after some discussion with
>> someone who knows something about cygwin/mingw (since you apparently
>> have no memory of it, I guess it was Kai), it was decided to use
>> GetTickCount & QPC also on cygwin.
>
> I searched a bit, and it seems the culprit is the thread starting at
>
> https://gcc.gnu.org/ml/fortran/2013-04/msg00033.html
>
> So it turned out that clock_gettime(CLOCK_MONOTONIC, ...) always
> returned 0 on cygwin, hence the code was changed to use the windows
> API functions GetTickCount and QPC also on cygwin at
>
> https://gcc.gnu.org/ml/fortran/2013-04/msg00124.html
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919
That all led me to this thread:
https://cygwin.com/ml/cygwin/2013-04/msg00184.html
which led me to:
https://cygwin.com/ml/cygwin/2013-04/msg00191.html
where Corinna fixed Angelo's original issue that sparked the whole
thing. So, from my perspective, Cygwin hasn't had this problem in
about 4 years.
To be complete, though, I took Angelo's original code and compiled it
with a GCC built with the change I suggested, and I received this:
$ ./a.exe
9.50646996E-02 0.435180306 0.939791977 0.851783216
0.308901191 0.447312355 0.766363919 0.163527727
1.25432014E-02
$ ./a.exe
0.445786893 9.30446386E-03 0.880381405 0.123394549
1.23693347E-02 0.485788047 0.623361290 0.921140671
0.119319797
$ ./a.exe
0.378171027 0.439836979 0.440582573 1.17767453E-02
0.427448869 0.530438244 0.182108700 0.147965968
0.668991745
$ ./a.exe
2.73125172E-02 0.916011810 0.854288757 0.913502872
0.508077919 0.210798383 8.76839161E-02 0.120695710
0.337186754
I then tried Janus' enhanced version from
https://gcc.gnu.org/ml/fortran/2013-04/msg00034.html
$ ./a.exe
n= 33
clock: 744091787
seed: 744091787 744091824 744091861 744091898 744091935
744091972 744092009 744092046 744092083 744092120 744092157
744092194 744092231 744092268 744092305 744092342 744092379
744092416 744092453 744092490 744092527 744092564
744092601 744092638 744092675 744092712 744092749 744092786
744092823 744092860 744092897 744092934 744092971
random: 0.801897824 0.180594921 0.280960143
8.65536928E-03 0.122029424 0.473693788 0.161536098
0.228073180 0.441518903
$ ./a.exe
n= 33
clock: 744093409
seed: 744093409 744093446 744093483 744093520 744093557
744093594 744093631 744093668 744093705 744093742 744093779
744093816 744093853 744093890 744093927 744093964 744094001
744094038 744094075 744094112 744094149 744094186
744094223 744094260 744094297 744094334 744094371 744094408
744094445 744094482 744094519 744094556 744094593
random: 0.164107382 0.762304962 0.511664748
0.700617492 0.692375600 0.207925439 0.920203805
0.160881400 0.339902878
$ ./a.exe
n= 33
clock: 744094930
seed: 744094930 744094967 744095004 744095041 744095078
744095115 744095152 744095189 744095226 744095263 744095300
744095337 744095374 744095411 744095448 744095485 744095522
744095559 744095596 744095633 744095670 744095707
744095744 744095781 744095818 744095855 744095892 744095929
744095966 744096003 744096040 744096077 744096114
random: 0.433895171 0.989695787 0.305223107
0.387590647 0.673205614 0.539944470 0.849159062
0.811356246 0.888609290
$ ./a.exe
n= 33
clock: 744096561
seed: 744096561 744096598 744096635 744096672 744096709
744096746 744096783 744096820 744096857 744096894 744096931
744096968 744097005 744097042 744097079 744097116 744097153
744097190 744097227 744097264 744097301 744097338
744097375 744097412 744097449 744097486 744097523 744097560
744097597 744097634 744097671 744097708 744097745
random: 0.224010825 0.763803065 0.111726880
0.500481725 6.07219338E-02 0.413555145 0.896298766
0.142876744 0.286089420
And finally, the output of
https://gcc.gnu.org/ml/fortran/2013-04/msg00038.html which you
requested in https://gcc.gnu.org/ml/fortran/2013-04/msg00207.html as a
test for the original fix:
$ ./a.exe
744248371 1000 2147483647
744248371346087 1000000000 9223372036854775807
$ ./a.exe
744249100 1000 2147483647
744249100677540 1000000000 9223372036854775807
$ ./a.exe
744249678 1000 2147483647
744249678546099 1000000000 9223372036854775807
$ ./a.exe
744250116 1000 2147483647
744250116491405 1000000000 9223372036854775807
So............ I know this was a long email, but it seems to me that
the better upstream fix went in a few weeks before these other
libgfortran changes, and they allow for a libgfortran that's untainted
by windows.h. I really think this is the better, safer approach that
will prevent future errors, and remove the need for CYGWIN macro
checks in multiple places.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
2017-03-14 4:32 ` NightStrike
@ 2017-03-14 8:05 ` Janne Blomqvist
2017-03-15 13:49 ` Janne Blomqvist
0 siblings, 1 reply; 8+ messages in thread
From: Janne Blomqvist @ 2017-03-14 8:05 UTC (permalink / raw)
To: NightStrike; +Cc: GCC Patches, Fortran List, JonY
On Tue, Mar 14, 2017 at 6:32 AM, NightStrike <nightstrike@gmail.com> wrote:
> On Mon, Mar 13, 2017 at 5:01 AM, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> On Sun, Mar 12, 2017 at 10:46 PM, Janne Blomqvist
>> <blomqvist.janne@gmail.com> wrote:
>>> On Sun, Mar 12, 2017 at 7:26 PM, NightStrike <nightstrike@gmail.com> wrote:
>>>> On Mon, Feb 27, 2017 at 6:15 AM, Janne Blomqvist
>>>> <blomqvist.janne@gmail.com> wrote:
>>>>> Don't try to use rand_s on CYGWIN
>>>>>
>>>>> CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is
>>>>> defined even though rand_s is not available. Thus add an extra check
>>>>> for __CYGWIN__.
>>>>>
>>>>> Thanks to Tim Prince and Nightstrike for bringing this issue to my attention.
>>>>>
>>>>> Committed as r245755.
>>>>>
>>>>> 2017-02-27 Janne Blomqvist <jb@gcc.gnu.org>
>>>>>
>>>>> * intrinsics/random.c (getosrandom): Don't try to use rand_s on
>>>>> CYGWIN.
>>>>
>>>> 1) There was no patch attached to the email.
>>>
>>> Oh, sorry. Well, you can see it at
>>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=245755
>
> Thanks. You know, this is actually better than attaching a patch (as
> a general thing for emails sent after things are already committed),
> as there can be no difference between what was emailed and what was
> committed.
>
>>>> 2) As mentioned on IRC, I don't think this is the right fix. The real
>>>> problem is that time_1.h includes windows.h on CYGWIN, which it
>>>> shouldn't be doing. This then pollutes the translation unit with all
>>>> sorts of MINGW-isms that aren't exactly appropriate for cygwin.
>>>> Removing the include in time_1.h and then adjusting a few ifdefs in
>>>> system_clock.c that also had the same bug fixes the problem. The
>>>> testsuite is running right now on this.
>>>
>>> It used to be something like that, but IIRC there were some complaints
>>> about SYSTEM_CLOCK on cygwin that were due to the cygwin
>>> clock_gettime() or something like that, and after some discussion with
>>> someone who knows something about cygwin/mingw (since you apparently
>>> have no memory of it, I guess it was Kai), it was decided to use
>>> GetTickCount & QPC also on cygwin.
>>
>> I searched a bit, and it seems the culprit is the thread starting at
>>
>> https://gcc.gnu.org/ml/fortran/2013-04/msg00033.html
>>
>> So it turned out that clock_gettime(CLOCK_MONOTONIC, ...) always
>> returned 0 on cygwin, hence the code was changed to use the windows
>> API functions GetTickCount and QPC also on cygwin at
>>
>> https://gcc.gnu.org/ml/fortran/2013-04/msg00124.html
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919
>
> That all led me to this thread:
>
> https://cygwin.com/ml/cygwin/2013-04/msg00184.html
>
> which led me to:
>
> https://cygwin.com/ml/cygwin/2013-04/msg00191.html
>
> where Corinna fixed Angelo's original issue that sparked the whole
> thing. So, from my perspective, Cygwin hasn't had this problem in
> about 4 years.
>
> To be complete, though, I took Angelo's original code and compiled it
> with a GCC built with the change I suggested, and I received this:
>
> $ ./a.exe
> 9.50646996E-02 0.435180306 0.939791977 0.851783216
> 0.308901191 0.447312355 0.766363919 0.163527727
> 1.25432014E-02
>
> $ ./a.exe
> 0.445786893 9.30446386E-03 0.880381405 0.123394549
> 1.23693347E-02 0.485788047 0.623361290 0.921140671
> 0.119319797
>
> $ ./a.exe
> 0.378171027 0.439836979 0.440582573 1.17767453E-02
> 0.427448869 0.530438244 0.182108700 0.147965968
> 0.668991745
>
> $ ./a.exe
> 2.73125172E-02 0.916011810 0.854288757 0.913502872
> 0.508077919 0.210798383 8.76839161E-02 0.120695710
> 0.337186754
>
>
> I then tried Janus' enhanced version from
> https://gcc.gnu.org/ml/fortran/2013-04/msg00034.html
>
> $ ./a.exe
> n= 33
> clock: 744091787
> seed: 744091787 744091824 744091861 744091898 744091935
> 744091972 744092009 744092046 744092083 744092120 744092157
> 744092194 744092231 744092268 744092305 744092342 744092379
> 744092416 744092453 744092490 744092527 744092564
> 744092601 744092638 744092675 744092712 744092749 744092786
> 744092823 744092860 744092897 744092934 744092971
> random: 0.801897824 0.180594921 0.280960143
> 8.65536928E-03 0.122029424 0.473693788 0.161536098
> 0.228073180 0.441518903
>
> $ ./a.exe
> n= 33
> clock: 744093409
> seed: 744093409 744093446 744093483 744093520 744093557
> 744093594 744093631 744093668 744093705 744093742 744093779
> 744093816 744093853 744093890 744093927 744093964 744094001
> 744094038 744094075 744094112 744094149 744094186
> 744094223 744094260 744094297 744094334 744094371 744094408
> 744094445 744094482 744094519 744094556 744094593
> random: 0.164107382 0.762304962 0.511664748
> 0.700617492 0.692375600 0.207925439 0.920203805
> 0.160881400 0.339902878
>
> $ ./a.exe
> n= 33
> clock: 744094930
> seed: 744094930 744094967 744095004 744095041 744095078
> 744095115 744095152 744095189 744095226 744095263 744095300
> 744095337 744095374 744095411 744095448 744095485 744095522
> 744095559 744095596 744095633 744095670 744095707
> 744095744 744095781 744095818 744095855 744095892 744095929
> 744095966 744096003 744096040 744096077 744096114
> random: 0.433895171 0.989695787 0.305223107
> 0.387590647 0.673205614 0.539944470 0.849159062
> 0.811356246 0.888609290
>
> $ ./a.exe
> n= 33
> clock: 744096561
> seed: 744096561 744096598 744096635 744096672 744096709
> 744096746 744096783 744096820 744096857 744096894 744096931
> 744096968 744097005 744097042 744097079 744097116 744097153
> 744097190 744097227 744097264 744097301 744097338
> 744097375 744097412 744097449 744097486 744097523 744097560
> 744097597 744097634 744097671 744097708 744097745
> random: 0.224010825 0.763803065 0.111726880
> 0.500481725 6.07219338E-02 0.413555145 0.896298766
> 0.142876744 0.286089420
>
>
> And finally, the output of
> https://gcc.gnu.org/ml/fortran/2013-04/msg00038.html which you
> requested in https://gcc.gnu.org/ml/fortran/2013-04/msg00207.html as a
> test for the original fix:
>
> $ ./a.exe
> 744248371 1000 2147483647
> 744248371346087 1000000000 9223372036854775807
>
> $ ./a.exe
> 744249100 1000 2147483647
> 744249100677540 1000000000 9223372036854775807
>
> $ ./a.exe
> 744249678 1000 2147483647
> 744249678546099 1000000000 9223372036854775807
>
> $ ./a.exe
> 744250116 1000 2147483647
> 744250116491405 1000000000 9223372036854775807
>
>
>
> So............ I know this was a long email, but it seems to me that
> the better upstream fix went in a few weeks before these other
> libgfortran changes, and they allow for a libgfortran that's untainted
> by windows.h. I really think this is the better, safer approach that
> will prevent future errors, and remove the need for CYGWIN macro
> checks in multiple places.
Ok for trunk.
You said on IRC that cygwin is a rolling release system and they don't
support old releases, and the bug that prompted this was fixed 4 years
ago, so I agree we don't need to support that anymore.
--
Janne Blomqvist
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
2017-03-14 8:05 ` Janne Blomqvist
@ 2017-03-15 13:49 ` Janne Blomqvist
0 siblings, 0 replies; 8+ messages in thread
From: Janne Blomqvist @ 2017-03-15 13:49 UTC (permalink / raw)
To: NightStrike; +Cc: GCC Patches, Fortran List, JonY
On Tue, Mar 14, 2017 at 10:05 AM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Tue, Mar 14, 2017 at 6:32 AM, NightStrike <nightstrike@gmail.com> wrote:
>> On Mon, Mar 13, 2017 at 5:01 AM, Janne Blomqvist
>> <blomqvist.janne@gmail.com> wrote:
>>> On Sun, Mar 12, 2017 at 10:46 PM, Janne Blomqvist
>>> <blomqvist.janne@gmail.com> wrote:
>>>> On Sun, Mar 12, 2017 at 7:26 PM, NightStrike <nightstrike@gmail.com> wrote:
>>>>> On Mon, Feb 27, 2017 at 6:15 AM, Janne Blomqvist
>>>>> <blomqvist.janne@gmail.com> wrote:
>>>>>> Don't try to use rand_s on CYGWIN
>>>>>>
>>>>>> CYGWIN seems to include _mingw.h and thus __MINGW64_VERSION_MAJOR is
>>>>>> defined even though rand_s is not available. Thus add an extra check
>>>>>> for __CYGWIN__.
>>>>>>
>>>>>> Thanks to Tim Prince and Nightstrike for bringing this issue to my attention.
>>>>>>
>>>>>> Committed as r245755.
>>>>>>
>>>>>> 2017-02-27 Janne Blomqvist <jb@gcc.gnu.org>
>>>>>>
>>>>>> * intrinsics/random.c (getosrandom): Don't try to use rand_s on
>>>>>> CYGWIN.
>>>>>
>>>>> 1) There was no patch attached to the email.
>>>>
>>>> Oh, sorry. Well, you can see it at
>>>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=245755
>>
>> Thanks. You know, this is actually better than attaching a patch (as
>> a general thing for emails sent after things are already committed),
>> as there can be no difference between what was emailed and what was
>> committed.
>>
>>>>> 2) As mentioned on IRC, I don't think this is the right fix. The real
>>>>> problem is that time_1.h includes windows.h on CYGWIN, which it
>>>>> shouldn't be doing. This then pollutes the translation unit with all
>>>>> sorts of MINGW-isms that aren't exactly appropriate for cygwin.
>>>>> Removing the include in time_1.h and then adjusting a few ifdefs in
>>>>> system_clock.c that also had the same bug fixes the problem. The
>>>>> testsuite is running right now on this.
>>>>
>>>> It used to be something like that, but IIRC there were some complaints
>>>> about SYSTEM_CLOCK on cygwin that were due to the cygwin
>>>> clock_gettime() or something like that, and after some discussion with
>>>> someone who knows something about cygwin/mingw (since you apparently
>>>> have no memory of it, I guess it was Kai), it was decided to use
>>>> GetTickCount & QPC also on cygwin.
>>>
>>> I searched a bit, and it seems the culprit is the thread starting at
>>>
>>> https://gcc.gnu.org/ml/fortran/2013-04/msg00033.html
>>>
>>> So it turned out that clock_gettime(CLOCK_MONOTONIC, ...) always
>>> returned 0 on cygwin, hence the code was changed to use the windows
>>> API functions GetTickCount and QPC also on cygwin at
>>>
>>> https://gcc.gnu.org/ml/fortran/2013-04/msg00124.html
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56919
>>
>> That all led me to this thread:
>>
>> https://cygwin.com/ml/cygwin/2013-04/msg00184.html
>>
>> which led me to:
>>
>> https://cygwin.com/ml/cygwin/2013-04/msg00191.html
>>
>> where Corinna fixed Angelo's original issue that sparked the whole
>> thing. So, from my perspective, Cygwin hasn't had this problem in
>> about 4 years.
>>
>> To be complete, though, I took Angelo's original code and compiled it
>> with a GCC built with the change I suggested, and I received this:
>>
>> $ ./a.exe
>> 9.50646996E-02 0.435180306 0.939791977 0.851783216
>> 0.308901191 0.447312355 0.766363919 0.163527727
>> 1.25432014E-02
>>
>> $ ./a.exe
>> 0.445786893 9.30446386E-03 0.880381405 0.123394549
>> 1.23693347E-02 0.485788047 0.623361290 0.921140671
>> 0.119319797
>>
>> $ ./a.exe
>> 0.378171027 0.439836979 0.440582573 1.17767453E-02
>> 0.427448869 0.530438244 0.182108700 0.147965968
>> 0.668991745
>>
>> $ ./a.exe
>> 2.73125172E-02 0.916011810 0.854288757 0.913502872
>> 0.508077919 0.210798383 8.76839161E-02 0.120695710
>> 0.337186754
>>
>>
>> I then tried Janus' enhanced version from
>> https://gcc.gnu.org/ml/fortran/2013-04/msg00034.html
>>
>> $ ./a.exe
>> n= 33
>> clock: 744091787
>> seed: 744091787 744091824 744091861 744091898 744091935
>> 744091972 744092009 744092046 744092083 744092120 744092157
>> 744092194 744092231 744092268 744092305 744092342 744092379
>> 744092416 744092453 744092490 744092527 744092564
>> 744092601 744092638 744092675 744092712 744092749 744092786
>> 744092823 744092860 744092897 744092934 744092971
>> random: 0.801897824 0.180594921 0.280960143
>> 8.65536928E-03 0.122029424 0.473693788 0.161536098
>> 0.228073180 0.441518903
>>
>> $ ./a.exe
>> n= 33
>> clock: 744093409
>> seed: 744093409 744093446 744093483 744093520 744093557
>> 744093594 744093631 744093668 744093705 744093742 744093779
>> 744093816 744093853 744093890 744093927 744093964 744094001
>> 744094038 744094075 744094112 744094149 744094186
>> 744094223 744094260 744094297 744094334 744094371 744094408
>> 744094445 744094482 744094519 744094556 744094593
>> random: 0.164107382 0.762304962 0.511664748
>> 0.700617492 0.692375600 0.207925439 0.920203805
>> 0.160881400 0.339902878
>>
>> $ ./a.exe
>> n= 33
>> clock: 744094930
>> seed: 744094930 744094967 744095004 744095041 744095078
>> 744095115 744095152 744095189 744095226 744095263 744095300
>> 744095337 744095374 744095411 744095448 744095485 744095522
>> 744095559 744095596 744095633 744095670 744095707
>> 744095744 744095781 744095818 744095855 744095892 744095929
>> 744095966 744096003 744096040 744096077 744096114
>> random: 0.433895171 0.989695787 0.305223107
>> 0.387590647 0.673205614 0.539944470 0.849159062
>> 0.811356246 0.888609290
>>
>> $ ./a.exe
>> n= 33
>> clock: 744096561
>> seed: 744096561 744096598 744096635 744096672 744096709
>> 744096746 744096783 744096820 744096857 744096894 744096931
>> 744096968 744097005 744097042 744097079 744097116 744097153
>> 744097190 744097227 744097264 744097301 744097338
>> 744097375 744097412 744097449 744097486 744097523 744097560
>> 744097597 744097634 744097671 744097708 744097745
>> random: 0.224010825 0.763803065 0.111726880
>> 0.500481725 6.07219338E-02 0.413555145 0.896298766
>> 0.142876744 0.286089420
>>
>>
>> And finally, the output of
>> https://gcc.gnu.org/ml/fortran/2013-04/msg00038.html which you
>> requested in https://gcc.gnu.org/ml/fortran/2013-04/msg00207.html as a
>> test for the original fix:
>>
>> $ ./a.exe
>> 744248371 1000 2147483647
>> 744248371346087 1000000000 9223372036854775807
>>
>> $ ./a.exe
>> 744249100 1000 2147483647
>> 744249100677540 1000000000 9223372036854775807
>>
>> $ ./a.exe
>> 744249678 1000 2147483647
>> 744249678546099 1000000000 9223372036854775807
>>
>> $ ./a.exe
>> 744250116 1000 2147483647
>> 744250116491405 1000000000 9223372036854775807
>>
>>
>>
>> So............ I know this was a long email, but it seems to me that
>> the better upstream fix went in a few weeks before these other
>> libgfortran changes, and they allow for a libgfortran that's untainted
>> by windows.h. I really think this is the better, safer approach that
>> will prevent future errors, and remove the need for CYGWIN macro
>> checks in multiple places.
>
> Ok for trunk.
>
> You said on IRC that cygwin is a rolling release system and they don't
> support old releases, and the bug that prompted this was fixed 4 years
> ago, so I agree we don't need to support that anymore.
As 'NightStrike' doesn't have commit access, I have committed r246162.
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=246162
Don't use Win32 functions on CYGWIN.
This was a workaround for a cygwin bug which was fixed 4 years ago,
and cygwin hasn't supported affected versions for a long time.
2017-03-15 NightStrike <nightstrike@gmail.com>
Janne Blomqvist <jb@gcc.gnu.org>
* intrinsics/random.c (getosrandom): Remove check for __CYGWIN__
preprocessor flag.
* intrinsics/system_clock.c: Likewise.
(system_clock_4): Likewise.
(system_clock_8): Likewise.
* intrinsics/time_1.h: Don't include windows.h if __CYGWIN__ is
defined.
--
Janne Blomqvist
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-03-15 13:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-27 11:15 [Patch, libgfortran, committed] Don't use rand_s on CYGWIN Janne Blomqvist
2017-03-12 17:27 ` NightStrike
2017-03-12 20:11 ` NightStrike
2017-03-12 20:46 ` Janne Blomqvist
2017-03-13 9:01 ` Janne Blomqvist
2017-03-14 4:32 ` NightStrike
2017-03-14 8:05 ` Janne Blomqvist
2017-03-15 13:49 ` Janne Blomqvist
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).