[PATCH] Simplify integer output-related functions in libgfortran

[PATCH] Simplify integer output-related functions in libgfortran

[FX]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

Merry Christmas!

The code related to integer output in libgfortran has accumulated some… oddities over the years. I will soon post a finalized patch for faster integer-to-decimal conversion (see https://gcc.gnu.org/pipermail/fortran/2021-December/057201.html), but while working on that I found a couple of things we ought to fix, that are not directly related.

So this patch is a simplification patch, a no-op. It does the following things:

 - gfc_itoa() is always called for nonnegative values, to make it take an unsigned arg. It allows us to simplify the code handling for negative signs (instead of doing it in two places).
 - fix undefined behaviour on possible overflow when negating large negative values (-HUGE-1)
 - all callers of write_decimal() always use gfc_itoa() as conversion function, so remove one layer of indirection: get rid of that argument, call gfc_itoa() directly inside write_decimal()
 - gfc_xtoa() is only used in one file anymore, so move it there and rename it to xtoa()
 - ztoa_big() is renamed to xtoa_big(), following the convention of other ?to_big() functions
 - runtime/backtrace.c is the only user of gfc_itoa() outside the I/O system; add a comment so we remember in the future why we use gfc_itoa() there… and what are its limits

All this makes the code easier to understand, more consistent, probably marginally more efficient (the gfc_itoa pointer indirection), and will make the future work on speeding up gfc_itoa() easier.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK to commit?

FX


[-- Attachment #2: itoa.patch --]
[-- Type: application/octet-stream, Size: 10520 bytes --]

commit 58f04cee5f9bf8329d494e9fa6be9f7605c75d5c
Author: Francois-Xavier Coudert <fxcoudert@gcc.gnu.org>
Date:   2021-12-25 11:58:14 +0100

    Fortran: simplify library code for integer-to-decimal conversion
    
    libgfortran/ChangeLog:
    
            PR libfortran/81986
            PR libfortran/99191
    
            * libgfortran.h: Remove gfc_xtoa(), adjust gfc_itoa() and
            GFC_ITOA_BUF_SIZE.
            * io/write.c (write_decimal): conversion parameter is always
            gfc_itoa(), so remove it. Protect from overflow.
            (xtoa): Move gfc_xtoa and update its name.
            (xtoa_big): Renamed from ztoa_big for consistency.
            (write_z): Adjust to new function names.
            (write_i, write_integer): Remove last arg of write_decimal.
            * runtime/backtrace.c (error_callback): Comment on the use of
            gfc_itoa().
            * runtime/error.c (gfc_xtoa): Move to io/write.c.
            * runtime/string.c (gfc_itoa): Take an unsigned argument,
            remove the handling of negative values.

diff --git a/libgfortran/io/write.c b/libgfortran/io/write.c
index 278cd47cabb..5be93550a3b 100644
--- a/libgfortran/io/write.c
+++ b/libgfortran/io/write.c
@@ -796,10 +796,10 @@ write_boz (st_parameter_dt *dtp, const fnode *f, const char *q, int n, int len)
 
 static void
 write_decimal (st_parameter_dt *dtp, const fnode *f, const char *source,
-	       int len,
-               const char *(*conv) (GFC_INTEGER_LARGEST, char *, size_t))
+	       int len)
 {
   GFC_INTEGER_LARGEST n = 0;
+  GFC_UINTEGER_LARGEST absn;
   int w, m, digits, nsign, nzero, nblank;
   char *p;
   const char *q;
@@ -832,18 +832,14 @@ write_decimal (st_parameter_dt *dtp, const fnode *f, const char *source,
 
   sign = calculate_sign (dtp, n < 0);
   if (n < 0)
-    n = -n;
+    /* Use unsigned to protect from overflow. */
+    absn = -(GFC_UINTEGER_LARGEST) n;
+  else
+    absn = n;
   nsign = sign == S_NONE ? 0 : 1;
 
-  /* conv calls itoa which sets the negative sign needed
-     by write_integer. The sign '+' or '-' is set below based on sign
-     calculated above, so we just point past the sign in the string
-     before proceeding to avoid double signs in corner cases.
-     (see PR38504)  */
-  q = conv (n, itoa_buf, sizeof (itoa_buf));
-  if (*q == '-')
-    q++;
-
+  /* gfc_itoa() converts the nonnegative value to decimal representation.  */
+  q = gfc_itoa (absn, itoa_buf, sizeof (itoa_buf));
   digits = strlen (q);
 
   /* Select a width if none was specified.  The idea here is to always
@@ -946,7 +942,37 @@ write_decimal (st_parameter_dt *dtp, const fnode *f, const char *source,
 }
 
 
-/* Convert unsigned octal to ascii.  */
+/* Convert hexadecimal to ASCII.  */
+
+static const char *
+xtoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
+{
+  int digit;
+  char *p;
+
+  assert (len >= GFC_XTOA_BUF_SIZE);
+
+  if (n == 0)
+    return "0";
+
+  p = buffer + GFC_XTOA_BUF_SIZE - 1;
+  *p = '\0';
+
+  while (n != 0)
+    {
+      digit = n & 0xF;
+      if (digit > 9)
+	digit += 'A' - '0' - 10;
+
+      *--p = '0' + digit;
+      n >>= 4;
+    }
+
+  return p;
+}
+
+
+/* Convert unsigned octal to ASCII.  */
 
 static const char *
 otoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
@@ -971,7 +997,7 @@ otoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
 }
 
 
-/* Convert unsigned binary to ascii.  */
+/* Convert unsigned binary to ASCII.  */
 
 static const char *
 btoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
@@ -995,7 +1021,7 @@ btoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
   return p;
 }
 
-/* The following three functions, btoa_big, otoa_big, and ztoa_big, are needed
+/* The following three functions, btoa_big, otoa_big, and xtoa_big, are needed
    to convert large reals with kind sizes that exceed the largest integer type
    available on certain platforms.  In these cases, byte by byte conversion is
    performed. Endianess is taken into account.  */
@@ -1133,10 +1159,10 @@ otoa_big (const char *s, char *buffer, int len, GFC_UINTEGER_LARGEST *n)
   return q;
 }
 
