[Patch, libgfortran, committed] Don't use rand_s on CYGWIN

Janne Blomqvist blomqvist.janne@gmail.com
Wed Mar 15 13:49:00 GMT 2017


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



More information about the Gcc-patches mailing list