[5/6] vfprintf: Introduce printf_positional function
diff mbox

Message ID 06448920c54ddf7d92cce8a2311a0daf470436aa.1425246936.git.fweimer@redhat.com
State New
Headers show

Commit Message

Florian Weimer March 1, 2015, 9:16 p.m. UTC
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(-)

Comments

Florian Weimer March 6, 2015, 12:57 p.m. UTC | #1
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
Carlos O'Donell March 6, 2015, 3:43 p.m. UTC | #2
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.
Carlos O'Donell May 21, 2015, 3:40 a.m. UTC | #3
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.

> +}
> +
> +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;
>  }
>  
> 

Cheers,
Carlos.

Patch
diff mbox

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;
+}
+
+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;
 }