-/* Conversion to hexidecimal.  */
+/* Conversion to hexadecimal.  */
 
 static const char *
-ztoa_big (const char *s, char *buffer, int len, GFC_UINTEGER_LARGEST *n)
+xtoa_big (const char *s, char *buffer, int len, GFC_UINTEGER_LARGEST *n)
 {
   static char a[16] = {'0', '1', '2', '3', '4', '5', '6', '7',
     '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
@@ -1178,7 +1204,7 @@ ztoa_big (const char *s, char *buffer, int len, GFC_UINTEGER_LARGEST *n)
 	}
     }
 
-  /* write_z, which calls ztoa_big, is called from transfer.c,
+  /* write_z, which calls xtoa_big, is called from transfer.c,
      formatted_transfer_scalar_write.  There it is passed the kind as
      argument, which means a maximum of 16.  The buffer is large
      enough, but the compiler does not know that, so shut up the
@@ -1202,7 +1228,7 @@ ztoa_big (const char *s, char *buffer, int len, GFC_UINTEGER_LARGEST *n)
 void
 write_i (st_parameter_dt *dtp, const fnode *f, const char *p, int len)
 {
-  write_decimal (dtp, f, p, len, (void *) gfc_itoa);
+  write_decimal (dtp, f, p, len);
 }
 
 
@@ -1259,13 +1285,13 @@ write_z (st_parameter_dt *dtp, const fnode *f, const char *source, int len)
 
   if (len > (int) sizeof (GFC_UINTEGER_LARGEST))
     {
-      p = ztoa_big (source, itoa_buf, len, &n);
+      p = xtoa_big (source, itoa_buf, len, &n);
       write_boz (dtp, f, p, n, len);
     }
   else
     {
       n = extract_uint (source, len);
-      p = gfc_xtoa (n, itoa_buf, sizeof (itoa_buf));
+      p = xtoa (n, itoa_buf, sizeof (itoa_buf));
       write_boz (dtp, f, p, n, len);
     }
 }
@@ -1366,7 +1392,7 @@ write_integer (st_parameter_dt *dtp, const char *source, int kind)
   f.u.integer.w = width;
   f.u.integer.m = -1;
   f.format = FMT_NONE;
-  write_decimal (dtp, &f, source, kind, (void *) gfc_itoa);
+  write_decimal (dtp, &f, source, kind);
 }
 
 
diff --git a/libgfortran/libgfortran.h b/libgfortran/libgfortran.h
index 285c36a00b5..edb4f28601c 100644
--- a/libgfortran/libgfortran.h
+++ b/libgfortran/libgfortran.h
@@ -695,7 +695,7 @@ internal_proto(show_backtrace);
 #define GFC_LARGEST_BUF (sizeof (GFC_INTEGER_LARGEST))
 #endif
 
-#define GFC_ITOA_BUF_SIZE (sizeof (GFC_INTEGER_LARGEST) * 3 + 2)
+#define GFC_ITOA_BUF_SIZE (sizeof (GFC_INTEGER_LARGEST) * 3 + 1)
 #define GFC_XTOA_BUF_SIZE (GFC_LARGEST_BUF * 2 + 1)
 #define GFC_OTOA_BUF_SIZE (GFC_LARGEST_BUF * 3 + 1)
 #define GFC_BTOA_BUF_SIZE (GFC_LARGEST_BUF * 8 + 1)
@@ -723,9 +723,6 @@ extern int st_printf (const char *, ...)
   __attribute__((format (gfc_printf, 1, 2)));
 internal_proto(st_printf);
 
-extern const char *gfc_xtoa (GFC_UINTEGER_LARGEST, char *, size_t);
-internal_proto(gfc_xtoa);
-
 extern _Noreturn void os_error (const char *);
 iexport_proto(os_error);
 
@@ -881,7 +878,7 @@ internal_proto(fc_strdup);
 extern char *fc_strdup_notrim(const char *, gfc_charlen_type);
 internal_proto(fc_strdup_notrim);
 
-extern const char *gfc_itoa(GFC_INTEGER_LARGEST, char *, size_t);
+extern const char *gfc_itoa(GFC_UINTEGER_LARGEST, char *, size_t);
 internal_proto(gfc_itoa);
 
 /* io/intrinsics.c */
