[PATCH 5/6] vfprintf: Introduce printf_positional function

[PATCH 5/6] vfprintf: Introduce printf_positional function

[Florian Weimer]

This splits a considerable chunk of code from the main vfprintf
function.  This will make it easier to remove the use of extend_alloca
from the positional argument handling code.
---
 stdio-common/vfprintf.c | 716 +++++++++++++++++++++++++-----------------------
 1 file changed, 369 insertions(+), 347 deletions(-)

diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index da52dbc..6e840aa 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -204,6 +204,14 @@ static const CHAR_T null[] = L_("(null)");
 static int buffered_vfprintf (FILE *stream, const CHAR_T *fmt, va_list)
      __THROW __attribute__ ((noinline)) internal_function;
 
+/* Handle positional format specifiers.  */
+static int printf_positional (_IO_FILE *s,
+			      const CHAR_T *format, int readonly_format,
+			      va_list ap, va_list *ap_savep, int done,
+			      int nspecs_done, const UCHAR_T *lead_str_end,
+			      CHAR_T *work_buffer, int save_errno,
+			      const char *grouping, THOUSANDS_SEP_T);
+
 /* Handle unknown format specifier.  */
 static int printf_unknown (FILE *, const struct printf_info *,
 			   const void *const *) __THROW;
@@ -1257,15 +1265,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
      0 if unknown.  */
   int readonly_format = 0;
 
-  /* For the argument descriptions, which may be allocated on the heap.  */
-  void *args_malloced = NULL;
-
-  /* For positional argument handling.  */
-  struct printf_spec *specs;
-
-  /* Track if we malloced the SPECS array and thus must free it.  */
-  bool specs_malloced = false;
-
   /* Orient the stream.  */
 #ifdef ORIENT
   ORIENT;
@@ -1670,232 +1669,265 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
   /* Unlock stream and return.  */
   goto all_done;
 
-  /* Here starts the more complex loop to handle positional parameters.  */
+  /* Hand off processing for positional parameters.  */
 do_positional:
-  {
-    /* Array with information about the needed arguments.  This has to
-       be dynamically extensible.  */
-    size_t nspecs = 0;
-    /* A more or less arbitrary start value.  */
-    size_t nspecs_size = 32 * sizeof (struct printf_spec);
-
-    specs = alloca (nspecs_size);
-    /* The number of arguments the format string requests.  This will
-       determine the size of the array needed to store the argument
-       attributes.  */
-    size_t nargs = 0;
-    size_t bytes_per_arg;
-    union printf_arg *args_value;
-    int *args_size;
-    int *args_type;
-
-    /* Positional parameters refer to arguments directly.  This could
-       also determine the maximum number of arguments.  Track the
-       maximum number.  */
-    size_t max_ref_arg = 0;
-
-    /* Just a counter.  */
-    size_t cnt;
-
-    if (__glibc_unlikely (workstart != NULL))
+  if (__glibc_unlikely (workstart != NULL))
+    {
       free (workstart);
-    workstart = NULL;
+      workstart = NULL;
+    }
+  done = printf_positional (s, format, readonly_format, ap, &ap_save,
+			    done, nspecs_done, lead_str_end, work_buffer,
+			    save_errno, grouping, thousands_sep);
 
-    if (grouping == (const char *) -1)
-      {
+ all_done:
+  if (__glibc_unlikely (workstart != NULL))
+    free (workstart);
+  /* Unlock the stream.  */
+  _IO_funlockfile (s);
+  _IO_cleanup_region_end (0);
+
+  return done;
+}
+\f
+static int
+printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
+		   va_list ap, va_list *ap_savep, int done, int nspecs_done,
+		   const UCHAR_T *lead_str_end,
+		   CHAR_T *work_buffer, int save_errno,
+		   const char *grouping, THOUSANDS_SEP_T thousands_sep)
+{
+  /* For the argument descriptions, which may be allocated on the heap.  */
+  void *args_malloced = NULL;
+
+  /* For positional argument handling.  */
+  struct printf_spec *specs;
+
+  /* Track if we malloced the SPECS array and thus must free it.  */
+  bool specs_malloced = false;
+
+  /* Array with information about the needed arguments.  This has to
+     be dynamically extensible.  */
+  size_t nspecs = 0;
+  /* A more or less arbitrary start value.  */
+  size_t nspecs_size = 32 * sizeof (struct printf_spec);
+
+  specs = alloca (nspecs_size);
+  /* The number of arguments the format string requests.  This will
+     determine the size of the array needed to store the argument
+     attributes.  */
+  size_t nargs = 0;
+  size_t bytes_per_arg;
+  union printf_arg *args_value;
+  int *args_size;
+  int *args_type;
+
+  /* Positional parameters refer to arguments directly.  This could
+     also determine the maximum number of arguments.  Track the
+     maximum number.  */
+  size_t max_ref_arg = 0;
+
+  /* Just a counter.  */
+  size_t cnt;
+
+  CHAR_T *workstart = NULL;
+
+  if (grouping == (const char *) -1)
+    {
 #ifdef COMPILE_WPRINTF
-	thousands_sep = _NL_CURRENT_WORD (LC_NUMERIC,
-					  _NL_NUMERIC_THOUSANDS_SEP_WC);
+      thousands_sep = _NL_CURRENT_WORD (LC_NUMERIC,
+					_NL_NUMERIC_THOUSANDS_SEP_WC);
 #else
-	thousands_sep = _NL_CURRENT (LC_NUMERIC, THOUSANDS_SEP);
+      thousands_sep = _NL_CURRENT (LC_NUMERIC, THOUSANDS_SEP);
 #endif
 
-	grouping = _NL_CURRENT (LC_NUMERIC, GROUPING);
-	if (*grouping == '\0' || *grouping == CHAR_MAX)
-	  grouping = NULL;
-      }
+      grouping = _NL_CURRENT (LC_NUMERIC, GROUPING);
+      if (*grouping == '\0' || *grouping == CHAR_MAX)
+	grouping = NULL;
+    }
 
-    for (f = lead_str_end; *f != L_('\0'); f = specs[nspecs++].next_fmt)
-      {
-	if (nspecs * sizeof (*specs) >= nspecs_size)
-	  {
-	    /* Extend the array of format specifiers.  */
-	    if (nspecs_size * 2 < nspecs_size)
-	      {
-		__set_errno (ENOMEM);
-		done = -1;
-		goto all_done;
-	      }
-	    struct printf_spec *old = specs;
-	    if (__libc_use_alloca (2 * nspecs_size))
-	      specs = extend_alloca (specs, nspecs_size, 2 * nspecs_size);
-	    else
-	      {
-		nspecs_size *= 2;
-		specs = malloc (nspecs_size);
-		if (specs == NULL)
-		  {
-		    __set_errno (ENOMEM);
-		    specs = old;
-		    done = -1;
-		    goto all_done;
-		  }
-	      }
+  for (const UCHAR_T *f = lead_str_end; *f != L_('\0');
+       f = specs[nspecs++].next_fmt)
+    {
+      if (nspecs * sizeof (*specs) >= nspecs_size)
+	{
+	  /* Extend the array of format specifiers.  */
+	  if (nspecs_size * 2 < nspecs_size)
+	    {
+	      __set_errno (ENOMEM);
+	      done = -1;
+	      goto all_done;
+	    }
+	  struct printf_spec *old = specs;
+	  if (__libc_use_alloca (2 * nspecs_size))
+	    specs = extend_alloca (specs, nspecs_size, 2 * nspecs_size);
+	  else
+	    {
+	      nspecs_size *= 2;
+	      specs = malloc (nspecs_size);
+	      if (specs == NULL)
+		{
+		  __set_errno (ENOMEM);
+		  specs = old;
+		  done = -1;
+		  goto all_done;
+		}
+	    }
 
-	    /* Copy the old array's elements to the new space.  */
-	    memmove (specs, old, nspecs * sizeof (*specs));
+	  /* Copy the old array's elements to the new space.  */
+	  memmove (specs, old, nspecs * sizeof (*specs));
 
-	    /* If we had previously malloc'd space for SPECS, then
-	       release it after the copy is complete.  */
-	    if (specs_malloced)
-	      free (old);
+	  /* If we had previously malloc'd space for SPECS, then
+	     release it after the copy is complete.  */
+	  if (specs_malloced)
+	    free (old);
 
-	    /* Now set SPECS_MALLOCED if needed.  */
-	    if (!__libc_use_alloca (nspecs_size))
-	      specs_malloced = true;
-	  }
+	  /* Now set SPECS_MALLOCED if needed.  */
+	  if (!__libc_use_alloca (nspecs_size))
+	    specs_malloced = true;
+	}
 
-	/* Parse the format specifier.  */
+      /* Parse the format specifier.  */
 #ifdef COMPILE_WPRINTF
-	nargs += __parse_one_specwc (f, nargs, &specs[nspecs], &max_ref_arg);
+      nargs += __parse_one_specwc (f, nargs, &specs[nspecs], &max_ref_arg);
 #else
-	nargs += __parse_one_specmb (f, nargs, &specs[nspecs], &max_ref_arg);
+      nargs += __parse_one_specmb (f, nargs, &specs[nspecs], &max_ref_arg);
 #endif
-      }
-
-    /* Determine the number of arguments the format string consumes.  */
-    nargs = MAX (nargs, max_ref_arg);
-    /* Calculate total size needed to represent a single argument across
-       all three argument-related arrays.  */
-    bytes_per_arg = (sizeof (*args_value) + sizeof (*args_size)
-		     + sizeof (*args_type));
+    }
 
-    /* Check for potential integer overflow.  */
-    if (__glibc_unlikely (nargs > INT_MAX / bytes_per_arg))
-      {
-	 __set_errno (EOVERFLOW);
-	 done = -1;
-	 goto all_done;
-      }
+  /* Determine the number of arguments the format string consumes.  */
+  nargs = MAX (nargs, max_ref_arg);
+  /* Calculate total size needed to represent a single argument across
+     all three argument-related arrays.  */
+  bytes_per_arg = (sizeof (*args_value) + sizeof (*args_size)
+		   + sizeof (*args_type));
 
-    /* Allocate memory for all three argument arrays.  */
-    if (__libc_use_alloca (nargs * bytes_per_arg))
-	args_value = alloca (nargs * bytes_per_arg);
-    else
-      {
-	args_value = args_malloced = malloc (nargs * bytes_per_arg);
-	if (args_value == NULL)
-	  {
-	    done = -1;
-	    goto all_done;
-	  }
-      }
+  /* Check for potential integer overflow.  */
+  if (__glibc_unlikely (nargs > INT_MAX / bytes_per_arg))
+    {
+      __set_errno (EOVERFLOW);
+      done = -1;
+      goto all_done;
+    }
 
-    /* Set up the remaining two arrays to each point past the end of the
-       prior array, since space for all three has been allocated now.  */
-    args_size = &args_value[nargs].pa_int;
-    args_type = &args_size[nargs];
-    memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
-	    nargs * sizeof (*args_type));
+  /* Allocate memory for all three argument arrays.  */
+  if (__libc_use_alloca (nargs * bytes_per_arg))
+    args_value = alloca (nargs * bytes_per_arg);
+  else
+    {
+      args_value = args_malloced = malloc (nargs * bytes_per_arg);
+      if (args_value == NULL)
+	{
+	  done = -1;
+	  goto all_done;
+	}
+    }
 
-    /* XXX Could do sanity check here: If any element in ARGS_TYPE is
-       still zero after this loop, format is invalid.  For now we
-       simply use 0 as the value.  */
+  /* Set up the remaining two arrays to each point past the end of the
+     prior array, since space for all three has been allocated now.  */
+  args_size = &args_value[nargs].pa_int;
+  args_type = &args_size[nargs];
+  memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
+	  nargs * sizeof (*args_type));
 
