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

["Carlos O'Donell" <carlos at redhat dot com>]

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.