diff mbox

[libgfortran] PR 61310 CTIME intrinsic output incorrect on MinGW

Message ID CAO9iq9HQvTKd_eLrh=uR0sQx9=1Y1JTkapYuaFWR2xsncbSAtg@mail.gmail.com
State New
Headers show

Commit Message

Janne Blomqvist May 25, 2014, 9:21 p.m. UTC
Hi,

GFortran currently uses strftime(...,"%c",...) to produce the result
for the CTIME and FDATE intrinsics. Unfortunately, it seems that on
MinGW this does not produce identical output to the C stdlib ctime(),
even in the default locale.

The attached patch implements an alternative approach, originally
suggested by Jakub in PR 47802, to produce a thread-safe ctime-like
function by using snprintf manually.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk/4.9/4.8/4.7?

2014-05-26  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/61310
    * intrinsics.texi (CTIME): Remove mention of locale-dependent
    behavior.

2014-05-26  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/61310
    * intrinsics/ctime.c (strctime): Rename to gf_ctime, use snprintf
    instead of strftime.
    (fdate): Use gf_ctime.
    (fdate_sub): Likewise.
    (ctime): Likewise.
    (ctime_sub): Likewise.

Comments

Steve Kargl May 25, 2014, 10:25 p.m. UTC | #1
On Mon, May 26, 2014 at 12:21:21AM +0300, Janne Blomqvist wrote:
> Hi,
> 
> GFortran currently uses strftime(...,"%c",...) to produce the result
> for the CTIME and FDATE intrinsics. Unfortunately, it seems that on
> MinGW this does not produce identical output to the C stdlib ctime(),
> even in the default locale.
> 
> The attached patch implements an alternative approach, originally
> suggested by Jakub in PR 47802, to produce a thread-safe ctime-like
> function by using snprintf manually.
> 
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk/4.9/4.8/4.7?
> 

Patch looks ok to me.

> +/* Maximum space a ctime-like string might need. A "normal" ctime
> +   string is 26 bytes, but the maximum possible year number is
> +   2,147,485,547 (2,147,483,647 + 1900, since tm_year is a 32-bit
> +   signed integer) so an extra 6 bytes are needed. */
> +#define CSZ 32

Is there a better name than CSZ, which is not exactly too descriptive?
Janne Blomqvist May 26, 2014, 10 a.m. UTC | #2
On Mon, May 26, 2014 at 1:25 AM, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Mon, May 26, 2014 at 12:21:21AM +0300, Janne Blomqvist wrote:
>> Hi,
>>
>> GFortran currently uses strftime(...,"%c",...) to produce the result
>> for the CTIME and FDATE intrinsics. Unfortunately, it seems that on
>> MinGW this does not produce identical output to the C stdlib ctime(),
>> even in the default locale.
>>
>> The attached patch implements an alternative approach, originally
>> suggested by Jakub in PR 47802, to produce a thread-safe ctime-like
>> function by using snprintf manually.
>>
>> Regtested on x86_64-unknown-linux-gnu, Ok for trunk/4.9/4.8/4.7?
>>
>
> Patch looks ok to me.
>
>> +/* Maximum space a ctime-like string might need. A "normal" ctime
>> +   string is 26 bytes, but the maximum possible year number is
>> +   2,147,485,547 (2,147,483,647 + 1900, since tm_year is a 32-bit
>> +   signed integer) so an extra 6 bytes are needed. */
>> +#define CSZ 32
>
> Is there a better name than CSZ, which is not exactly too descriptive?

Hmm, what about CTIME_BUFSZ? Ok with that change?
Steve Kargl May 26, 2014, 11:41 a.m. UTC | #3
On Mon, May 26, 2014 at 01:00:56PM +0300, Janne Blomqvist wrote:
> On Mon, May 26, 2014 at 1:25 AM, Steve Kargl
> <sgk@troutmask.apl.washington.edu> wrote:
> > On Mon, May 26, 2014 at 12:21:21AM +0300, Janne Blomqvist wrote:
> >> Hi,
> >>
> >> GFortran currently uses strftime(...,"%c",...) to produce the result
> >> for the CTIME and FDATE intrinsics. Unfortunately, it seems that on
> >> MinGW this does not produce identical output to the C stdlib ctime(),
> >> even in the default locale.
> >>
> >> The attached patch implements an alternative approach, originally
> >> suggested by Jakub in PR 47802, to produce a thread-safe ctime-like
> >> function by using snprintf manually.
> >>
> >> Regtested on x86_64-unknown-linux-gnu, Ok for trunk/4.9/4.8/4.7?
> >>
> >
> > Patch looks ok to me.
> >
> >> +/* Maximum space a ctime-like string might need. A "normal" ctime
> >> +   string is 26 bytes, but the maximum possible year number is
> >> +   2,147,485,547 (2,147,483,647 + 1900, since tm_year is a 32-bit
> >> +   signed integer) so an extra 6 bytes are needed. */
> >> +#define CSZ 32
> >
> > Is there a better name than CSZ, which is not exactly too descriptive?
> 
> Hmm, what about CTIME_BUFSZ? Ok with that change?
> 

Yes, the name is much better.  Yes, patch is ok.
diff mbox

Patch

