Patchwork [libgfortran] : Fix build of libgfortran for mingw and improve POSIX ctime_r implementation

login
register
mail settings
Submitter Kai Tietz
Date Jan. 29, 2011, 4:57 p.m.
Message ID <AANLkTikx7BpCr3aK8Tdzs=LOESJOe=vaWf-sHwQEo3a7@mail.gmail.com>
Download mbox | patch
Permalink /patch/80941/
State New
Headers show

Comments

Kai Tietz - Jan. 29, 2011, 4:57 p.m.
Hello,

recent changes lead to a build failure for mingw targets in gfortran.
Following patch solves this.

ChangeLog

2011-01-29  Kai Tietz

        * intrinsics/ctime.c (ctime_r): Improve implementation.

Tested for x86_64-w64-mingw32. Ok for apply?

Regards,
Kai
Janne Blomqvist - Jan. 29, 2011, 5:16 p.m.
On Sat, Jan 29, 2011 at 18:57, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hello,
>
> recent changes lead to a build failure for mingw targets in gfortran.
> Following patch solves this.
>
> ChangeLog
>
> 2011-01-29  Kai Tietz
>
>        * intrinsics/ctime.c (ctime_r): Improve implementation.
>
> Tested for x86_64-w64-mingw32. Ok for apply?

Ok, thanks for the patch!

