| Message ID | AANLkTikx7BpCr3aK8Tdzs=LOESJOe=vaWf-sHwQEo3a7@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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
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?
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 >
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
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).
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 >
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.
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
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