diff --git a/gcc/fortran/intrinsic.texi b/gcc/fortran/intrinsic.texi
index 776cb00..8402a1f 100644
--- a/gcc/fortran/intrinsic.texi
+++ b/gcc/fortran/intrinsic.texi
@@ -3508,10 +3508,8 @@  end program test_cshift
 @table @asis
 @item @emph{Description}:
 @code{CTIME} converts a system time value, such as returned by
-@code{TIME8}, to a string. Unless the application has called
-@code{setlocale}, the output will be in the default locale, of length
-24 and of the form @samp{Sat Aug 19 18:13:14 1995}. In other locales,
-a longer string may result.
+@code{TIME8}, to a string. The output will be of the form @samp{Sat
+Aug 19 18:13:14 1995}.
 
 This intrinsic is provided in both subroutine and function forms; however,
 only one form can be used in any given program unit.
diff --git a/libgfortran/intrinsics/ctime.c b/libgfortran/intrinsics/ctime.c
index db41f02..468e1e5 100644
--- a/libgfortran/intrinsics/ctime.c
+++ b/libgfortran/intrinsics/ctime.c
@@ -31,31 +31,52 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #include <string.h>
 
 
-/* strftime-like function that fills a C string with %c format which
-   is identical to ctime in the default locale. As ctime and ctime_r
-   are poorly specified and their usage not recommended, the
-   implementation instead uses strftime.  */
+/* Maximum space a ctime-like string might need. A "normal" ctime
+   string is 26 bytes, but the maximum possible year number is
+   2,147,485,547 (2,147,483,647 + 1900, since tm_year is a 32-bit
+   signed integer) so an extra 6 bytes are needed. */
+#define CSZ 32
 
-static size_t
-strctime (char *s, size_t max, const time_t *timep)
+
+/* Thread-safe ctime-like function that fills a Fortran
+   string. ctime_r is a portability headache and marked as obsolescent
+   in POSIX 2008 which recommends strftime in its place. However,
+   strftime(..., "%c",...)  doesn't produce ctime-like output on
+   MinGW, so do it manually with snprintf.  */
+
+static int
+gf_ctime (char *s, size_t max, const time_t timev)
 {
   struct tm ltm;
   int failed;
+  char buf[32];
   /* Some targets provide a localtime_r based on a draft of the POSIX
      standard where the return type is int rather than the
      standardized struct tm*.  */
-  __builtin_choose_expr (__builtin_classify_type (localtime_r (timep, &ltm)) 
+  __builtin_choose_expr (__builtin_classify_type (localtime_r (&timev, &ltm)) 
 			 == 5,
-			 failed = localtime_r (timep, &ltm) == NULL,
-			 failed = localtime_r (timep, &ltm) != 0);
+			 failed = localtime_r (&timev, &ltm) == NULL,
+			 failed = localtime_r (&timev, &ltm) != 0);
   if (failed)
-    return 0;
-  return strftime (s, max, "%c", &ltm);
+    goto blank;
+  int n = snprintf (buf, sizeof (buf), 
+		    "%3.3s %3.3s%3d %.2d:%.2d:%.2d %d",
+		    "SunMonTueWedThuFriSat" + ltm.tm_wday * 3,
+		    "JanFebMarAprMayJunJulAugSepOctNovDec" + ltm.tm_mon * 3,
+		    ltm.tm_mday, ltm.tm_hour, ltm.tm_min, ltm.tm_sec, 
+		    1900 + ltm.tm_year);
+  if (n < 0)
+    goto blank;
+  if ((size_t) n <= max)
+    {
+      cf_strcpy (s, max, buf);
+      return n;
+    }
+ blank:
+  memset (s, ' ', max);
+  return 0;
 }
 
-/* In the default locale, the date and time representation fits in 26
-   bytes. However, other locales might need more space.  */
-#define CSZ 100
 
 extern void fdate (char **, gfc_charlen_type *);
 export_proto(fdate);
@@ -65,7 +86,7 @@  fdate (char ** date, gfc_charlen_type * date_len)
 {
   time_t now = time(NULL);
   *date = xmalloc (CSZ);
-  *date_len = strctime (*date, CSZ, &now);
+  *date_len = gf_ctime (*date, CSZ, now);
 }
 
 
@@ -76,10 +97,7 @@  void
 fdate_sub (char * date, gfc_charlen_type date_len)
 {
   time_t now = time(NULL);
-  char *s = xmalloc (date_len + 1);
-  size_t n = strctime (s, date_len + 1, &now);
-  fstrcpy (date, date_len, s, n);
-  free (s);
+  gf_ctime (date, date_len, now);
 }
 
 
@@ -92,7 +110,7 @@  PREFIX(ctime) (char ** date, gfc_charlen_type * date_len, GFC_INTEGER_8 t)
 {
   time_t now = t;
   *date = xmalloc (CSZ);
-  *date_len = strctime (*date, CSZ, &now);
+  *date_len = gf_ctime (*date, CSZ, now);
 }
 
 
@@ -103,8 +121,5 @@  void
 ctime_sub (GFC_INTEGER_8 * t, char * date, gfc_charlen_type date_len)
 {
   time_t now = *t;
-  char *s = xmalloc (date_len + 1);
-  size_t n = strctime (s, date_len + 1, &now);
-  fstrcpy (date, date_len, s, n);
-  free (s);
+  gf_ctime (date, date_len, now);
 }