>
> Regards,
> Kai
>
> Index: intrinsics/ctime.c
> ===================================================================
> --- intrinsics/ctime.c  (revision 169388)
> +++ intrinsics/ctime.c  (working copy)
> @@ -42,11 +42,17 @@
>
>
>  #ifndef HAVE_CTIME_R
> +/* Make sure we don't see here a macro.  */
> +#undef ctime_r
> +
>  static char *
>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
>  {
>  #ifdef HAVE_CTIME
> -  return ctime (timep);
> +  char *tmp = ctime (timep);
> +  if (tmp)
> +    tmp = strcpy (buf, tmp);
> +  return tmp;
>  #else
>   return NULL;
>  #endif
>
Kai Tietz - Jan. 29, 2011, 5:20 p.m.
2011/1/29 Janne Blomqvist <blomqvist.janne@gmail.com>:
> On Sat, Jan 29, 2011 at 18:57, Kai Tietz <ktietz70@googlemail.com> wrote:
>> Hello,
>>
>> recent changes lead to a build failure for mingw targets in gfortran.
>> Following patch solves this.
>>
>> ChangeLog
>>
>> 2011-01-29  Kai Tietz
>>
>>        * intrinsics/ctime.c (ctime_r): Improve implementation.
>>
>> Tested for x86_64-w64-mingw32. Ok for apply?
>
> Ok, thanks for the patch!
>
>>
>> Regards,
>> Kai
>>
>> Index: intrinsics/ctime.c
>> ===================================================================
>> --- intrinsics/ctime.c  (revision 169388)
>> +++ intrinsics/ctime.c  (working copy)
>> @@ -42,11 +42,17 @@
>>
>>
>>  #ifndef HAVE_CTIME_R
>> +/* Make sure we don't see here a macro.  */
>> +#undef ctime_r
>> +
>>  static char *
>>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
>>  {
>>  #ifdef HAVE_CTIME
>> -  return ctime (timep);
>> +  char *tmp = ctime (timep);
>> +  if (tmp)
>> +    tmp = strcpy (buf, tmp);
>> +  return tmp;
>>  #else
>>   return NULL;
>>  #endif
>>
>
>
>
> --
> Janne Blomqvist
>

Applied at revision 169389.

Thanks,
Kai
Daniel Franke - Jan. 29, 2011, 6:46 p.m.
On Saturday 29 January 2011 18:20:55 Kai Tietz wrote:
[...]
> >>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
[...]
> >> +    tmp = strcpy (buf, tmp);
[...]
> Applied at revision 169389.

You may want to remove the "__attribute_((unused))" on "buf" as well?

Cheers

	Daniel
Janne Blomqvist - Jan. 29, 2011, 7:16 p.m.
On Sat, Jan 29, 2011 at 20:46, Daniel Franke <franke.daniel@gmail.com> wrote:
> On Saturday 29 January 2011 18:20:55 Kai Tietz wrote:
> [...]
>> >>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
> [...]
>> >> +    tmp = strcpy (buf, tmp);
> [...]
>> Applied at revision 169389.
>
> You may want to remove the "__attribute_((unused))" on "buf" as well?

That would cause a warning on targets without ctime() (the  #else branch).

That being said, what are the things we can actually assume without
testing for it and having fallbacks? Things like ctime() and time()
are C89, do we really support any targets where those are not
available?
Richard Guenther - Jan. 31, 2011, 12:35 p.m.
On Sat, Jan 29, 2011 at 5:57 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hello,
>
> recent changes lead to a build failure for mingw targets in gfortran.
> Following patch solves this.
>
> ChangeLog
>
> 2011-01-29  Kai Tietz
>
>        * intrinsics/ctime.c (ctime_r): Improve implementation.
>
> Tested for x86_64-w64-mingw32. Ok for apply?
>
> Regards,
> Kai
>
> Index: intrinsics/ctime.c
> ===================================================================
> --- intrinsics/ctime.c  (revision 169388)
> +++ intrinsics/ctime.c  (working copy)
> @@ -42,11 +42,17 @@
>
>
>  #ifndef HAVE_CTIME_R
> +/* Make sure we don't see here a macro.  */
> +#undef ctime_r
> +
>  static char *
>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
>  {
>  #ifdef HAVE_CTIME
> -  return ctime (timep);
> +  char *tmp = ctime (timep);
> +  if (tmp)
> +    tmp = strcpy (buf, tmp);
> +  return tmp;

Note that this isn't any better than returning the statically allocated
result of ctime() (I suppose we don't free the result of ctime_r anywhere?).
So we now create memory leaks.  What's the problem with the original
implementation?

Richard.

>  #else
>   return NULL;
>  #endif
>
Kai Tietz - Jan. 31, 2011, 12:46 p.m.
2011/1/31 Richard Guenther <richard.guenther@gmail.com>:
> On Sat, Jan 29, 2011 at 5:57 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> Hello,
>>
>> recent changes lead to a build failure for mingw targets in gfortran.
>> Following patch solves this.
>>
>> ChangeLog
>>
>> 2011-01-29  Kai Tietz
>>
>>        * intrinsics/ctime.c (ctime_r): Improve implementation.
>>
>> Tested for x86_64-w64-mingw32. Ok for apply?
>>
>> Regards,
>> Kai
>>
>> Index: intrinsics/ctime.c
>> ===================================================================
>> --- intrinsics/ctime.c  (revision 169388)
>> +++ intrinsics/ctime.c  (working copy)
>> @@ -42,11 +42,17 @@
>>
>>
>>  #ifndef HAVE_CTIME_R
>> +/* Make sure we don't see here a macro.  */
>> +#undef ctime_r
>> +
>>  static char *
>>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
>>  {
>>  #ifdef HAVE_CTIME
>> -  return ctime (timep);
>> +  char *tmp = ctime (timep);
>> +  if (tmp)
>> +    tmp = strcpy (buf, tmp);
>> +  return tmp;
>
> Note that this isn't any better than returning the statically allocated
> result of ctime() (I suppose we don't free the result of ctime_r anywhere?).
> So we now create memory leaks.  What's the problem with the original
> implementation?
>
> Richard.
>
>>  #else
>>   return NULL;
>>  #endif
>>
>

ctime returns (for windows) a thread-safe pointer, which gets reused
on thread-base. So it is by default thread-safe. But as intention is
here to emulate POSIX behavior of ctime_r, well, we should do that and
not simply introduce here none-standard behavioir by ignoring the
buffer argument. Return of ctime_r shall be NULL on failure, or
address of passed buf argument.

Main issue here is the undefine of ctime_r, as this can be (and is for
mingw in combination with _POSIX) a define to emulate the POSIX
defined behavor.

Kai
Janne Blomqvist - Jan. 31, 2011, 12:51 p.m.
On Mon, Jan 31, 2011 at 14:35, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Sat, Jan 29, 2011 at 5:57 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> Hello,
>>
>> recent changes lead to a build failure for mingw targets in gfortran.
>> Following patch solves this.
>>
>> ChangeLog
>>
>> 2011-01-29  Kai Tietz
>>
>>        * intrinsics/ctime.c (ctime_r): Improve implementation.
>>
>> Tested for x86_64-w64-mingw32. Ok for apply?
>>
>> Regards,
>> Kai
>>
>> Index: intrinsics/ctime.c
>> ===================================================================
>> --- intrinsics/ctime.c  (revision 169388)
>> +++ intrinsics/ctime.c  (working copy)
>> @@ -42,11 +42,17 @@
>>
>>
>>  #ifndef HAVE_CTIME_R
>> +/* Make sure we don't see here a macro.  */
>> +#undef ctime_r
>> +
>>  static char *
>>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
>>  {
>>  #ifdef HAVE_CTIME
>> -  return ctime (timep);
>> +  char *tmp = ctime (timep);
>> +  if (tmp)
>> +    tmp = strcpy (buf, tmp);
>> +  return tmp;
>
> Note that this isn't any better than returning the statically allocated
> result of ctime() (I suppose we don't free the result of ctime_r anywhere?).

All uses of ctime_r() in this file actually use a stack allocated
buffer, so no need to explicitly free it. If anyone cares, one could
even avoid an extra copy by passing the application buffer directly to
ctime_r().

> So we now create memory leaks.  What's the problem with the original
> implementation?

ctime() is not guaranteed to be thread-safe, hence I added the code to
use ctime_r() if available.

Or do you mean what the problem was with the fallback implementation
that called ctime()? Presumably the macro definiton thing was what
caused the build failures, but Kai apparently wanted to fix the
implementation to match POSIX ctime_r() at the same time (the previous
implementation was just enough for the ctime_r() usage in
intrinsics/ctime.c, it wasn't a general purpose ctime_r()
replacement).
Richard Guenther - Jan. 31, 2011, 1:11 p.m.
On Mon, Jan 31, 2011 at 1:51 PM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Mon, Jan 31, 2011 at 14:35, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Sat, Jan 29, 2011 at 5:57 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> Hello,
>>>
>>> recent changes lead to a build failure for mingw targets in gfortran.
>>> Following patch solves this.
>>>
>>> ChangeLog
>>>
>>> 2011-01-29  Kai Tietz
>>>
>>>        * intrinsics/ctime.c (ctime_r): Improve implementation.
>>>
>>> Tested for x86_64-w64-mingw32. Ok for apply?
>>>
>>> Regards,
>>> Kai
>>>
>>> Index: intrinsics/ctime.c
>>> ===================================================================
>>> --- intrinsics/ctime.c  (revision 169388)
>>> +++ intrinsics/ctime.c  (working copy)
>>> @@ -42,11 +42,17 @@
>>>
>>>
>>>  #ifndef HAVE_CTIME_R
>>> +/* Make sure we don't see here a macro.  */
>>> +#undef ctime_r
>>> +
>>>  static char *
>>>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
>>>  {
>>>  #ifdef HAVE_CTIME
>>> -  return ctime (timep);
>>> +  char *tmp = ctime (timep);
>>> +  if (tmp)
>>> +    tmp = strcpy (buf, tmp);
>>> +  return tmp;
>>
>> Note that this isn't any better than returning the statically allocated
>> result of ctime() (I suppose we don't free the result of ctime_r anywhere?).
>
> All uses of ctime_r() in this file actually use a stack allocated
> buffer, so no need to explicitly free it. If anyone cares, one could
> even avoid an extra copy by passing the application buffer directly to
> ctime_r().
>
>> So we now create memory leaks.  What's the problem with the original
>> implementation?
>
> ctime() is not guaranteed to be thread-safe, hence I added the code to
> use ctime_r() if available.
>
> Or do you mean what the problem was with the fallback implementation
> that called ctime()? Presumably the macro definiton thing was what
> caused the build failures, but Kai apparently wanted to fix the
> implementation to match POSIX ctime_r() at the same time (the previous
> implementation was just enough for the ctime_r() usage in
> intrinsics/ctime.c, it wasn't a general purpose ctime_r()
> replacement).

The macro fix is ok, but the rewrite doesn't make it thread-safe
(you'd need a lock for that).  So I think the rewrite doesn't make
any sense.

Richard.

>
>
> --
> Janne Blomqvist
>
Janne Blomqvist - Jan. 31, 2011, 1:39 p.m.
On Mon, Jan 31, 2011 at 15:11, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, Jan 31, 2011 at 1:51 PM, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> On Mon, Jan 31, 2011 at 14:35, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Sat, Jan 29, 2011 at 5:57 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>> Hello,
>>>>
>>>> recent changes lead to a build failure for mingw targets in gfortran.
>>>> Following patch solves this.
>>>>
>>>> ChangeLog
>>>>
>>>> 2011-01-29  Kai Tietz
>>>>
>>>>        * intrinsics/ctime.c (ctime_r): Improve implementation.
>>>>
>>>> Tested for x86_64-w64-mingw32. Ok for apply?
>>>>
>>>> Regards,
>>>> Kai
>>>>
>>>> Index: intrinsics/ctime.c
>>>> ===================================================================
>>>> --- intrinsics/ctime.c  (revision 169388)
>>>> +++ intrinsics/ctime.c  (working copy)
>>>> @@ -42,11 +42,17 @@
>>>>
>>>>
>>>>  #ifndef HAVE_CTIME_R
>>>> +/* Make sure we don't see here a macro.  */
>>>> +#undef ctime_r
>>>> +
>>>>  static char *
>>>>  ctime_r (const time_t * timep, char * buf __attribute__((unused)))
>>>>  {
>>>>  #ifdef HAVE_CTIME
>>>> -  return ctime (timep);
>>>> +  char *tmp = ctime (timep);
>>>> +  if (tmp)
>>>> +    tmp = strcpy (buf, tmp);
>>>> +  return tmp;
>>>
>>> Note that this isn't any better than returning the statically allocated
>>> result of ctime() (I suppose we don't free the result of ctime_r anywhere?).
>>
>> All uses of ctime_r() in this file actually use a stack allocated
>> buffer, so no need to explicitly free it. If anyone cares, one could
>> even avoid an extra copy by passing the application buffer directly to
>> ctime_r().
>>
>>> So we now create memory leaks.  What's the problem with the original
>>> implementation?
>>
>> ctime() is not guaranteed to be thread-safe, hence I added the code to
>> use ctime_r() if available.
>>
>> Or do you mean what the problem was with the fallback implementation
>> that called ctime()? Presumably the macro definiton thing was what
>> caused the build failures, but Kai apparently wanted to fix the
>> implementation to match POSIX ctime_r() at the same time (the previous
>> implementation was just enough for the ctime_r() usage in
>> intrinsics/ctime.c, it wasn't a general purpose ctime_r()
>> replacement).
>
> The macro fix is ok, but the rewrite doesn't make it thread-safe
> (you'd need a lock for that).  So I think the rewrite doesn't make
> any sense.

I think the point was that the rewrite makes it conform to POSIX
semantics, i.e. the buffer is filled, return NULL in case an error
occurs.  It might not matter at the moment, but if someone later
changes the usage of ctime_r() and does not have the opportunity to
test on MingW, Kai might have pre-empted a regression.

Wrt locks, my guess is that targets that a) support threads and b)
matter, either provide _r() functions or provide thread-safe non-_r()
functions (presumably by using TLS, such as MingW). Hence locking is
not needed. In case anyone feels differently, their target is in any
case no worse off than it was before libgfortran started using the
_r() functions, and they are of course welcome to submit patches.

Patch

Index: intrinsics/ctime.c
===================================================================
--- intrinsics/ctime.c  (revision 169388)
+++ intrinsics/ctime.c  (working copy)
@@ -42,11 +42,17 @@ 


 #ifndef HAVE_CTIME_R
+/* Make sure we don't see here a macro.  */
+#undef ctime_r
+
 static char *
 ctime_r (const time_t * timep, char * buf __attribute__((unused)))
 {
 #ifdef HAVE_CTIME
-  return ctime (timep);
+  char *tmp = ctime (timep);
+  if (tmp)
+    tmp = strcpy (buf, tmp);
+  return tmp;
 #else
   return NULL;
 #endif