-    /* Fill in the types of all the arguments.  */
-    for (cnt = 0; cnt < nspecs; ++cnt)
-      {
-	/* If the width is determined by an argument this is an int.  */
-	if (specs[cnt].width_arg != -1)
-	  args_type[specs[cnt].width_arg] = PA_INT;
+  /* XXX Could do sanity check here: If any element in ARGS_TYPE is
+     still zero after this loop, format is invalid.  For now we
+     simply use 0 as the value.  */
 
-	/* If the precision is determined by an argument this is an int.  */
-	if (specs[cnt].prec_arg != -1)
-	  args_type[specs[cnt].prec_arg] = PA_INT;
+  /* Fill in the types of all the arguments.  */
+  for (cnt = 0; cnt < nspecs; ++cnt)
+    {
+      /* If the width is determined by an argument this is an int.  */
+      if (specs[cnt].width_arg != -1)
+	args_type[specs[cnt].width_arg] = PA_INT;
 
-	switch (specs[cnt].ndata_args)
-	  {
-	  case 0:		/* No arguments.  */
-	    break;
-	  case 1:		/* One argument; we already have the
-				   type and size.  */
-	    args_type[specs[cnt].data_arg] = specs[cnt].data_arg_type;
-	    args_size[specs[cnt].data_arg] = specs[cnt].size;
-	    break;
-	  default:
-	    /* We have more than one argument for this format spec.
-	       We must call the arginfo function again to determine
-	       all the types.  */
-	    (void) (*__printf_arginfo_table[specs[cnt].info.spec])
-	      (&specs[cnt].info,
-	       specs[cnt].ndata_args, &args_type[specs[cnt].data_arg],
-	       &args_size[specs[cnt].data_arg]);
-	    break;
-	  }
-      }
+      /* If the precision is determined by an argument this is an int.  */
+      if (specs[cnt].prec_arg != -1)
+	args_type[specs[cnt].prec_arg] = PA_INT;
 
-    /* Now we know all the types and the order.  Fill in the argument
-       values.  */
-    for (cnt = 0; cnt < nargs; ++cnt)
-      switch (args_type[cnt])
+      switch (specs[cnt].ndata_args)
 	{
-#define T(tag, mem, type)						      \
-	case tag:							      \
-	  args_value[cnt].mem = va_arg (ap_save, type);			      \
+	case 0:		/* No arguments.  */
+	  break;
+	case 1:		/* One argument; we already have the
+			   type and size.  */
+	  args_type[specs[cnt].data_arg] = specs[cnt].data_arg_type;
+	  args_size[specs[cnt].data_arg] = specs[cnt].size;
+	  break;
+	default:
+	  /* We have more than one argument for this format spec.
+	     We must call the arginfo function again to determine
+	     all the types.  */
+	  (void) (*__printf_arginfo_table[specs[cnt].info.spec])
+	    (&specs[cnt].info,
+	     specs[cnt].ndata_args, &args_type[specs[cnt].data_arg],
+	     &args_size[specs[cnt].data_arg]);
+	  break;
+	}
+    }
+
+  /* Now we know all the types and the order.  Fill in the argument
+     values.  */
+  for (cnt = 0; cnt < nargs; ++cnt)
+    switch (args_type[cnt])
+      {
+#define T(tag, mem, type)				\
+	case tag:					\
+	  args_value[cnt].mem = va_arg (*ap_savep, type); \
 	  break
 
 	T (PA_WCHAR, pa_wchar, wint_t);
-	case PA_CHAR:				/* Promoted.  */
-	case PA_INT|PA_FLAG_SHORT:		/* Promoted.  */
+      case PA_CHAR:				/* Promoted.  */
+      case PA_INT|PA_FLAG_SHORT:		/* Promoted.  */
 #if LONG_MAX == INT_MAX
-	case PA_INT|PA_FLAG_LONG:
+      case PA_INT|PA_FLAG_LONG:
 #endif
 	T (PA_INT, pa_int, int);
 #if LONG_MAX == LONG_LONG_MAX
-	case PA_INT|PA_FLAG_LONG:
+      case PA_INT|PA_FLAG_LONG:
 #endif
 	T (PA_INT|PA_FLAG_LONG_LONG, pa_long_long_int, long long int);
 #if LONG_MAX != INT_MAX && LONG_MAX != LONG_LONG_MAX
 # error "he?"
 #endif
-	case PA_FLOAT:				/* Promoted.  */
+      case PA_FLOAT:				/* Promoted.  */
 	T (PA_DOUBLE, pa_double, double);
-	case PA_DOUBLE|PA_FLAG_LONG_DOUBLE:
-	  if (__ldbl_is_dbl)
-	    {
-	      args_value[cnt].pa_double = va_arg (ap_save, double);
-	      args_type[cnt] &= ~PA_FLAG_LONG_DOUBLE;
-	    }
-	  else
-	    args_value[cnt].pa_long_double = va_arg (ap_save, long double);
-	  break;
-	case PA_STRING:				/* All pointers are the same */
-	case PA_WSTRING:			/* All pointers are the same */
+      case PA_DOUBLE|PA_FLAG_LONG_DOUBLE:
+	if (__ldbl_is_dbl)
+	  {
+	    args_value[cnt].pa_double = va_arg (*ap_savep, double);
+	    args_type[cnt] &= ~PA_FLAG_LONG_DOUBLE;
+	  }
+	else
+	  args_value[cnt].pa_long_double = va_arg (*ap_savep, long double);
+	break;
+      case PA_STRING:				/* All pointers are the same */
+      case PA_WSTRING:			/* All pointers are the same */
 	T (PA_POINTER, pa_pointer, void *);
 #undef T
-	default:
-	  if ((args_type[cnt] & PA_FLAG_PTR) != 0)
-	    args_value[cnt].pa_pointer = va_arg (ap_save, void *);
-	  else if (__glibc_unlikely (__printf_va_arg_table != NULL)
-		   && __printf_va_arg_table[args_type[cnt] - PA_LAST] != NULL)
-	    {
-	      args_value[cnt].pa_user = alloca (args_size[cnt]);
-	      (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
-		(args_value[cnt].pa_user, &ap_save);
-	    }
-	  else
-	    args_value[cnt].pa_long_double = 0.0;
-	  break;
-	case -1:
-	  /* Error case.  Not all parameters appear in N$ format
-	     strings.  We have no way to determine their type.  */
-	  assert (s->_flags2 & _IO_FLAGS2_FORTIFY);
-	  __libc_fatal ("*** invalid %N$ use detected ***\n");
-	}
+      default:
+	if ((args_type[cnt] & PA_FLAG_PTR) != 0)
+	  args_value[cnt].pa_pointer = va_arg (*ap_savep, void *);
+	else if (__glibc_unlikely (__printf_va_arg_table != NULL)
+		 && __printf_va_arg_table[args_type[cnt] - PA_LAST] != NULL)
+	  {
+	    args_value[cnt].pa_user = alloca (args_size[cnt]);
+	    (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
+	      (args_value[cnt].pa_user, ap_savep);
+	  }
+	else
+	  args_value[cnt].pa_long_double = 0.0;
+	break;
+      case -1:
+	/* Error case.  Not all parameters appear in N$ format
+	   strings.  We have no way to determine their type.  */
+	assert (s->_flags2 & _IO_FLAGS2_FORTIFY);
+	__libc_fatal ("*** invalid %N$ use detected ***\n");
+      }
 
-    /* Now walk through all format specifiers and process them.  */
-    for (; (size_t) nspecs_done < nspecs; ++nspecs_done)
-      {
+  /* Now walk through all format specifiers and process them.  */
+  for (; (size_t) nspecs_done < nspecs; ++nspecs_done)
+    {
 #undef REF
 #ifdef SHARED
 # undef JUMP_TABLE_BASE_LABEL
@@ -1906,184 +1938,174 @@ do_positional:
 #endif
 #undef LABEL
 #define LABEL(Name) do2_##Name
-	STEP4_TABLE;
+      STEP4_TABLE;
 
-	int is_negative;
-	union
+      int is_negative;
+      union
+      {
+	unsigned long long int longlong;
+	unsigned long int word;
+      } number;
+      int base;
+      union printf_arg the_arg;
+      CHAR_T *string;		/* Pointer to argument string.  */
+
+      /* Fill variables from values in struct.  */
+      int alt = specs[nspecs_done].info.alt;
+      int space = specs[nspecs_done].info.space;
+      int left = specs[nspecs_done].info.left;
+      int showsign = specs[nspecs_done].info.showsign;
+      int group = specs[nspecs_done].info.group;
+      int is_long_double = specs[nspecs_done].info.is_long_double;
+      int is_short = specs[nspecs_done].info.is_short;
+      int is_char = specs[nspecs_done].info.is_char;
+      int is_long = specs[nspecs_done].info.is_long;
+      int width = specs[nspecs_done].info.width;
+      int prec = specs[nspecs_done].info.prec;
+      int use_outdigits = specs[nspecs_done].info.i18n;
+      char pad = specs[nspecs_done].info.pad;
+      CHAR_T spec = specs[nspecs_done].info.spec;
+
+      workstart = NULL;
+      CHAR_T *workend = work_buffer + WORK_BUFFER_SIZE;
+
+      /* Fill in last information.  */
+      if (specs[nspecs_done].width_arg != -1)
 	{
-	  unsigned long long int longlong;
-	  unsigned long int word;
-	} number;
-	int base;
-	union printf_arg the_arg;
-	CHAR_T *string;		/* Pointer to argument string.  */
-
-	/* Fill variables from values in struct.  */
-	int alt = specs[nspecs_done].info.alt;
-	int space = specs[nspecs_done].info.space;
-	int left = specs[nspecs_done].info.left;
-	int showsign = specs[nspecs_done].info.showsign;
-	int group = specs[nspecs_done].info.group;
-	int is_long_double = specs[nspecs_done].info.is_long_double;
-	int is_short = specs[nspecs_done].info.is_short;
-	int is_char = specs[nspecs_done].info.is_char;
-	int is_long = specs[nspecs_done].info.is_long;
-	int width = specs[nspecs_done].info.width;
-	int prec = specs[nspecs_done].info.prec;
-	int use_outdigits = specs[nspecs_done].info.i18n;
-	char pad = specs[nspecs_done].info.pad;
-	CHAR_T spec = specs[nspecs_done].info.spec;
-
-	workstart = NULL;
-	workend = work_buffer + WORK_BUFFER_SIZE;
-
-	/* Fill in last information.  */
-	if (specs[nspecs_done].width_arg != -1)
-	  {
-	    /* Extract the field width from an argument.  */
-	    specs[nspecs_done].info.width =
-	      args_value[specs[nspecs_done].width_arg].pa_int;
+	  /* Extract the field width from an argument.  */
+	  specs[nspecs_done].info.width =
+	    args_value[specs[nspecs_done].width_arg].pa_int;
 
-	    if (specs[nspecs_done].info.width < 0)
-	      /* If the width value is negative left justification is
-		 selected and the value is taken as being positive.  */
-	      {
-		specs[nspecs_done].info.width *= -1;
-		left = specs[nspecs_done].info.left = 1;
-	      }
-	    width = specs[nspecs_done].info.width;
-	  }
+	  if (specs[nspecs_done].info.width < 0)
+	    /* If the width value is negative left justification is
+	       selected and the value is taken as being positive.  */
+	    {
+	      specs[nspecs_done].info.width *= -1;
+	      left = specs[nspecs_done].info.left = 1;
+	    }
+	  width = specs[nspecs_done].info.width;
+	}
 
-	if (specs[nspecs_done].prec_arg != -1)
-	  {
-	    /* Extract the precision from an argument.  */
-	    specs[nspecs_done].info.prec =
-	      args_value[specs[nspecs_done].prec_arg].pa_int;
+      if (specs[nspecs_done].prec_arg != -1)
+	{
+	  /* Extract the precision from an argument.  */
+	  specs[nspecs_done].info.prec =
+	    args_value[specs[nspecs_done].prec_arg].pa_int;
 
-	    if (specs[nspecs_done].info.prec < 0)
-	      /* If the precision is negative the precision is
-		 omitted.  */
-	      specs[nspecs_done].info.prec = -1;
+	  if (specs[nspecs_done].info.prec < 0)
+	    /* If the precision is negative the precision is
+	       omitted.  */
+	    specs[nspecs_done].info.prec = -1;
 
-	    prec = specs[nspecs_done].info.prec;
-	  }
+	  prec = specs[nspecs_done].info.prec;
+	}
 
-	/* Maybe the buffer is too small.  */
-	if (MAX (prec, width) + 32 > WORK_BUFFER_SIZE)
-	  {
-	    if (__libc_use_alloca ((MAX (prec, width) + 32)
-				   * sizeof (CHAR_T)))
-	      workend = ((CHAR_T *) alloca ((MAX (prec, width) + 32)
-					    * sizeof (CHAR_T))
-			 + (MAX (prec, width) + 32));
-	    else
-	      {
-		workstart = (CHAR_T *) malloc ((MAX (prec, width) + 32)
-					       * sizeof (CHAR_T));
-		if (workstart == NULL)
-		  {
-		    done = -1;
-		    goto all_done;
-		  }
-		workend = workstart + (MAX (prec, width) + 32);
-	      }
-	  }
+      /* Maybe the buffer is too small.  */
+      if (MAX (prec, width) + 32 > WORK_BUFFER_SIZE)
+	{
+	  if (__libc_use_alloca ((MAX (prec, width) + 32)
+				 * sizeof (CHAR_T)))
+	    workend = ((CHAR_T *) alloca ((MAX (prec, width) + 32)
+					  * sizeof (CHAR_T))
+		       + (MAX (prec, width) + 32));
+	  else
+	    {
+	      workstart = (CHAR_T *) malloc ((MAX (prec, width) + 32)
+					     * sizeof (CHAR_T));
+	      if (workstart == NULL)
+		{
+		  done = -1;
+		  goto all_done;
+		}
+	      workend = workstart + (MAX (prec, width) + 32);
+	    }
+	}
 
-	/* Process format specifiers.  */
-	while (1)
-	  {
-	    extern printf_function **__printf_function_table;
-	    int function_done;
+      /* Process format specifiers.  */
+      while (1)
+	{
+	  extern printf_function **__printf_function_table;
+	  int function_done;
 
-	    if (spec <= UCHAR_MAX
-		&& __printf_function_table != NULL
-		&& __printf_function_table[(size_t) spec] != NULL)
-	      {
-		const void **ptr = alloca (specs[nspecs_done].ndata_args
-					   * sizeof (const void *));
+	  if (spec <= UCHAR_MAX
+	      && __printf_function_table != NULL
+	      && __printf_function_table[(size_t) spec] != NULL)
+	    {
+	      const void **ptr = alloca (specs[nspecs_done].ndata_args
+					 * sizeof (const void *));
 
-		/* Fill in an array of pointers to the argument values.  */
-		for (unsigned int i = 0; i < specs[nspecs_done].ndata_args;
-		     ++i)
-		  ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
+	      /* Fill in an array of pointers to the argument values.  */
+	      for (unsigned int i = 0; i < specs[nspecs_done].ndata_args;
+		   ++i)
+		ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
 
-		/* Call the function.  */
-		function_done = __printf_function_table[(size_t) spec]
-		  (s, &specs[nspecs_done].info, ptr);
+	      /* Call the function.  */
+	      function_done = __printf_function_table[(size_t) spec]
+		(s, &specs[nspecs_done].info, ptr);
 
-		if (function_done != -2)
-		  {
-		    /* If an error occurred we don't have information
-		       about # of chars.  */
-		    if (function_done < 0)
-		      {
-			/* Function has set errno.  */
-			done = -1;
-			goto all_done;
-		      }
-
-		    done_add (function_done);
-		    break;
-		  }
-	      }
+	      if (function_done != -2)
+		{
+		  /* If an error occurred we don't have information
+		     about # of chars.  */
+		  if (function_done < 0)
+		    {
+		      /* Function has set errno.  */
+		      done = -1;
+		      goto all_done;
+		    }
+
+		  done_add (function_done);
+		  break;
+		}
+	    }
 
-	    JUMP (spec, step4_jumps);
+	  JUMP (spec, step4_jumps);
 
-	    process_arg ((&specs[nspecs_done]));
-	    process_string_arg ((&specs[nspecs_done]));
+	  process_arg ((&specs[nspecs_done]));
+	  process_string_arg ((&specs[nspecs_done]));
 
 	  LABEL (form_unknown):
-	    {
-	      unsigned int i;
-	      const void **ptr;
+	  {
+	    unsigned int i;
+	    const void **ptr;
 
-	      ptr = alloca (specs[nspecs_done].ndata_args
-			    * sizeof (const void *));
+	    ptr = alloca (specs[nspecs_done].ndata_args
+			  * sizeof (const void *));
 
-	      /* Fill in an array of pointers to the argument values.  */
-	      for (i = 0; i < specs[nspecs_done].ndata_args; ++i)
-		ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
+	    /* Fill in an array of pointers to the argument values.  */
+	    for (i = 0; i < specs[nspecs_done].ndata_args; ++i)
+	      ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
 
-	      /* Call the function.  */
-	      function_done = printf_unknown (s, &specs[nspecs_done].info,
-					      ptr);
+	    /* Call the function.  */
+	    function_done = printf_unknown (s, &specs[nspecs_done].info,
+					    ptr);
 
-	      /* If an error occurred we don't have information about #
-		 of chars.  */
-	      if (function_done < 0)
-		{
-		  /* Function has set errno.  */
-		  done = -1;
-		  goto all_done;
-		}
+	    /* If an error occurred we don't have information about #
+	       of chars.  */
+	    if (function_done < 0)
+	      {
+		/* Function has set errno.  */
+		done = -1;
+		goto all_done;
+	      }
 
-	      done_add (function_done);
-	    }
-	    break;
+	    done_add (function_done);
 	  }
+	  break;
+	}
 
-	if (__glibc_unlikely (workstart != NULL))
-	  free (workstart);
-	workstart = NULL;
-
-	/* Write the following constant string.  */
-	outstring (specs[nspecs_done].end_of_fmt,
-		   specs[nspecs_done].next_fmt
-		   - specs[nspecs_done].end_of_fmt);
-      }
-  }
+      if (__glibc_unlikely (workstart != NULL))
+	free (workstart);
+      workstart = NULL;
 
-all_done:
-  if (specs_malloced)
-    free (specs);
-  if (__glibc_unlikely (args_malloced != NULL))
-    free (args_malloced);
+      /* Write the following constant string.  */
+      outstring (specs[nspecs_done].end_of_fmt,
+		 specs[nspecs_done].next_fmt
+		 - specs[nspecs_done].end_of_fmt);
+    }
+ all_done:
   if (__glibc_unlikely (workstart != NULL))
     free (workstart);
