This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch, libgfortran, committed] Don't use rand_s on CYGWIN
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