diff --git a/libgfortran/runtime/backtrace.c b/libgfortran/runtime/backtrace.c
index 5ac08316f71..403c7c37f18 100644
--- a/libgfortran/runtime/backtrace.c
+++ b/libgfortran/runtime/backtrace.c
@@ -97,6 +97,7 @@ error_callback (void *data, const char *msg, int errnum)
 	  iov[1].iov_len = strlen (msg);
 	  iov[2].iov_base = (char*) ", errno: ";
 	  iov[2].iov_len = strlen (iov[2].iov_base);
+	  /* Async-signal-safe function, errnum must be positive.  */
 	  const char *p = gfc_itoa (errnum, errbuf, sizeof (errbuf));
 	  iov[3].iov_base = (char*) p;
 	  iov[3].iov_len = strlen (p);
diff --git a/libgfortran/runtime/error.c b/libgfortran/runtime/error.c
index b9c75742690..ba6ff866a01 100644
--- a/libgfortran/runtime/error.c
+++ b/libgfortran/runtime/error.c
@@ -219,37 +219,6 @@ exit_error (int status)
 }
 
 
-
-/* gfc_xtoa()-- Integer to hexadecimal conversion.  */
-
-const char *
-gfc_xtoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
-{
-  int digit;
-  char *p;
-
-  assert (len >= GFC_XTOA_BUF_SIZE);
-
-  if (n == 0)
-    return "0";
-
-  p = buffer + GFC_XTOA_BUF_SIZE - 1;
-  *p = '\0';
-
-  while (n != 0)
-    {
-      digit = n & 0xF;
-      if (digit > 9)
-	digit += 'A' - '0' - 10;
-
-      *--p = '0' + digit;
-      n >>= 4;
-    }
-
-  return p;
-}
-
-
 /* Hopefully thread-safe wrapper for a strerror() style function.  */
 
 char *
diff --git a/libgfortran/runtime/string.c b/libgfortran/runtime/string.c
index 536a9cd3f2b..835027a7cd6 100644
--- a/libgfortran/runtime/string.c
+++ b/libgfortran/runtime/string.c
@@ -169,21 +169,22 @@ find_option (st_parameter_common *cmp, const char *s1, gfc_charlen_type s1_len,
 }
 
 
-/* gfc_itoa()-- Integer to decimal conversion.
-   The itoa function is a widespread non-standard extension to
-   standard C, often declared in <stdlib.h>.  Even though the itoa
-   defined here is a static function we take care not to conflict with
-   any prior non-static declaration.  Hence the 'gfc_' prefix, which
-   is normally reserved for functions with external linkage.  Notably,
-   in contrast to the *printf() family of functions, this ought to be
-   async-signal-safe.  */
+/* Integer to decimal conversion.
+
+   This function is much more restricted than the widespread (but
+   non-standard) itoa() function.  This version has the following
+   characteristics:
+
+     - it takes only non-negative arguments
+     - it is async-signal-safe (we use it runtime/backtrace.c)
+     - it works in base 10 (see xtoa, otoa, btoa functions
+       in io/write.c for other radices)
+ */
 
 const char *
-gfc_itoa (GFC_INTEGER_LARGEST n, char *buffer, size_t len)
+gfc_itoa (GFC_UINTEGER_LARGEST n, char *buffer, size_t len)
 {
-  int negative;
   char *p;
-  GFC_UINTEGER_LARGEST t;
 
   if (len < GFC_ITOA_BUF_SIZE)
     sys_abort ();
@@ -191,24 +192,14 @@ gfc_itoa (GFC_INTEGER_LARGEST n, char *buffer, size_t len)
   if (n == 0)
     return "0";
 
-  negative = 0;
-  t = n;
-  if (n < 0)
-    {
-      negative = 1;
-      t = -(GFC_UINTEGER_LARGEST) n;  /* Must use unsigned to protect from overflow. */
-    }
-
   p = buffer + GFC_ITOA_BUF_SIZE - 1;
   *p = '\0';
 
-  while (t != 0)
+  while (n != 0)
     {
-      *--p = '0' + (t % 10);
-      t /= 10;
+      *--p = '0' + (n % 10);
+      n /= 10;
     }
 
-  if (negative)
-    *--p = '-';
   return p;
 }

Re: [PATCH] Simplify integer output-related functions in libgfortran

[Thomas Koenig]

First merry Christmas to all!

> Bootstrapped and regtested on x86_64-pc-linux-gnu.
> OK to commit?

OK.

Thanks for the (preliminary) patch!