-  /* Unlock the stream.  */
-  _IO_funlockfile (s);
-  _IO_cleanup_region_end (0);
-
   return done;
 }
 \f
-- 
2.1.0

[PATCH 2/6] vfprintf: Introduce JUMP_TABLE_BASE_LABEL

[Florian Weimer]

This makes the offset handling more explicit and avoids
cross-references between the jump tables.
---
 stdio-common/vfprintf.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index d575994..16e70b8 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -304,7 +304,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	  spec = (ChExpr);						      \
 	  offset = NOT_IN_JUMP_RANGE (spec) ? REF (form_unknown)	      \
 	    : table[CHAR_CLASS (spec)];					      \
-	  ptr = &&do_form_unknown + offset;				      \
+	  ptr = &&JUMP_TABLE_BASE_LABEL + offset;			      \
 	  goto *ptr;							      \
 	}								      \
       while (0)
@@ -1329,7 +1329,8 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
   do
     {
 #ifdef SHARED
-# define REF(Name) &&do_##Name - &&do_form_unknown
+# define JUMP_TABLE_BASE_LABEL do_form_unknown
+# define REF(Name) &&do_##Name - &&JUMP_TABLE_BASE_LABEL
 #else
 # define REF(Name) &&do_##Name
 #endif
@@ -1897,7 +1898,9 @@ do_positional:
       {
 #undef REF
 #ifdef SHARED
-# define REF(Name) &&do2_##Name - &&do_form_unknown
+# undef JUMP_TABLE_BASE_LABEL
+# define JUMP_TABLE_BASE_LABEL do2_form_unknown
+# define REF(Name) &&do2_##Name - &&JUMP_TABLE_BASE_LABEL
 #else
 # define REF(Name) &&do2_##Name
 #endif
-- 
2.1.0

[PATCH 1/6] vfprintf: Introduce THOUSANDS_SEP_T

[Florian Weimer]

This avoids preprocessor conditionals in function declarations.
---
 stdio-common/vfprintf.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index a41449d..d575994 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -81,6 +81,7 @@
 # define CHAR_T		char
 # define UCHAR_T	unsigned char
 # define INT_T		int
+typedef const char *THOUSANDS_SEP_T;
 # define L_(Str)	Str
 # define ISDIGIT(Ch)	((unsigned int) ((Ch) - '0') < 10)
 # define STR_LEN(Str)	strlen (Str)
@@ -108,6 +109,7 @@
 /* This is a hack!!!  There should be a type uwchar_t.  */
 # define UCHAR_T	unsigned int /* uwchar_t */
 # define INT_T		wint_t
+typedef wchar_t THOUSANDS_SEP_T;
 # define L_(Str)	L##Str
 # define ISDIGIT(Ch)	((unsigned int) ((Ch) - L'0') < 10)
 # define STR_LEN(Str)	__wcslen (Str)
@@ -207,25 +209,15 @@ static int printf_unknown (FILE *, const struct printf_info *,
 			   const void *const *) __THROW;
 
 /* Group digits of number string.  */
-#ifdef COMPILE_WPRINTF
-static CHAR_T *group_number (CHAR_T *, CHAR_T *, const char *, wchar_t)
-     __THROW internal_function;
-#else
-static CHAR_T *group_number (CHAR_T *, CHAR_T *, const char *, const char *)
+static CHAR_T *group_number (CHAR_T *, CHAR_T *, const char *, THOUSANDS_SEP_T)
      __THROW internal_function;
-#endif
-
 
 /* The function itself.  */
 int
 vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 {
   /* The character used as thousands separator.  */
-#ifdef COMPILE_WPRINTF
-  wchar_t thousands_sep = L'\0';
-#else
-  const char *thousands_sep = NULL;
-#endif
+  THOUSANDS_SEP_T thousands_sep = 0;
 
   /* The string describing the size of groups of digits.  */
   const char *grouping;
@@ -2150,12 +2142,7 @@ printf_unknown (FILE *s, const struct printf_info *info,
 static CHAR_T *
 internal_function
 group_number (CHAR_T *w, CHAR_T *rear_ptr, const char *grouping,
-#ifdef COMPILE_WPRINTF
-	      wchar_t thousands_sep
-#else
-	      const char *thousands_sep
-#endif
-	      )
+	      THOUSANDS_SEP_T thousands_sep)
 {
   int len;
   CHAR_T *src, *s;
-- 
2.1.0

[PATCH 4/6] vfprintf: Move jump table definition and the macros out of function

[Florian Weimer]

The second jump table will be moved to a separate function
in the next commit.
---
 stdio-common/vfprintf.c | 108 ++++++++++++++++++++++++------------------------
 1 file changed, 54 insertions(+), 54 deletions(-)

diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index dcc24b3..da52dbc 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -212,60 +212,10 @@ static int printf_unknown (FILE *, const struct printf_info *,
 static CHAR_T *group_number (CHAR_T *, CHAR_T *, const char *, THOUSANDS_SEP_T)
      __THROW internal_function;
 
-/* The function itself.  */
-int
-vfprintf (FILE *s, const CHAR_T *format, va_list ap)
-{
-  /* The character used as thousands separator.  */
-  THOUSANDS_SEP_T thousands_sep = 0;
-
-  /* The string describing the size of groups of digits.  */
-  const char *grouping;
-
-  /* Place to accumulate the result.  */
-  int done;
-
-  /* Current character in format string.  */
-  const UCHAR_T *f;
-
-  /* End of leading constant string.  */
-  const UCHAR_T *lead_str_end;
-
-  /* Points to next format specifier.  */
-  const UCHAR_T *end_of_spec;
-
-  /* Buffer intermediate results.  */
-#define WORK_BUFFER_SIZE 1000
-  CHAR_T work_buffer[WORK_BUFFER_SIZE];
-  CHAR_T *workstart = NULL;
-  CHAR_T *workend;
-
-  /* We have to save the original argument pointer.  */
-  va_list ap_save;
-
-  /* Count number of specifiers we already processed.  */
-  int nspecs_done;
-
-  /* For the %m format we may need the current `errno' value.  */
-  int save_errno = errno;
-
-  /* 1 if format is in read-only memory, -1 if it is in writable memory,
-     0 if unknown.  */
-  int readonly_format = 0;
-
-  /* For the argument descriptions, which may be allocated on the heap.  */
-  void *args_malloced = NULL;
-
-  /* For positional argument handling.  */
-  struct printf_spec *specs;
-
-  /* Track if we malloced the SPECS array and thus must free it.  */
-  bool specs_malloced = false;
-
-  /* This table maps a character into a number representing a
-     class.  In each step there is a destination label for each
-     class.  */
-  static const uint8_t jump_table[] =
+/* Jump tables.  This table maps a character into a number
+   representing a class.  In each step there is a destination label
+   for each class.  */
+static const uint8_t jump_table[] =
   {
     /* ' ' */  1,            0,            0, /* '#' */  4,
 	       0, /* '%' */ 14,            0, /* '\''*/  6,
@@ -1266,6 +1216,56 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
       break;
 #endif
 
+/* The function itself.  */
+int
+vfprintf (FILE *s, const CHAR_T *format, va_list ap)
+{
+  /* The character used as thousands separator.  */
+  THOUSANDS_SEP_T thousands_sep = 0;
+
+  /* The string describing the size of groups of digits.  */
+  const char *grouping;
+
+  /* Place to accumulate the result.  */
+  int done;
+
+  /* Current character in format string.  */
+  const UCHAR_T *f;
+
+  /* End of leading constant string.  */
+  const UCHAR_T *lead_str_end;
+
+  /* Points to next format specifier.  */
+  const UCHAR_T *end_of_spec;
+
+  /* Buffer intermediate results.  */
+#define WORK_BUFFER_SIZE 1000
+  CHAR_T work_buffer[WORK_BUFFER_SIZE];
+  CHAR_T *workstart = NULL;
+  CHAR_T *workend;
+
+  /* We have to save the original argument pointer.  */
+  va_list ap_save;
+
+  /* Count number of specifiers we already processed.  */
+  int nspecs_done;
+
+  /* For the %m format we may need the current `errno' value.  */
+  int save_errno = errno;
+
+  /* 1 if format is in read-only memory, -1 if it is in writable memory,
+     0 if unknown.  */
+  int readonly_format = 0;
+
+  /* For the argument descriptions, which may be allocated on the heap.  */
+  void *args_malloced = NULL;
+
+  /* For positional argument handling.  */
+  struct printf_spec *specs;
+
+  /* Track if we malloced the SPECS array and thus must free it.  */
+  bool specs_malloced = false;
+
   /* Orient the stream.  */
 #ifdef ORIENT
   ORIENT;
-- 
2.1.0

[PATCH 0/6] Some vfprintf refactoring

[Florian Weimer]

These patches move around some code in vfprintf, and finally split off
the positional argument handling code from the main vfprintf function.

Tested on x86_64-redhat-linux-gnu, with no regressions.

2015-03-01  Florian Weimer  <fweimer@redhat.com>

        * stdio-common/vfprintf.c (THOUSANDS_SEP_T): New typedef.
        (group_number, vfprintf): Use it.
        (JUMP_TABLE_BASE_LABEL): New preprocessor macro.
        (JUMP, REF): Use it.
        (WORK_BUFFER_SIZE): New preprocessor macro.
        (process_arg, vfprintf): Use it.
        (jump_table): Move out of the vfprintf function.
        (NOT_IN_JUMP_RANGE, CHAR_CLASS, LABEL, REF, JUMP, STEP0_3_TABLE,
        STEP4_TABLE, process_arg): Move macro definitions
        out of the vfprintf function.  (Cosmetic change only.)
        (vfprintf): Move local variables args_malloced, specs,
        specs_malloced, and the code after do_positional to the
        printf_positional function.
        (printf_positional): New function.
        (LABEL, JUMP_TABLE_BASE_LABEL, REF): Adjust jump table label
        generation macros.

Florian Weimer (6):
  vfprintf: Introduce THOUSANDS_SEP_T
  vfprintf: Introduce JUMP_TABLE_BASE_LABEL
  vfprintf: Define WORK_BUFFER_SIZE
  vfprintf: Move jump table definition and the macros out of function
  vfprintf: Introduce printf_positional function
  vfprintf: Remove label name switching for the jump table

 stdio-common/vfprintf.c | 856 ++++++++++++++++++++++++------------------------
 1 file changed, 427 insertions(+), 429 deletions(-)

-- 
2.1.0

[PATCH 3/6] vfprintf: Define WORK_BUFFER_SIZE

[Florian Weimer]

This constant will allow us to refer to the number of elements in
work_buffer across a function call boundary.
---
 stdio-common/vfprintf.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 16e70b8..dcc24b3 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -235,7 +235,8 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
   const UCHAR_T *end_of_spec;
 
   /* Buffer intermediate results.  */
-  CHAR_T work_buffer[1000];
+#define WORK_BUFFER_SIZE 1000
+  CHAR_T work_buffer[WORK_BUFFER_SIZE];
   CHAR_T *workstart = NULL;
   CHAR_T *workend;
 
@@ -986,7 +987,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
       /* Print description of error ERRNO.  */				      \
       string =								      \
 	(CHAR_T *) __strerror_r (save_errno, (char *) work_buffer,	      \
-				 sizeof work_buffer);			      \
+				 WORK_BUFFER_SIZE * sizeof (CHAR_T));	      \
       is_long = 0;		/* This is no wide-char string.  */	      \
       goto LABEL (print_string)
 
@@ -1366,7 +1367,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
       CHAR_T spec;
 
       workstart = NULL;
-      workend = &work_buffer[sizeof (work_buffer) / sizeof (CHAR_T)];
+      workend = work_buffer + WORK_BUFFER_SIZE;
 
       /* Get current character in format string.  */
       JUMP (*++f, step0_jumps);
@@ -1465,7 +1466,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	    goto all_done;
 	  }
 
-	if (width >= sizeof (work_buffer) / sizeof (work_buffer[0]) - 32)
+	if (width >= WORK_BUFFER_SIZE - 32)
 	  {
 	    /* We have to use a special buffer.  The "32" is just a safe
 	       bet for all the output which is not counted in the width.  */
@@ -1498,7 +1499,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	  goto all_done;
 	}
 
-      if (width >= sizeof (work_buffer) / sizeof (work_buffer[0]) - 32)
+      if (width >= WORK_BUFFER_SIZE - 32)
 	{
 	  /* We have to use a special buffer.  The "32" is just a safe
 	     bet for all the output which is not counted in the width.  */
@@ -1564,8 +1565,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
 	}
       else
 	prec = 0;
-      if (prec > width
-	  && prec > sizeof (work_buffer) / sizeof (work_buffer[0]) - 32)
+      if (prec > width && prec > WORK_BUFFER_SIZE - 32)
 	{
 	  if (__glibc_unlikely (prec >= INT_MAX / sizeof (CHAR_T) - 32))
 	    {
@@ -1935,7 +1935,7 @@ do_positional:
 	CHAR_T spec = specs[nspecs_done].info.spec;
 
 	workstart = NULL;
-	workend = &work_buffer[sizeof (work_buffer) / sizeof (CHAR_T)];
+	workend = work_buffer + WORK_BUFFER_SIZE;
 
 	/* Fill in last information.  */
 	if (specs[nspecs_done].width_arg != -1)
@@ -1969,8 +1969,7 @@ do_positional:
 	  }
 
 	/* Maybe the buffer is too small.  */
-	if (MAX (prec, width) + 32 > (int) (sizeof (work_buffer)
-					    / sizeof (CHAR_T)))
+	if (MAX (prec, width) + 32 > WORK_BUFFER_SIZE)
 	  {
 	    if (__libc_use_alloca ((MAX (prec, width) + 32)
 				   * sizeof (CHAR_T)))
-- 
2.1.0

Re: [PATCH 2/6] vfprintf: Introduce JUMP_TABLE_BASE_LABEL

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> @@ -1897,7 +1898,9 @@ do_positional:
>        {
>  #undef REF
>  #ifdef SHARED
> -# define REF(Name) &&do2_##Name - &&do_form_unknown
> +# undef JUMP_TABLE_BASE_LABEL
> +# define JUMP_TABLE_BASE_LABEL do2_form_unknown

Where do you define that label?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 2/6] vfprintf: Introduce JUMP_TABLE_BASE_LABEL

[Florian Weimer]

On 03/01/2015 11:36 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> @@ -1897,7 +1898,9 @@ do_positional:
>>        {
>>  #undef REF
>>  #ifdef SHARED
>> -# define REF(Name) &&do2_##Name - &&do_form_unknown
>> +# undef JUMP_TABLE_BASE_LABEL
>> +# define JUMP_TABLE_BASE_LABEL do2_form_unknown
> 
> Where do you define that label?

Its name is generated with the LABEL macro:

#define LABEL(Name) do2_##Name

and eventually defined by the STEP4_TABLE macro.

-- 
Florian Weimer / Red Hat Product Security

[PATCH 6/6] vfprintf: Remove label name switching for the jump table

[Florian Weimer]

Different labels are no longer needed because the tables are now in
separate functions.
---
 stdio-common/vfprintf.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 6e840aa..6bf59ff 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -252,9 +252,12 @@ static const uint8_t jump_table[] =
 
 #define NOT_IN_JUMP_RANGE(Ch) ((Ch) < L_(' ') || (Ch) > L_('z'))
 #define CHAR_CLASS(Ch) (jump_table[(INT_T) (Ch) - L_(' ')])
+#define LABEL(Name) do_##Name
 #ifdef SHARED
   /* 'int' is enough and it saves some space on 64 bit systems.  */
 # define JUMP_TABLE_TYPE const int
+# define JUMP_TABLE_BASE_LABEL do_form_unknown
+# define REF(Name) &&do_##Name - &&JUMP_TABLE_BASE_LABEL
 # define JUMP(ChExpr, table)						      \
       do								      \
 	{								      \
@@ -269,6 +272,7 @@ static const uint8_t jump_table[] =
       while (0)
 #else
 # define JUMP_TABLE_TYPE const void *const
+# define REF(Name) &&do_##Name
 # define JUMP(ChExpr, table)						      \
       do								      \
 	{								      \
@@ -1328,13 +1332,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
   /* Process whole format string.  */
   do
     {
-#ifdef SHARED
-# define JUMP_TABLE_BASE_LABEL do_form_unknown
-# define REF(Name) &&do_##Name - &&JUMP_TABLE_BASE_LABEL
-#else
-# define REF(Name) &&do_##Name
-#endif
-#define LABEL(Name) do_##Name
       STEP0_3_TABLE;
       STEP4_TABLE;
 
@@ -1928,16 +1925,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
   /* Now walk through all format specifiers and process them.  */
   for (; (size_t) nspecs_done < nspecs; ++nspecs_done)
     {
-#undef REF
-#ifdef SHARED
-# undef JUMP_TABLE_BASE_LABEL
-# define JUMP_TABLE_BASE_LABEL do2_form_unknown
-# define REF(Name) &&do2_##Name - &&JUMP_TABLE_BASE_LABEL
-#else
-# define REF(Name) &&do2_##Name
-#endif
-#undef LABEL
-#define LABEL(Name) do2_##Name
       STEP4_TABLE;
 
       int is_negative;
-- 
2.1.0

Re: [PATCH 1/6] vfprintf: Introduce THOUSANDS_SEP_T

[Paul Eggert]

Florian Weimer wrote:
>   # define INT_T		int
> +typedef const char *THOUSANDS_SEP_T;

A nit: if the name's not a macro, I'd keep it lower-case.  Upper-case is for 
shouting "WATCH OUT! I LOOK LIKE A NORMAL NAME, BUT I ACTUALLY AM A MACRO!"

Re: [PATCH 2/6] vfprintf: Introduce JUMP_TABLE_BASE_LABEL

[Paul Eggert]

Florian Weimer wrote:
> +# define REF(Name) &&do_##Name - &&JUMP_TABLE_BASE_LABEL
...
> +# define REF(Name) &&do2_##Name - &&JUMP_TABLE_BASE_LABEL

The definientia should be parenthesized.

Re: [PATCH 3/6] vfprintf: Define WORK_BUFFER_SIZE

[Paul Eggert]

Florian Weimer wrote:
> -  CHAR_T work_buffer[1000];
> +#define WORK_BUFFER_SIZE 1000
> +  CHAR_T work_buffer[WORK_BUFFER_SIZE];

Another nit: I suggest avoiding the macro, as it's confusing when #defined 
inside a function body but intended to be used outside the function, and instead 
doing this at the top level:

enum { WORK_BUFFER_SIZE = 1000 };

The general idea is to use a macro only when necessary.

Re: [PATCH 1/6] vfprintf: Introduce THOUSANDS_SEP_T

[Florian Weimer]

On 03/03/2015 02:59 AM, Paul Eggert wrote:
> Florian Weimer wrote:
>>   # define INT_T        int
>> +typedef const char *THOUSANDS_SEP_T;
> 
> A nit: if the name's not a macro, I'd keep it lower-case.  Upper-case is
> for shouting "WATCH OUT! I LOOK LIKE A NORMAL NAME, BUT I ACTUALLY AM A
> MACRO!"

In this case, I was more going for “careful, this depends on whether you
are in the wide variant or not” (like CHAR_T).  I agree with you that we
should avoid the preprocessor if possible, so I used a typedef.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH 3/6] vfprintf: Define WORK_BUFFER_SIZE

[Florian Weimer]

On 03/03/2015 03:07 AM, Paul Eggert wrote:
> Florian Weimer wrote:
>> -  CHAR_T work_buffer[1000];
>> +#define WORK_BUFFER_SIZE 1000
>> +  CHAR_T work_buffer[WORK_BUFFER_SIZE];
> 
> Another nit: I suggest avoiding the macro, as it's confusing when
> #defined inside a function body but intended to be used outside the
> function, and instead doing this at the top level:
> 
> enum { WORK_BUFFER_SIZE = 1000 };
> 
> The general idea is to use a macro only when necessary.

Good idea, thanks.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH 0/6] Some vfprintf refactoring

[Carlos O'Donell]

On 03/01/2015 04:55 PM, Florian Weimer wrote:
> These patches move around some code in vfprintf, and finally split off
> the positional argument handling code from the main vfprintf function.

Why? :-)

Cheers,
Carlos.

Re: [PATCH 3/6] vfprintf: Define WORK_BUFFER_SIZE

[Carlos O'Donell]

On 03/01/2015 03:52 PM, Florian Weimer wrote:
> This constant will allow us to refer to the number of elements in
> work_buffer across a function call boundary.
> ---
>  stdio-common/vfprintf.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

Looks to me as long as you follow Paul's suggestion to
use an enum instead of a burried macro.

Cheers,
Carlos.

Re: [PATCH 4/6] vfprintf: Move jump table definition and the macros out of function

[Carlos O'Donell]

On 03/01/2015 04:00 PM, Florian Weimer wrote:
> The second jump table will be moved to a separate function
> in the next commit.
> ---
>  stdio-common/vfprintf.c | 108 ++++++++++++++++++++++++------------------------
>  1 file changed, 54 insertions(+), 54 deletions(-)

How does this impact the generated code and performance of these routines?

Cheers,
Carlos.

Re: [PATCH 1/6] vfprintf: Introduce THOUSANDS_SEP_T

[Carlos O'Donell]

On 03/01/2015 03:42 PM, Florian Weimer wrote:
> This avoids preprocessor conditionals in function declarations.
> ---
>  stdio-common/vfprintf.c | 23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)

OK.

c.

Re: [PATCH 0/6] Some vfprintf refactoring

[Florian Weimer]

On 03/05/2015 08:35 PM, Carlos O'Donell wrote:
> On 03/01/2015 04:55 PM, Florian Weimer wrote:
>> These patches move around some code in vfprintf, and finally split off
>> the positional argument handling code from the main vfprintf function.
> 
> Why? :-)

The goal is to remove extend_alloca.  Define a local struct
scratch_buffer variable involves a large stack allocation.  My initial
version just put a struct scratch_buffer variable definition roughly in
the same place as the work_buffer array.  But that would increase stack
usage in all cases, even with positional parameters.  I expected that
this would raise a red flag during review.

So I tried to eliminate the unconditional stack allocation of the
scratch buffer.  First, I tried to move the scratch buffer definition
after the do_positional label.  While doing that, I noticed the business
with the jump tables.  It turns out that both the non-positional and
positional copy of the tables jump to the all_done label.  In the
positional case, the scratch buffer is out of scope and cannot be freed.
 So this does not work with the current scratch buffer functions.  I
started adding a parameter to the jump table code so that I could inject
cleanup code specific to the positional case, but I stopped when I
realized what I was doing.  Rather than writing yet another version of
the append-to-buffer code, I then realized that separating out the
positional case into a separate function would allow me to customize the
action when the all_done label is reached, which eventually led to the
current patch series.

GCC will inline the new printf_positional function because it is called
in just one place, so any differences should be just random noise by
slightly different optimizer choices.  I will try to verify that and
report back the results.  But due to the huge size of the function, it
could be very difficult to get a complete picture.  Benchmarking is
difficult because I don't know what a representative use case of
positional arguments would be.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH 0/6] Some vfprintf refactoring

[Carlos O'Donell]

On 03/05/2015 03:53 PM, Florian Weimer wrote:
> On 03/05/2015 08:35 PM, Carlos O'Donell wrote:
>> On 03/01/2015 04:55 PM, Florian Weimer wrote:
>>> These patches move around some code in vfprintf, and finally split off
>>> the positional argument handling code from the main vfprintf function.
>>
>> Why? :-)
> 
> The goal is to remove extend_alloca.  Define a local struct
> scratch_buffer variable involves a large stack allocation.  My initial
> version just put a struct scratch_buffer variable definition roughly in
> the same place as the work_buffer array.  But that would increase stack
> usage in all cases, even with positional parameters.  I expected that
> this would raise a red flag during review.
> 
> So I tried to eliminate the unconditional stack allocation of the
> scratch buffer.  First, I tried to move the scratch buffer definition
> after the do_positional label.  While doing that, I noticed the business
> with the jump tables.  It turns out that both the non-positional and
> positional copy of the tables jump to the all_done label.  In the
> positional case, the scratch buffer is out of scope and cannot be freed.
>  So this does not work with the current scratch buffer functions.  I
> started adding a parameter to the jump table code so that I could inject
> cleanup code specific to the positional case, but I stopped when I
> realized what I was doing.  Rather than writing yet another version of
> the append-to-buffer code, I then realized that separating out the
> positional case into a separate function would allow me to customize the
> action when the all_done label is reached, which eventually led to the
> current patch series.
> 
> GCC will inline the new printf_positional function because it is called
> in just one place, so any differences should be just random noise by
> slightly different optimizer choices.  I will try to verify that and
> report back the results.  But due to the huge size of the function, it
> could be very difficult to get a complete picture.  Benchmarking is
> difficult because I don't know what a representative use case of
> positional arguments would be.
> 

Excellent answer, and exactly the kind of detailed summary
I'm expecting in a 0-th entry for a long series of patches.

Always list the shortcomings, or problems, or attempted
solutions. Don't let the reader infer these or need to
consider them and then reject them just as you did.

You want to make it easy for me to accept the patches.

Thanks for taking the time to write this up.

Cheers,
Carlos.

Re: [PATCH 4/6] vfprintf: Move jump table definition and the macros out of function

[Florian Weimer]

On 03/05/2015 08:38 PM, Carlos O'Donell wrote:
> On 03/01/2015 04:00 PM, Florian Weimer wrote:
>> The second jump table will be moved to a separate function
>> in the next commit.
>> ---
>>  stdio-common/vfprintf.c | 108 ++++++++++++++++++++++++------------------------
>>  1 file changed, 54 insertions(+), 54 deletions(-)
> 
> How does this impact the generated code and performance of these routines?

The generated code is identical.

Or is your question about next change (the printf_positional function)?

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH 2/6] vfprintf: Introduce JUMP_TABLE_BASE_LABEL

[Florian Weimer]

On 03/03/2015 03:00 AM, Paul Eggert wrote:
> Florian Weimer wrote:
>> +# define REF(Name) &&do_##Name - &&JUMP_TABLE_BASE_LABEL
> ...
>> +# define REF(Name) &&do2_##Name - &&JUMP_TABLE_BASE_LABEL
> 
> The definientia should be parenthesized.

Committed with this change.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Florian Weimer]

On 03/01/2015 10:16 PM, Florian Weimer wrote:
> This splits a considerable chunk of code from the main vfprintf
> function.  This will make it easier to remove the use of extend_alloca
> from the positional argument handling code.

Inspection of the generated assembly on x86_64 shows that splitting the
two functions helps GCC 4.9 with register allocation; there are fewer
spills.

I used the following totally made-up benchmark to see if there is a
performance regression.

#define _GNU_SOURCE

#include <stdlib.h>
#include <stdio.h>
#include <time.h>
#include <err.h>

#ifdef POSITIONAL
#define FORMAT "   %1$d: %2$c%3$c%4$c%5$c%6$c %7$20s %8$f (%9$02x)\n"
#else
#define FORMAT "   %d: %c%c%c%c%c %20s %f (%02x)\n"
#endif

int
main (int argc, char **argv)
{
  int iterations = atoi (argv[1]);
  struct timespec ts_start;
  if (clock_gettime (CLOCK_MONOTONIC_RAW, &ts_start) != 0)
    err (1, "clock_gettime");
  for (int i = 0; i < iterations; ++i)
    {
      char buf[256];
      sprintf (buf, FORMAT,
	       1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234);
    }
  struct timespec ts_end;
  if (clock_gettime (CLOCK_MONOTONIC_RAW, &ts_end) != 0)
    err (1, "clock_gettime");
  printf ("%f\n",
	  ts_end.tv_sec + ts_end.tv_nsec * 1e-9
	  - ts_start.tv_sec - ts_start.tv_nsec * 1e-9);
  return 0;
}

t-test without -DPOSITONAL (50 runs):

data:  before and after
t = 3.8079, df = 91.121, p-value = 0.0002539
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
 0.01236649 0.03933739
sample estimates:
mean of x mean of y
1.0200367 0.9941847


t-test with -DPOSITIONAL=1 (50 runs):

data:  before and after
t = 5.2566, df = 65.134, p-value = 1.74e-06
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
 0.06132365 0.13646651
sample estimates:
mean of x mean of y
 1.749552  1.650657

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH 0/6] Some vfprintf refactoring

[Florian Weimer]

On 03/05/2015 09:53 PM, Florian Weimer wrote:

> GCC will inline the new printf_positional function because it is called
> in just one place, so any differences should be just random noise by
> slightly different optimizer choices.

I forgot to mention in my message about the performance impact that this
is not actually true: GCC will not inline printf_positional because of
the way it uses label addresses/computed gotos.  But as explained in the
other message, this does not make the code worse (with GCC 4.9 at least):

<https://sourceware.org/ml/libc-alpha/2015-03/msg00266.html>

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH 4/6] vfprintf: Move jump table definition and the macros out of function

[Carlos O'Donell]

On 03/06/2015 05:35 AM, Florian Weimer wrote:
> On 03/05/2015 08:38 PM, Carlos O'Donell wrote:
>> On 03/01/2015 04:00 PM, Florian Weimer wrote:
>>> The second jump table will be moved to a separate function
>>> in the next commit.
>>> ---
>>>  stdio-common/vfprintf.c | 108 ++++++++++++++++++++++++------------------------
>>>  1 file changed, 54 insertions(+), 54 deletions(-)
>>
>> How does this impact the generated code and performance of these routines?
> 
> The generated code is identical.
> 
> Or is your question about next change (the printf_positional function)?

The question was for this specific change, but it does apply equally
across the board. I see your other post about the fact that it actually
improves performance and I'll reply there.

c.
 

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Carlos O'Donell]

On 03/06/2015 07:57 AM, Florian Weimer wrote:
> On 03/01/2015 10:16 PM, Florian Weimer wrote:
>> This splits a considerable chunk of code from the main vfprintf
>> function.  This will make it easier to remove the use of extend_alloca
>> from the positional argument handling code.
> 
> Inspection of the generated assembly on x86_64 shows that splitting the
> two functions helps GCC 4.9 with register allocation; there are fewer
> spills.

Awesome.

> I used the following totally made-up benchmark to see if there is a
> performance regression.

Made up or not, it's a microbenchmark you used to test the changes.

Could you please post this as a separate patch for inclusion into 
benchtests/ as a positional printf microbenchmark? That way we have
a record of what was used, and can look at changes to printf as we
adjust this code?

Cheers,
Carlos.

Re: [PATCH 6/6] vfprintf: Remove label name switching for the jump table

[Carlos O'Donell]

On 03/01/2015 04:19 PM, Florian Weimer wrote:
> Different labels are no longer needed because the tables are now in
> separate functions.
> ---
>  stdio-common/vfprintf.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)

This looks good to me and simplifies the code, which is always
a win for vfprintf.

Cheers,
Carlos.

> diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
> index 6e840aa..6bf59ff 100644
> --- a/stdio-common/vfprintf.c
> +++ b/stdio-common/vfprintf.c
> @@ -252,9 +252,12 @@ static const uint8_t jump_table[] =
>  
>  #define NOT_IN_JUMP_RANGE(Ch) ((Ch) < L_(' ') || (Ch) > L_('z'))
>  #define CHAR_CLASS(Ch) (jump_table[(INT_T) (Ch) - L_(' ')])
> +#define LABEL(Name) do_##Name
>  #ifdef SHARED
>    /* 'int' is enough and it saves some space on 64 bit systems.  */
>  # define JUMP_TABLE_TYPE const int
> +# define JUMP_TABLE_BASE_LABEL do_form_unknown
> +# define REF(Name) &&do_##Name - &&JUMP_TABLE_BASE_LABEL
>  # define JUMP(ChExpr, table)						      \
>        do								      \
>  	{								      \
> @@ -269,6 +272,7 @@ static const uint8_t jump_table[] =
>        while (0)
>  #else
>  # define JUMP_TABLE_TYPE const void *const
> +# define REF(Name) &&do_##Name
>  # define JUMP(ChExpr, table)						      \
>        do								      \
>  	{								      \
> @@ -1328,13 +1332,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
>    /* Process whole format string.  */
>    do
>      {
> -#ifdef SHARED
> -# define JUMP_TABLE_BASE_LABEL do_form_unknown
> -# define REF(Name) &&do_##Name - &&JUMP_TABLE_BASE_LABEL
> -#else
> -# define REF(Name) &&do_##Name
> -#endif
> -#define LABEL(Name) do_##Name
>        STEP0_3_TABLE;
>        STEP4_TABLE;
>  
> @@ -1928,16 +1925,6 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
>    /* Now walk through all format specifiers and process them.  */
>    for (; (size_t) nspecs_done < nspecs; ++nspecs_done)
>      {
> -#undef REF
> -#ifdef SHARED
> -# undef JUMP_TABLE_BASE_LABEL
> -# define JUMP_TABLE_BASE_LABEL do2_form_unknown
> -# define REF(Name) &&do2_##Name - &&JUMP_TABLE_BASE_LABEL
> -#else
> -# define REF(Name) &&do2_##Name
> -#endif
> -#undef LABEL
> -#define LABEL(Name) do2_##Name
>        STEP4_TABLE;
>  
>        int is_negative;
> 

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Carlos O'Donell]

On 03/06/2015 07:57 AM, Florian Weimer wrote:
> On 03/01/2015 10:16 PM, Florian Weimer wrote:
>> This splits a considerable chunk of code from the main vfprintf
>> function.  This will make it easier to remove the use of extend_alloca
>> from the positional argument handling code.
> 
> Inspection of the generated assembly on x86_64 shows that splitting the
> two functions helps GCC 4.9 with register allocation; there are fewer
> spills.
> 
> I used the following totally made-up benchmark to see if there is a
> performance regression.

I converted your made-up benchmark into a benchtest test.

Care to test this with your patches and see if it shows a difference?

I'd say if it shows no difference, then checkin your changes, and I'll
checkin this benchmark to prevent regressions in sprintf, despite it
being trivial.

Example results before were (i5-4690K):
  "sprintf": {
   "": {
    "duration": 3.49974e+10,
    "iterations": 2.162e+07,
    "max": 2077.29,
    "min": 1214.74,
    "mean": 1618.75
   }
  }


2015-05-20  Carlos O'Donell  <carlos@redhat.com>

	* benchtests/Makefile (stdio-common-bench): Define.
	(benchset): Add stdio-common-bench.
	* sprintf-inputs: New file.
	* sprintf-source.c: New file.

diff --git a/benchtests/Makefile b/benchtests/Makefile
index cb7a97e..8e615e5 100644
--- a/benchtests/Makefile
+++ b/benchtests/Makefile
@@ -48,7 +48,9 @@ include ../gen-locales.mk
 
 stdlib-bench := strtod
 
-benchset := $(string-bench-all) $(stdlib-bench)
+stdio-common-bench := sprintf
+
+benchset := $(string-bench-all) $(stdlib-bench) $(stdio-common-bench)
 
 CFLAGS-bench-ffs.c += -fno-builtin
 CFLAGS-bench-ffsll.c += -fno-builtin
diff --git a/benchtests/sprintf-inputs b/benchtests/sprintf-inputs
new file mode 100644
index 0000000..0e034b5
--- /dev/null
+++ b/benchtests/sprintf-inputs
@@ -0,0 +1,8 @@
+## args: char *:const char *:int:char:char:char:char:char:const char *:float:unsigned int
+## ret: int
+## includes: stdio.h
+## include-sources: sprintf-source.c
+# Test positional arguments:
+buf, FORMAT1, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
+# Test non-positional arguments:
+buf, FORMAT2, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
diff --git a/benchtests/sprintf-source.c b/benchtests/sprintf-source.c
new file mode 100644
index 0000000..fc125a5
--- /dev/null
+++ b/benchtests/sprintf-source.c
@@ -0,0 +1,6 @@
+/* A set of arbitrarily selected positional format specifiers.  */
+#define FORMAT1 "   %1$d: %2$c%3$c%4$c%5$c%6$c %7$20s %8$f (%9$02x)\n"
+/* A matching, but arbitrarily selected, non-positional format specifiers.  */
+#define FORMAT2 "   %d: %c%c%c%c%c %20s %f (%02x)\n"
+/* Sufficiently large buffer.  */
+char buf[256];
---

Cheers,
Carlos.

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Carlos O'Donell]

On 03/01/2015 04:16 PM, Florian Weimer wrote:
> This splits a considerable chunk of code from the main vfprintf
> function.  This will make it easier to remove the use of extend_alloca
> from the positional argument handling code.
> ---
>  stdio-common/vfprintf.c | 716 +++++++++++++++++++++++++-----------------------
>  1 file changed, 369 insertions(+), 347 deletions(-)

This patch is OK to checkin modulo testing with the new
bench-sprintf I just posted downthread.

> diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
> index da52dbc..6e840aa 100644
> --- a/stdio-common/vfprintf.c
> +++ b/stdio-common/vfprintf.c
> @@ -204,6 +204,14 @@ static const CHAR_T null[] = L_("(null)");
>  static int buffered_vfprintf (FILE *stream, const CHAR_T *fmt, va_list)
>       __THROW __attribute__ ((noinline)) internal_function;
>  
> +/* Handle positional format specifiers.  */
> +static int printf_positional (_IO_FILE *s,
> +			      const CHAR_T *format, int readonly_format,
> +			      va_list ap, va_list *ap_savep, int done,
> +			      int nspecs_done, const UCHAR_T *lead_str_end,
> +			      CHAR_T *work_buffer, int save_errno,
> +			      const char *grouping, THOUSANDS_SEP_T);

OK. That's a lot of arguments, but we've already discussed the performance
impact downthread.

> +
>  /* Handle unknown format specifier.  */
>  static int printf_unknown (FILE *, const struct printf_info *,
>  			   const void *const *) __THROW;
> @@ -1257,15 +1265,6 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
>       0 if unknown.  */
>    int readonly_format = 0;
>  
> -  /* For the argument descriptions, which may be allocated on the heap.  */
> -  void *args_malloced = NULL;
> -
> -  /* For positional argument handling.  */
> -  struct printf_spec *specs;
> -
> -  /* Track if we malloced the SPECS array and thus must free it.  */
> -  bool specs_malloced = false;
> -

OK.

>    /* Orient the stream.  */
>  #ifdef ORIENT
>    ORIENT;
> @@ -1670,232 +1669,265 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
>    /* Unlock stream and return.  */
>    goto all_done;
>  
> -  /* Here starts the more complex loop to handle positional parameters.  */
> +  /* Hand off processing for positional parameters.  */
>  do_positional:
> -  {
> -    /* Array with information about the needed arguments.  This has to
> -       be dynamically extensible.  */
> -    size_t nspecs = 0;
> -    /* A more or less arbitrary start value.  */
> -    size_t nspecs_size = 32 * sizeof (struct printf_spec);
> -
> -    specs = alloca (nspecs_size);
> -    /* The number of arguments the format string requests.  This will
> -       determine the size of the array needed to store the argument
> -       attributes.  */
> -    size_t nargs = 0;
> -    size_t bytes_per_arg;
> -    union printf_arg *args_value;
> -    int *args_size;
> -    int *args_type;
> -
> -    /* Positional parameters refer to arguments directly.  This could
> -       also determine the maximum number of arguments.  Track the
> -       maximum number.  */
> -    size_t max_ref_arg = 0;
> -
> -    /* Just a counter.  */
> -    size_t cnt;
> -
> -    if (__glibc_unlikely (workstart != NULL))
> +  if (__glibc_unlikely (workstart != NULL))
> +    {
>        free (workstart);
> -    workstart = NULL;
> +      workstart = NULL;
> +    }
> +  done = printf_positional (s, format, readonly_format, ap, &ap_save,
> +			    done, nspecs_done, lead_str_end, work_buffer,
> +			    save_errno, grouping, thousands_sep);

OK.

>  
> -    if (grouping == (const char *) -1)
> -      {
> + all_done:
> +  if (__glibc_unlikely (workstart != NULL))
> +    free (workstart);
> +  /* Unlock the stream.  */
> +  _IO_funlockfile (s);
> +  _IO_cleanup_region_end (0);
> +
> +  return done;

OK.

> +}
> +\f
> +static int
> +printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
> +		   va_list ap, va_list *ap_savep, int done, int nspecs_done,
> +		   const UCHAR_T *lead_str_end,
> +		   CHAR_T *work_buffer, int save_errno,
> +		   const char *grouping, THOUSANDS_SEP_T thousands_sep)
> +{
> +  /* For the argument descriptions, which may be allocated on the heap.  */
> +  void *args_malloced = NULL;
> +
> +  /* For positional argument handling.  */
> +  struct printf_spec *specs;
> +
> +  /* Track if we malloced the SPECS array and thus must free it.  */
> +  bool specs_malloced = false;
> +
> +  /* Array with information about the needed arguments.  This has to
> +     be dynamically extensible.  */
> +  size_t nspecs = 0;
> +  /* A more or less arbitrary start value.  */
> +  size_t nspecs_size = 32 * sizeof (struct printf_spec);
> +
> +  specs = alloca (nspecs_size);
> +  /* The number of arguments the format string requests.  This will
> +     determine the size of the array needed to store the argument
> +     attributes.  */
> +  size_t nargs = 0;
> +  size_t bytes_per_arg;
> +  union printf_arg *args_value;
> +  int *args_size;
> +  int *args_type;
> +
> +  /* Positional parameters refer to arguments directly.  This could
> +     also determine the maximum number of arguments.  Track the
> +     maximum number.  */
> +  size_t max_ref_arg = 0;
> +
> +  /* Just a counter.  */
> +  size_t cnt;
> +
> +  CHAR_T *workstart = NULL;
> +
> +  if (grouping == (const char *) -1)
> +    {

OK.

>  #ifdef COMPILE_WPRINTF
> -	thousands_sep = _NL_CURRENT_WORD (LC_NUMERIC,
> -					  _NL_NUMERIC_THOUSANDS_SEP_WC);
> +      thousands_sep = _NL_CURRENT_WORD (LC_NUMERIC,
> +					_NL_NUMERIC_THOUSANDS_SEP_WC);
>  #else
> -	thousands_sep = _NL_CURRENT (LC_NUMERIC, THOUSANDS_SEP);
> +      thousands_sep = _NL_CURRENT (LC_NUMERIC, THOUSANDS_SEP);
>  #endif
>  
> -	grouping = _NL_CURRENT (LC_NUMERIC, GROUPING);
> -	if (*grouping == '\0' || *grouping == CHAR_MAX)
> -	  grouping = NULL;
> -      }
> +      grouping = _NL_CURRENT (LC_NUMERIC, GROUPING);
> +      if (*grouping == '\0' || *grouping == CHAR_MAX)
> +	grouping = NULL;
> +    }
>  

OK.

> -    for (f = lead_str_end; *f != L_('\0'); f = specs[nspecs++].next_fmt)
> -      {
> -	if (nspecs * sizeof (*specs) >= nspecs_size)
> -	  {
> -	    /* Extend the array of format specifiers.  */
> -	    if (nspecs_size * 2 < nspecs_size)
> -	      {
> -		__set_errno (ENOMEM);
> -		done = -1;
> -		goto all_done;
> -	      }
> -	    struct printf_spec *old = specs;
> -	    if (__libc_use_alloca (2 * nspecs_size))
> -	      specs = extend_alloca (specs, nspecs_size, 2 * nspecs_size);
> -	    else
> -	      {
> -		nspecs_size *= 2;
> -		specs = malloc (nspecs_size);
> -		if (specs == NULL)
> -		  {
> -		    __set_errno (ENOMEM);
> -		    specs = old;
> -		    done = -1;
> -		    goto all_done;
> -		  }
> -	      }
> +  for (const UCHAR_T *f = lead_str_end; *f != L_('\0');
> +       f = specs[nspecs++].next_fmt)
> +    {
> +      if (nspecs * sizeof (*specs) >= nspecs_size)
> +	{
> +	  /* Extend the array of format specifiers.  */
> +	  if (nspecs_size * 2 < nspecs_size)
> +	    {
> +	      __set_errno (ENOMEM);
> +	      done = -1;
> +	      goto all_done;
> +	    }
> +	  struct printf_spec *old = specs;
> +	  if (__libc_use_alloca (2 * nspecs_size))
> +	    specs = extend_alloca (specs, nspecs_size, 2 * nspecs_size);
> +	  else
> +	    {
> +	      nspecs_size *= 2;
> +	      specs = malloc (nspecs_size);
> +	      if (specs == NULL)
> +		{
> +		  __set_errno (ENOMEM);
> +		  specs = old;
> +		  done = -1;
> +		  goto all_done;
> +		}

OK.

> +	    }
>  
> -	    /* Copy the old array's elements to the new space.  */
> -	    memmove (specs, old, nspecs * sizeof (*specs));
> +	  /* Copy the old array's elements to the new space.  */
> +	  memmove (specs, old, nspecs * sizeof (*specs));
>  
> -	    /* If we had previously malloc'd space for SPECS, then
> -	       release it after the copy is complete.  */
> -	    if (specs_malloced)
> -	      free (old);
> +	  /* If we had previously malloc'd space for SPECS, then
> +	     release it after the copy is complete.  */
> +	  if (specs_malloced)
> +	    free (old);
>  
> -	    /* Now set SPECS_MALLOCED if needed.  */
> -	    if (!__libc_use_alloca (nspecs_size))
> -	      specs_malloced = true;
> -	  }
> +	  /* Now set SPECS_MALLOCED if needed.  */
> +	  if (!__libc_use_alloca (nspecs_size))
> +	    specs_malloced = true;

OK.

> +	}
>  
> -	/* Parse the format specifier.  */
> +      /* Parse the format specifier.  */
>  #ifdef COMPILE_WPRINTF
> -	nargs += __parse_one_specwc (f, nargs, &specs[nspecs], &max_ref_arg);
> +      nargs += __parse_one_specwc (f, nargs, &specs[nspecs], &max_ref_arg);
>  #else
> -	nargs += __parse_one_specmb (f, nargs, &specs[nspecs], &max_ref_arg);
> +      nargs += __parse_one_specmb (f, nargs, &specs[nspecs], &max_ref_arg);
>  #endif
> -      }
> -
> -    /* Determine the number of arguments the format string consumes.  */
> -    nargs = MAX (nargs, max_ref_arg);
> -    /* Calculate total size needed to represent a single argument across
> -       all three argument-related arrays.  */
> -    bytes_per_arg = (sizeof (*args_value) + sizeof (*args_size)
> -		     + sizeof (*args_type));
> +    }
>  
> -    /* Check for potential integer overflow.  */
> -    if (__glibc_unlikely (nargs > INT_MAX / bytes_per_arg))
> -      {
> -	 __set_errno (EOVERFLOW);
> -	 done = -1;
> -	 goto all_done;
> -      }
> +  /* Determine the number of arguments the format string consumes.  */
> +  nargs = MAX (nargs, max_ref_arg);
> +  /* Calculate total size needed to represent a single argument across
> +     all three argument-related arrays.  */
> +  bytes_per_arg = (sizeof (*args_value) + sizeof (*args_size)
> +		   + sizeof (*args_type));

OK.

>  
> -    /* Allocate memory for all three argument arrays.  */
> -    if (__libc_use_alloca (nargs * bytes_per_arg))
> -	args_value = alloca (nargs * bytes_per_arg);
> -    else
> -      {
> -	args_value = args_malloced = malloc (nargs * bytes_per_arg);
> -	if (args_value == NULL)
> -	  {
> -	    done = -1;
> -	    goto all_done;
> -	  }
> -      }
> +  /* Check for potential integer overflow.  */
> +  if (__glibc_unlikely (nargs > INT_MAX / bytes_per_arg))
> +    {
> +      __set_errno (EOVERFLOW);
> +      done = -1;
> +      goto all_done;
> +    }

OK.

>  
> -    /* Set up the remaining two arrays to each point past the end of the
> -       prior array, since space for all three has been allocated now.  */
> -    args_size = &args_value[nargs].pa_int;
> -    args_type = &args_size[nargs];
> -    memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
> -	    nargs * sizeof (*args_type));
> +  /* Allocate memory for all three argument arrays.  */
> +  if (__libc_use_alloca (nargs * bytes_per_arg))
> +    args_value = alloca (nargs * bytes_per_arg);
> +  else
> +    {
> +      args_value = args_malloced = malloc (nargs * bytes_per_arg);
> +      if (args_value == NULL)
> +	{
> +	  done = -1;
> +	  goto all_done;
> +	}
> +    }

OK.

>  
> -    /* XXX Could do sanity check here: If any element in ARGS_TYPE is
> -       still zero after this loop, format is invalid.  For now we
> -       simply use 0 as the value.  */
> +  /* Set up the remaining two arrays to each point past the end of the
> +     prior array, since space for all three has been allocated now.  */
> +  args_size = &args_value[nargs].pa_int;
> +  args_type = &args_size[nargs];
> +  memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
> +	  nargs * sizeof (*args_type));
>  
> -    /* Fill in the types of all the arguments.  */
> -    for (cnt = 0; cnt < nspecs; ++cnt)
> -      {
> -	/* If the width is determined by an argument this is an int.  */
> -	if (specs[cnt].width_arg != -1)
> -	  args_type[specs[cnt].width_arg] = PA_INT;
> +  /* XXX Could do sanity check here: If any element in ARGS_TYPE is
> +     still zero after this loop, format is invalid.  For now we
> +     simply use 0 as the value.  */
>  
> -	/* If the precision is determined by an argument this is an int.  */
> -	if (specs[cnt].prec_arg != -1)
> -	  args_type[specs[cnt].prec_arg] = PA_INT;
> +  /* Fill in the types of all the arguments.  */
> +  for (cnt = 0; cnt < nspecs; ++cnt)
> +    {
> +      /* If the width is determined by an argument this is an int.  */
> +      if (specs[cnt].width_arg != -1)
> +	args_type[specs[cnt].width_arg] = PA_INT;
>  
> -	switch (specs[cnt].ndata_args)
> -	  {
> -	  case 0:		/* No arguments.  */
> -	    break;
> -	  case 1:		/* One argument; we already have the
> -				   type and size.  */
> -	    args_type[specs[cnt].data_arg] = specs[cnt].data_arg_type;
> -	    args_size[specs[cnt].data_arg] = specs[cnt].size;
> -	    break;
> -	  default:
> -	    /* We have more than one argument for this format spec.
> -	       We must call the arginfo function again to determine
> -	       all the types.  */
> -	    (void) (*__printf_arginfo_table[specs[cnt].info.spec])
> -	      (&specs[cnt].info,
> -	       specs[cnt].ndata_args, &args_type[specs[cnt].data_arg],
> -	       &args_size[specs[cnt].data_arg]);
> -	    break;
> -	  }
> -      }
> +      /* If the precision is determined by an argument this is an int.  */
> +      if (specs[cnt].prec_arg != -1)
> +	args_type[specs[cnt].prec_arg] = PA_INT;
>  
> -    /* Now we know all the types and the order.  Fill in the argument
> -       values.  */
> -    for (cnt = 0; cnt < nargs; ++cnt)
> -      switch (args_type[cnt])
> +      switch (specs[cnt].ndata_args)
>  	{
> -#define T(tag, mem, type)						      \
> -	case tag:							      \
> -	  args_value[cnt].mem = va_arg (ap_save, type);			      \
> +	case 0:		/* No arguments.  */
> +	  break;
> +	case 1:		/* One argument; we already have the
> +			   type and size.  */
> +	  args_type[specs[cnt].data_arg] = specs[cnt].data_arg_type;
> +	  args_size[specs[cnt].data_arg] = specs[cnt].size;
> +	  break;
> +	default:
> +	  /* We have more than one argument for this format spec.
> +	     We must call the arginfo function again to determine
> +	     all the types.  */
> +	  (void) (*__printf_arginfo_table[specs[cnt].info.spec])
> +	    (&specs[cnt].info,
> +	     specs[cnt].ndata_args, &args_type[specs[cnt].data_arg],
> +	     &args_size[specs[cnt].data_arg]);
> +	  break;
> +	}
> +    }
> +

OK.

> +  /* Now we know all the types and the order.  Fill in the argument
> +     values.  */
> +  for (cnt = 0; cnt < nargs; ++cnt)
> +    switch (args_type[cnt])
> +      {
> +#define T(tag, mem, type)				\
> +	case tag:					\
> +	  args_value[cnt].mem = va_arg (*ap_savep, type); \
>  	  break
>  
>  	T (PA_WCHAR, pa_wchar, wint_t);
> -	case PA_CHAR:				/* Promoted.  */
> -	case PA_INT|PA_FLAG_SHORT:		/* Promoted.  */
> +      case PA_CHAR:				/* Promoted.  */
> +      case PA_INT|PA_FLAG_SHORT:		/* Promoted.  */
>  #if LONG_MAX == INT_MAX
> -	case PA_INT|PA_FLAG_LONG:
> +      case PA_INT|PA_FLAG_LONG:
>  #endif
>  	T (PA_INT, pa_int, int);
>  #if LONG_MAX == LONG_LONG_MAX
> -	case PA_INT|PA_FLAG_LONG:
> +      case PA_INT|PA_FLAG_LONG:
>  #endif
>  	T (PA_INT|PA_FLAG_LONG_LONG, pa_long_long_int, long long int);
>  #if LONG_MAX != INT_MAX && LONG_MAX != LONG_LONG_MAX
>  # error "he?"
>  #endif
> -	case PA_FLOAT:				/* Promoted.  */
> +      case PA_FLOAT:				/* Promoted.  */
>  	T (PA_DOUBLE, pa_double, double);
> -	case PA_DOUBLE|PA_FLAG_LONG_DOUBLE:
> -	  if (__ldbl_is_dbl)
> -	    {
> -	      args_value[cnt].pa_double = va_arg (ap_save, double);
> -	      args_type[cnt] &= ~PA_FLAG_LONG_DOUBLE;
> -	    }
> -	  else
> -	    args_value[cnt].pa_long_double = va_arg (ap_save, long double);
> -	  break;
> -	case PA_STRING:				/* All pointers are the same */
> -	case PA_WSTRING:			/* All pointers are the same */
> +      case PA_DOUBLE|PA_FLAG_LONG_DOUBLE:
> +	if (__ldbl_is_dbl)
> +	  {
> +	    args_value[cnt].pa_double = va_arg (*ap_savep, double);
> +	    args_type[cnt] &= ~PA_FLAG_LONG_DOUBLE;
> +	  }
> +	else
> +	  args_value[cnt].pa_long_double = va_arg (*ap_savep, long double);
> +	break;
> +      case PA_STRING:				/* All pointers are the same */
> +      case PA_WSTRING:			/* All pointers are the same */
>  	T (PA_POINTER, pa_pointer, void *);
>  #undef T
> -	default:
> -	  if ((args_type[cnt] & PA_FLAG_PTR) != 0)
> -	    args_value[cnt].pa_pointer = va_arg (ap_save, void *);
> -	  else if (__glibc_unlikely (__printf_va_arg_table != NULL)
> -		   && __printf_va_arg_table[args_type[cnt] - PA_LAST] != NULL)
> -	    {
> -	      args_value[cnt].pa_user = alloca (args_size[cnt]);
> -	      (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
> -		(args_value[cnt].pa_user, &ap_save);
> -	    }
> -	  else
> -	    args_value[cnt].pa_long_double = 0.0;
> -	  break;
> -	case -1:
> -	  /* Error case.  Not all parameters appear in N$ format
> -	     strings.  We have no way to determine their type.  */
> -	  assert (s->_flags2 & _IO_FLAGS2_FORTIFY);
> -	  __libc_fatal ("*** invalid %N$ use detected ***\n");
> -	}
> +      default:
> +	if ((args_type[cnt] & PA_FLAG_PTR) != 0)
> +	  args_value[cnt].pa_pointer = va_arg (*ap_savep, void *);
> +	else if (__glibc_unlikely (__printf_va_arg_table != NULL)
> +		 && __printf_va_arg_table[args_type[cnt] - PA_LAST] != NULL)
> +	  {
> +	    args_value[cnt].pa_user = alloca (args_size[cnt]);
> +	    (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
> +	      (args_value[cnt].pa_user, ap_savep);
> +	  }
> +	else
> +	  args_value[cnt].pa_long_double = 0.0;
> +	break;
> +      case -1:
> +	/* Error case.  Not all parameters appear in N$ format
> +	   strings.  We have no way to determine their type.  */
> +	assert (s->_flags2 & _IO_FLAGS2_FORTIFY);
> +	__libc_fatal ("*** invalid %N$ use detected ***\n");
> +      }
>  
> -    /* Now walk through all format specifiers and process them.  */
> -    for (; (size_t) nspecs_done < nspecs; ++nspecs_done)
> -      {
> +  /* Now walk through all format specifiers and process them.  */
> +  for (; (size_t) nspecs_done < nspecs; ++nspecs_done)
> +    {
>  #undef REF
>  #ifdef SHARED
>  # undef JUMP_TABLE_BASE_LABEL
> @@ -1906,184 +1938,174 @@ do_positional:
>  #endif
>  #undef LABEL
>  #define LABEL(Name) do2_##Name
> -	STEP4_TABLE;
> +      STEP4_TABLE;
>  
> -	int is_negative;
> -	union
> +      int is_negative;
> +      union
> +      {
> +	unsigned long long int longlong;
> +	unsigned long int word;
> +      } number;
> +      int base;
> +      union printf_arg the_arg;
> +      CHAR_T *string;		/* Pointer to argument string.  */
> +
> +      /* Fill variables from values in struct.  */
> +      int alt = specs[nspecs_done].info.alt;
> +      int space = specs[nspecs_done].info.space;
> +      int left = specs[nspecs_done].info.left;
> +      int showsign = specs[nspecs_done].info.showsign;
> +      int group = specs[nspecs_done].info.group;
> +      int is_long_double = specs[nspecs_done].info.is_long_double;
> +      int is_short = specs[nspecs_done].info.is_short;
> +      int is_char = specs[nspecs_done].info.is_char;
> +      int is_long = specs[nspecs_done].info.is_long;
> +      int width = specs[nspecs_done].info.width;
> +      int prec = specs[nspecs_done].info.prec;
> +      int use_outdigits = specs[nspecs_done].info.i18n;
> +      char pad = specs[nspecs_done].info.pad;
> +      CHAR_T spec = specs[nspecs_done].info.spec;
> +
> +      workstart = NULL;
> +      CHAR_T *workend = work_buffer + WORK_BUFFER_SIZE;
> +
> +      /* Fill in last information.  */
> +      if (specs[nspecs_done].width_arg != -1)
>  	{
> -	  unsigned long long int longlong;
> -	  unsigned long int word;
> -	} number;
> -	int base;
> -	union printf_arg the_arg;
> -	CHAR_T *string;		/* Pointer to argument string.  */
> -
> -	/* Fill variables from values in struct.  */
> -	int alt = specs[nspecs_done].info.alt;
> -	int space = specs[nspecs_done].info.space;
> -	int left = specs[nspecs_done].info.left;
> -	int showsign = specs[nspecs_done].info.showsign;
> -	int group = specs[nspecs_done].info.group;
> -	int is_long_double = specs[nspecs_done].info.is_long_double;
> -	int is_short = specs[nspecs_done].info.is_short;
> -	int is_char = specs[nspecs_done].info.is_char;
> -	int is_long = specs[nspecs_done].info.is_long;
> -	int width = specs[nspecs_done].info.width;
> -	int prec = specs[nspecs_done].info.prec;
> -	int use_outdigits = specs[nspecs_done].info.i18n;
> -	char pad = specs[nspecs_done].info.pad;
> -	CHAR_T spec = specs[nspecs_done].info.spec;
> -
> -	workstart = NULL;
> -	workend = work_buffer + WORK_BUFFER_SIZE;
> -
> -	/* Fill in last information.  */
> -	if (specs[nspecs_done].width_arg != -1)
> -	  {
> -	    /* Extract the field width from an argument.  */
> -	    specs[nspecs_done].info.width =
> -	      args_value[specs[nspecs_done].width_arg].pa_int;
> +	  /* Extract the field width from an argument.  */
> +	  specs[nspecs_done].info.width =
> +	    args_value[specs[nspecs_done].width_arg].pa_int;
>  
> -	    if (specs[nspecs_done].info.width < 0)
> -	      /* If the width value is negative left justification is
> -		 selected and the value is taken as being positive.  */
> -	      {
> -		specs[nspecs_done].info.width *= -1;
> -		left = specs[nspecs_done].info.left = 1;
> -	      }
> -	    width = specs[nspecs_done].info.width;
> -	  }
> +	  if (specs[nspecs_done].info.width < 0)
> +	    /* If the width value is negative left justification is
> +	       selected and the value is taken as being positive.  */
> +	    {
> +	      specs[nspecs_done].info.width *= -1;
> +	      left = specs[nspecs_done].info.left = 1;
> +	    }
> +	  width = specs[nspecs_done].info.width;
> +	}
>  
> -	if (specs[nspecs_done].prec_arg != -1)
> -	  {
> -	    /* Extract the precision from an argument.  */
> -	    specs[nspecs_done].info.prec =
> -	      args_value[specs[nspecs_done].prec_arg].pa_int;
> +      if (specs[nspecs_done].prec_arg != -1)
> +	{
> +	  /* Extract the precision from an argument.  */
> +	  specs[nspecs_done].info.prec =
> +	    args_value[specs[nspecs_done].prec_arg].pa_int;
>  
> -	    if (specs[nspecs_done].info.prec < 0)
> -	      /* If the precision is negative the precision is
> -		 omitted.  */
> -	      specs[nspecs_done].info.prec = -1;
> +	  if (specs[nspecs_done].info.prec < 0)
> +	    /* If the precision is negative the precision is
> +	       omitted.  */
> +	    specs[nspecs_done].info.prec = -1;
>  
> -	    prec = specs[nspecs_done].info.prec;
> -	  }
> +	  prec = specs[nspecs_done].info.prec;
> +	}
>  
> -	/* Maybe the buffer is too small.  */
> -	if (MAX (prec, width) + 32 > WORK_BUFFER_SIZE)
> -	  {
> -	    if (__libc_use_alloca ((MAX (prec, width) + 32)
> -				   * sizeof (CHAR_T)))
> -	      workend = ((CHAR_T *) alloca ((MAX (prec, width) + 32)
> -					    * sizeof (CHAR_T))
> -			 + (MAX (prec, width) + 32));
> -	    else
> -	      {
> -		workstart = (CHAR_T *) malloc ((MAX (prec, width) + 32)
> -					       * sizeof (CHAR_T));
> -		if (workstart == NULL)
> -		  {
> -		    done = -1;
> -		    goto all_done;
> -		  }
> -		workend = workstart + (MAX (prec, width) + 32);
> -	      }
> -	  }
> +      /* Maybe the buffer is too small.  */
> +      if (MAX (prec, width) + 32 > WORK_BUFFER_SIZE)
> +	{
> +	  if (__libc_use_alloca ((MAX (prec, width) + 32)
> +				 * sizeof (CHAR_T)))
> +	    workend = ((CHAR_T *) alloca ((MAX (prec, width) + 32)
> +					  * sizeof (CHAR_T))
> +		       + (MAX (prec, width) + 32));
> +	  else
> +	    {
> +	      workstart = (CHAR_T *) malloc ((MAX (prec, width) + 32)
> +					     * sizeof (CHAR_T));
> +	      if (workstart == NULL)
> +		{
> +		  done = -1;
> +		  goto all_done;
> +		}
> +	      workend = workstart + (MAX (prec, width) + 32);
> +	    }
> +	}
>  
> -	/* Process format specifiers.  */
> -	while (1)
> -	  {
> -	    extern printf_function **__printf_function_table;
> -	    int function_done;
> +      /* Process format specifiers.  */
> +      while (1)
> +	{
> +	  extern printf_function **__printf_function_table;
> +	  int function_done;
>  
> -	    if (spec <= UCHAR_MAX
> -		&& __printf_function_table != NULL
> -		&& __printf_function_table[(size_t) spec] != NULL)
> -	      {
> -		const void **ptr = alloca (specs[nspecs_done].ndata_args
> -					   * sizeof (const void *));
> +	  if (spec <= UCHAR_MAX
> +	      && __printf_function_table != NULL
> +	      && __printf_function_table[(size_t) spec] != NULL)
> +	    {
> +	      const void **ptr = alloca (specs[nspecs_done].ndata_args
> +					 * sizeof (const void *));
>  
> -		/* Fill in an array of pointers to the argument values.  */
> -		for (unsigned int i = 0; i < specs[nspecs_done].ndata_args;
> -		     ++i)
> -		  ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
> +	      /* Fill in an array of pointers to the argument values.  */
> +	      for (unsigned int i = 0; i < specs[nspecs_done].ndata_args;
> +		   ++i)
> +		ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
>  
> -		/* Call the function.  */
> -		function_done = __printf_function_table[(size_t) spec]
> -		  (s, &specs[nspecs_done].info, ptr);
> +	      /* Call the function.  */
> +	      function_done = __printf_function_table[(size_t) spec]
> +		(s, &specs[nspecs_done].info, ptr);
>  
> -		if (function_done != -2)
> -		  {
> -		    /* If an error occurred we don't have information
> -		       about # of chars.  */
> -		    if (function_done < 0)
> -		      {
> -			/* Function has set errno.  */
> -			done = -1;
> -			goto all_done;
> -		      }
> -
> -		    done_add (function_done);
> -		    break;
> -		  }
> -	      }
> +	      if (function_done != -2)
> +		{
> +		  /* If an error occurred we don't have information
> +		     about # of chars.  */
> +		  if (function_done < 0)
> +		    {
> +		      /* Function has set errno.  */
> +		      done = -1;
> +		      goto all_done;
> +		    }
> +
> +		  done_add (function_done);
> +		  break;
> +		}
> +	    }
>  
> -	    JUMP (spec, step4_jumps);
> +	  JUMP (spec, step4_jumps);
>  
> -	    process_arg ((&specs[nspecs_done]));
> -	    process_string_arg ((&specs[nspecs_done]));
> +	  process_arg ((&specs[nspecs_done]));
> +	  process_string_arg ((&specs[nspecs_done]));
>  
>  	  LABEL (form_unknown):
> -	    {
> -	      unsigned int i;
> -	      const void **ptr;
> +	  {
> +	    unsigned int i;
> +	    const void **ptr;
>  
> -	      ptr = alloca (specs[nspecs_done].ndata_args
> -			    * sizeof (const void *));
> +	    ptr = alloca (specs[nspecs_done].ndata_args
> +			  * sizeof (const void *));
>  
> -	      /* Fill in an array of pointers to the argument values.  */
> -	      for (i = 0; i < specs[nspecs_done].ndata_args; ++i)
> -		ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
> +	    /* Fill in an array of pointers to the argument values.  */
> +	    for (i = 0; i < specs[nspecs_done].ndata_args; ++i)
> +	      ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
>  
> -	      /* Call the function.  */
> -	      function_done = printf_unknown (s, &specs[nspecs_done].info,
> -					      ptr);
> +	    /* Call the function.  */
> +	    function_done = printf_unknown (s, &specs[nspecs_done].info,
> +					    ptr);
>  
> -	      /* If an error occurred we don't have information about #
> -		 of chars.  */
> -	      if (function_done < 0)
> -		{
> -		  /* Function has set errno.  */
> -		  done = -1;
> -		  goto all_done;
> -		}
> +	    /* If an error occurred we don't have information about #
> +	       of chars.  */
> +	    if (function_done < 0)
> +	      {
> +		/* Function has set errno.  */
> +		done = -1;
> +		goto all_done;
> +	      }
>  
> -	      done_add (function_done);
> -	    }
> -	    break;
> +	    done_add (function_done);
>  	  }
> +	  break;
> +	}
>  
> -	if (__glibc_unlikely (workstart != NULL))
> -	  free (workstart);
> -	workstart = NULL;
> -
> -	/* Write the following constant string.  */
> -	outstring (specs[nspecs_done].end_of_fmt,
> -		   specs[nspecs_done].next_fmt
> -		   - specs[nspecs_done].end_of_fmt);
> -      }
> -  }
> +      if (__glibc_unlikely (workstart != NULL))
> +	free (workstart);
> +      workstart = NULL;
>  
> -all_done:
> -  if (specs_malloced)
> -    free (specs);
> -  if (__glibc_unlikely (args_malloced != NULL))
> -    free (args_malloced);
> +      /* Write the following constant string.  */
> +      outstring (specs[nspecs_done].end_of_fmt,
> +		 specs[nspecs_done].next_fmt
> +		 - specs[nspecs_done].end_of_fmt);
> +    }
> + all_done:
>    if (__glibc_unlikely (workstart != NULL))
>      free (workstart);
> -  /* Unlock the stream.  */
> -  _IO_funlockfile (s);
> -  _IO_cleanup_region_end (0);
> -

OK.

>    return done;
>  }
>  \f
> 

Cheers,
Carlos.

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Siddhesh Poyarekar]

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

On Wed, May 20, 2015 at 11:40:17PM -0400, Carlos O'Donell wrote:
> 2015-05-20  Carlos O'Donell  <carlos@redhat.com>
> 
> 	* benchtests/Makefile (stdio-common-bench): Define.
> 	(benchset): Add stdio-common-bench.
> 	* sprintf-inputs: New file.
> 	* sprintf-source.c: New file.
> 
> diff --git a/benchtests/Makefile b/benchtests/Makefile
> index cb7a97e..8e615e5 100644
> --- a/benchtests/Makefile
> +++ b/benchtests/Makefile
> @@ -48,7 +48,9 @@ include ../gen-locales.mk
>  
>  stdlib-bench := strtod
>  
> -benchset := $(string-bench-all) $(stdlib-bench)
> +stdio-common-bench := sprintf
> +
> +benchset := $(string-bench-all) $(stdlib-bench) $(stdio-common-bench)
>  
>  CFLAGS-bench-ffs.c += -fno-builtin
>  CFLAGS-bench-ffsll.c += -fno-builtin
> diff --git a/benchtests/sprintf-inputs b/benchtests/sprintf-inputs
> new file mode 100644
> index 0000000..0e034b5
> --- /dev/null
> +++ b/benchtests/sprintf-inputs
> @@ -0,0 +1,8 @@
> +## args: char *:const char *:int:char:char:char:char:char:const char *:float:unsigned int
> +## ret: int
> +## includes: stdio.h
> +## include-sources: sprintf-source.c
> +# Test positional arguments:
> +buf, FORMAT1, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
> +# Test non-positional arguments:
> +buf, FORMAT2, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234

You would want two different outputs for positional and non-positional
respectively to match Florian's output.  You can achieve that with the
##name directive by putting the line:

##name: positional

and

##name: nonpositional

before each of the inputs.

> diff --git a/benchtests/sprintf-source.c b/benchtests/sprintf-source.c
> new file mode 100644
> index 0000000..fc125a5
> --- /dev/null
> +++ b/benchtests/sprintf-source.c
> @@ -0,0 +1,6 @@
> +/* A set of arbitrarily selected positional format specifiers.  */
> +#define FORMAT1 "   %1$d: %2$c%3$c%4$c%5$c%6$c %7$20s %8$f (%9$02x)\n"
> +/* A matching, but arbitrarily selected, non-positional format specifiers.  */
> +#define FORMAT2 "   %d: %c%c%c%c%c %20s %f (%02x)\n"
> +/* Sufficiently large buffer.  */
> +char buf[256];
> ---
> 
> Cheers,
> Carlos.
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Carlos O'Donell]

On 05/21/2015 12:16 AM, Siddhesh Poyarekar wrote:
> On Wed, May 20, 2015 at 11:40:17PM -0400, Carlos O'Donell wrote:
>> 2015-05-20  Carlos O'Donell  <carlos@redhat.com>
>>
>> 	* benchtests/Makefile (stdio-common-bench): Define.
>> 	(benchset): Add stdio-common-bench.
>> 	* sprintf-inputs: New file.
>> 	* sprintf-source.c: New file.
>>
>> diff --git a/benchtests/Makefile b/benchtests/Makefile
>> index cb7a97e..8e615e5 100644
>> --- a/benchtests/Makefile
>> +++ b/benchtests/Makefile
>> @@ -48,7 +48,9 @@ include ../gen-locales.mk
>>  
>>  stdlib-bench := strtod
>>  
>> -benchset := $(string-bench-all) $(stdlib-bench)
>> +stdio-common-bench := sprintf
>> +
>> +benchset := $(string-bench-all) $(stdlib-bench) $(stdio-common-bench)
>>  
>>  CFLAGS-bench-ffs.c += -fno-builtin
>>  CFLAGS-bench-ffsll.c += -fno-builtin
>> diff --git a/benchtests/sprintf-inputs b/benchtests/sprintf-inputs
>> new file mode 100644
>> index 0000000..0e034b5
>> --- /dev/null
>> +++ b/benchtests/sprintf-inputs
>> @@ -0,0 +1,8 @@
>> +## args: char *:const char *:int:char:char:char:char:char:const char *:float:unsigned int
>> +## ret: int
>> +## includes: stdio.h
>> +## include-sources: sprintf-source.c
>> +# Test positional arguments:
>> +buf, FORMAT1, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
>> +# Test non-positional arguments:
>> +buf, FORMAT2, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
> 
> You would want two different outputs for positional and non-positional
> respectively to match Florian's output.  You can achieve that with the
> ##name directive by putting the line:

Oh! You're saying that the performance domain of positional versus
non-positional is different and should not be lumped together, as
Florian did not lump them together in his analysis.

v2
- Split out positional vs non-positional.

Test run:

  "sprintf": {
   "positional": {
    "duration": 3.49965e+10,
    "iterations": 1.7381e+07,
    "max": 2733.1,
    "min": 2007.32,
    "mean": 2013.49
   },
   "non-positional": {
    "duration": 3.49927e+10,
    "iterations": 2.8668e+07,
    "max": 1379.96,
    "min": 1214.8,
    "mean": 1220.62
   }
  }

2015-05-21  Carlos O'Donell  <carlos@redhat.com>

	* benchtests/Makefile (stdio-common-bench): Define.
	(benchset): Add stdio-common-bench.
	* sprintf-inputs: New file.
	* sprintf-source.c: New file.

diff --git a/benchtests/Makefile b/benchtests/Makefile
index cb7a97e..8e615e5 100644
--- a/benchtests/Makefile
+++ b/benchtests/Makefile
@@ -48,7 +48,9 @@ include ../gen-locales.mk
 
 stdlib-bench := strtod
 
-benchset := $(string-bench-all) $(stdlib-bench)
+stdio-common-bench := sprintf
+
+benchset := $(string-bench-all) $(stdlib-bench) $(stdio-common-bench)
 
 CFLAGS-bench-ffs.c += -fno-builtin
 CFLAGS-bench-ffsll.c += -fno-builtin
diff --git a/benchtests/sprintf-inputs b/benchtests/sprintf-inputs
new file mode 100644
index 0000000..9a7710d
--- /dev/null
+++ b/benchtests/sprintf-inputs
@@ -0,0 +1,10 @@
+## args: char *:const char *:int:char:char:char:char:char:const char *:float:unsigned int
+## ret: int
+## includes: stdio.h
+## include-sources: sprintf-source.c
+## name: positional
+# Test positional arguments:
+buf, FORMAT1, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
+## name: non-positional
+# Test non-positional arguments:
+buf, FORMAT2, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
diff --git a/benchtests/sprintf-source.c b/benchtests/sprintf-source.c
new file mode 100644
index 0000000..fc125a5
--- /dev/null
+++ b/benchtests/sprintf-source.c
@@ -0,0 +1,6 @@
+/* A set of arbitrarily selected positional format specifiers.  */
+#define FORMAT1 "   %1$d: %2$c%3$c%4$c%5$c%6$c %7$20s %8$f (%9$02x)\n"
+/* A matching, but arbitrarily selected, set of non-positional format specifiers.  */
+#define FORMAT2 "   %d: %c%c%c%c%c %20s %f (%02x)\n"
+/* Sufficiently large buffer.  */
+char buf[256];
---

Cheers,
Carlos.

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Siddhesh Poyarekar]

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

On Thu, May 21, 2015 at 12:55:19AM -0400, Carlos O'Donell wrote:
> Oh! You're saying that the performance domain of positional versus
> non-positional is different and should not be lumped together, as
> Florian did not lump them together in his analysis.
> 
> v2
> - Split out positional vs non-positional.
> 
> Test run:
> 
>   "sprintf": {
>    "positional": {
>     "duration": 3.49965e+10,
>     "iterations": 1.7381e+07,
>     "max": 2733.1,
>     "min": 2007.32,
>     "mean": 2013.49
>    },
>    "non-positional": {
>     "duration": 3.49927e+10,
>     "iterations": 2.8668e+07,
>     "max": 1379.96,
>     "min": 1214.8,
>     "mean": 1220.62
>    }
>   }
> 
> 2015-05-21  Carlos O'Donell  <carlos@redhat.com>
> 
> 	* benchtests/Makefile (stdio-common-bench): Define.
> 	(benchset): Add stdio-common-bench.
> 	* sprintf-inputs: New file.
> 	* sprintf-source.c: New file.

Looks good to me.

Thanks,
Siddhesh

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Florian Weimer]

On 05/21/2015 06:55 AM, Carlos O'Donell wrote:

> Oh! You're saying that the performance domain of positional versus
> non-positional is different and should not be lumped together, as
> Florian did not lump them together in his analysis.

Here's the run with both vfprintf-patched (build) and unpatched
(build-master) sources:

==> build/benchtests/bench-sprintf.out <==
  "sprintf": {
   "positional": {
    "duration": 2.4935e+10,
    "iterations": 1.6674e+07,
    "max": 1724.24,
    "min": 1486.34,
    "mean": 1495.44
   },
   "non-positional": {
    "duration": 2.49332e+10,
    "iterations": 2.5191e+07,
    "max": 1204.71,
    "min": 984.675,
    "mean": 989.767
   }
  }
==> build-master/benchtests/bench-sprintf.out <==
  "sprintf": {
   "positional": {
    "duration": 2.49366e+10,
    "iterations": 1.4544e+07,
    "max": 1948.33,
    "min": 1709.47,
    "mean": 1714.57
   },
   "non-positional": {
    "duration": 2.49334e+10,
    "iterations": 2.3758e+07,
    "max": 1277.08,
    "min": 1042.82,
    "mean": 1049.47
   }
  }

Again, the numbers are suspiciously good.  But at least they do not show
a performance regression.

I'll commit the remaining vfprintf patches then.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Carlos O'Donell]

On 05/21/2015 10:02 AM, Florian Weimer wrote:
> Again, the numbers are suspiciously good.  But at least they do not show
> a performance regression.

Agreed. At least the benchmarks allow interested parties to
test roughly the same test on their hardware and complain if
it made it slower for them. The fact that sprintf got faster
is awesome, and isn't entirely far fetched given the cleanup.

> I'll commit the remaining vfprintf patches then.

Please do. Thanks.

Cheers,
Carlos.
 

Re: [PATCH 5/6] vfprintf: Introduce printf_positional function

[Carlos O'Donell]

On 05/21/2015 12:55 AM, Carlos O'Donell wrote:
> On 05/21/2015 12:16 AM, Siddhesh Poyarekar wrote:
>> On Wed, May 20, 2015 at 11:40:17PM -0400, Carlos O'Donell wrote:
>>> 2015-05-20  Carlos O'Donell  <carlos@redhat.com>
>>>
>>> 	* benchtests/Makefile (stdio-common-bench): Define.
>>> 	(benchset): Add stdio-common-bench.
>>> 	* sprintf-inputs: New file.
>>> 	* sprintf-source.c: New file.
>>>
>>> diff --git a/benchtests/Makefile b/benchtests/Makefile
>>> index cb7a97e..8e615e5 100644
>>> --- a/benchtests/Makefile
>>> +++ b/benchtests/Makefile
>>> @@ -48,7 +48,9 @@ include ../gen-locales.mk
>>>  
>>>  stdlib-bench := strtod
>>>  
>>> -benchset := $(string-bench-all) $(stdlib-bench)
>>> +stdio-common-bench := sprintf
>>> +
>>> +benchset := $(string-bench-all) $(stdlib-bench) $(stdio-common-bench)
>>>  
>>>  CFLAGS-bench-ffs.c += -fno-builtin
>>>  CFLAGS-bench-ffsll.c += -fno-builtin
>>> diff --git a/benchtests/sprintf-inputs b/benchtests/sprintf-inputs
>>> new file mode 100644
>>> index 0000000..0e034b5
>>> --- /dev/null
>>> +++ b/benchtests/sprintf-inputs
>>> @@ -0,0 +1,8 @@
>>> +## args: char *:const char *:int:char:char:char:char:char:const char *:float:unsigned int
>>> +## ret: int
>>> +## includes: stdio.h
>>> +## include-sources: sprintf-source.c
>>> +# Test positional arguments:
>>> +buf, FORMAT1, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
>>> +# Test non-positional arguments:
>>> +buf, FORMAT2, 1001, '1', '2', '3', '4', '5', "string", 1.5, 0x1234
>>
>> You would want two different outputs for positional and non-positional
>> respectively to match Florian's output.  You can achieve that with the
>> ##name directive by putting the line:
> 
> Oh! You're saying that the performance domain of positional versus
> non-positional is different and should not be lumped together, as
> Florian did not lump them together in his analysis.
> 
> v2
> - Split out positional vs non-positional.
> 
> Test run:
> 
>   "sprintf": {
>    "positional": {
>     "duration": 3.49965e+10,
>     "iterations": 1.7381e+07,
>     "max": 2733.1,
>     "min": 2007.32,
>     "mean": 2013.49
>    },
>    "non-positional": {
>     "duration": 3.49927e+10,
>     "iterations": 2.8668e+07,
>     "max": 1379.96,
>     "min": 1214.8,
>     "mean": 1220.62
>    }
>   }
> 
> 2015-05-21  Carlos O'Donell  <carlos@redhat.com>
> 
> 	* benchtests/Makefile (stdio-common-bench): Define.
> 	(benchset): Add stdio-common-bench.
> 	* sprintf-inputs: New file.
> 	* sprintf-source.c: New file.

Checked in.

c.