[v5] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg

  Changes since v4:

  - Fixed typo in comment (s/macros/macro).
  - Replaced macros with inline functions.

Changes since v3

  - Update after the commit 2d9837c1fbf4 ('Set behavior of sprintf-like
    functions with overlapping source and destination').

Changes since v2:

  - Fixed style error in `do { ... } while (0)' blocks.
  - Zero-initialize args_value[cnt] with memset, rather than relying on
    the `.pa_long_double' member being the largest of the members.

Changes since v1:

  - Updated to the revised and integrated patches for __ldbl_is_dbl
    removal, i.e.: the patches in the following thread:
    <https://sourceware.org/ml/libc-alpha/2018-12/msg00186.html>.
    - Added description for the PRINTF_LDBL_USES_FLOAT128 macro.
    - Removed the LDBL_USES_FLOAT128 macro.
  - Added `do { } while (0)' to the PARSE_FLOAT_VA_ARG_EXTENDED,
    PARSE_FLOAT_VA_ARG, and SETUP_FLOAT128_INFO macros.  Appended
    expansions with `;', accordingly.

-- 8< --
On powerpc64le, long double can currently take two formats: the same as
double (-mlong-double-64) or IBM Extended Precision (default with
-mlong-double-128 or explicitly with -mabi=ibmlongdouble).  The internal
implementation of printf-like functions is aware of these possibilities
and properly parses floating-point values from the variable arguments,
before making calls to __printf_fp and __printf_fphex.  These functions
are also aware of the format possibilities and know how to convert both
formats to string.

When library support for TS 18661-3 was added to glibc, __printf_fp and
__printf_fphex were extended with support for an additional type
(__float128/_Float128) with a different format (binary128).  Now that
powerpc64le is getting support for its third long double format, and
taking into account that this format is the same as the format of
__float128/_Float128, this patch extends __vfprintf_internal to properly
call __printf_fp and __printf_fphex with this new format.

Tested for powerpc64le (with additional patches to actually enable the
use of these preparations) and for x86_64.

	* libio/libioP.h (PRINTF_LDBL_USES_FLOAT128): New macro to be
	used as a mask for the mode argument of __vfprintf_internal.
	* stdio-common/printf-parse.h (printf_arg): New union member:
	pa_float128.
	* stdio-common/vfprintf-internal.c (parse_float_va_arg)
	(setup_float128_info): New functions.
	(process_arg): Use parse_float_va_arg and setup_float128_info.
	[__HAVE_FLOAT128_UNLIKE_LDBL] (printf_positional): Write
	floating-point value to the new union member, pa_float128.
	(printf_positional): Zero-initialize args_value[cnt] with memset.
---
 libio/libioP.h                   | 20 +++++++++++---
 stdio-common/printf-parse.h      |  3 ++
 stdio-common/vfprintf-internal.c | 60 +++++++++++++++++++++++++++++++---------
 3 files changed, 66 insertions(+), 17 deletions(-)
  

Hi,

Any more comments on this patch?  I believe I have addressed all
concerns, so I'm planning to merge it by the end of the week.

Thanks.

On Tue, May 21 2019, Gabriel F. T. Gomes wrote:
> Changes since v4:
> 
>   - Fixed typo in comment (s/macros/macro).
>   - Replaced macros with inline functions.
> 
> Changes since v3
> 
>   - Update after the commit 2d9837c1fbf4 ('Set behavior of sprintf-like
>     functions with overlapping source and destination').
> 
> Changes since v2:
> 
>   - Fixed style error in `do { ... } while (0)' blocks.
>   - Zero-initialize args_value[cnt] with memset, rather than relying on
>     the `.pa_long_double' member being the largest of the members.
> 
> Changes since v1:
> 
>   - Updated to the revised and integrated patches for __ldbl_is_dbl
>     removal, i.e.: the patches in the following thread:
>     <https://sourceware.org/ml/libc-alpha/2018-12/msg00186.html>.
>     - Added description for the PRINTF_LDBL_USES_FLOAT128 macro.
>     - Removed the LDBL_USES_FLOAT128 macro.
>   - Added `do { } while (0)' to the PARSE_FLOAT_VA_ARG_EXTENDED,
>     PARSE_FLOAT_VA_ARG, and SETUP_FLOAT128_INFO macros.  Appended
>     expansions with `;', accordingly.
> 
> -- 8< --
> On powerpc64le, long double can currently take two formats: the same as
> double (-mlong-double-64) or IBM Extended Precision (default with
> -mlong-double-128 or explicitly with -mabi=ibmlongdouble).  The internal
> implementation of printf-like functions is aware of these possibilities
> and properly parses floating-point values from the variable arguments,
> before making calls to __printf_fp and __printf_fphex.  These functions
> are also aware of the format possibilities and know how to convert both
> formats to string.
> 
> When library support for TS 18661-3 was added to glibc, __printf_fp and
> __printf_fphex were extended with support for an additional type
> (__float128/_Float128) with a different format (binary128).  Now that
> powerpc64le is getting support for its third long double format, and
> taking into account that this format is the same as the format of
> __float128/_Float128, this patch extends __vfprintf_internal to properly
> call __printf_fp and __printf_fphex with this new format.
> 
> Tested for powerpc64le (with additional patches to actually enable the
> use of these preparations) and for x86_64.
> 
> 	* libio/libioP.h (PRINTF_LDBL_USES_FLOAT128): New macro to be
> 	used as a mask for the mode argument of __vfprintf_internal.
> 	* stdio-common/printf-parse.h (printf_arg): New union member:
> 	pa_float128.
> 	* stdio-common/vfprintf-internal.c (parse_float_va_arg)
> 	(setup_float128_info): New functions.
> 	(process_arg): Use parse_float_va_arg and setup_float128_info.
> 	[__HAVE_FLOAT128_UNLIKE_LDBL] (printf_positional): Write
> 	floating-point value to the new union member, pa_float128.
> 	(printf_positional): Zero-initialize args_value[cnt] with memset.
> ---
>  libio/libioP.h                   | 20 +++++++++++---
>  stdio-common/printf-parse.h      |  3 ++
>  stdio-common/vfprintf-internal.c | 60 +++++++++++++++++++++++++++++++---------
>  3 files changed, 66 insertions(+), 17 deletions(-)
> 
> diff --git a/libio/libioP.h b/libio/libioP.h
> index 7bdec86a62..8e7e7ff9b3 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -708,10 +708,22 @@ extern int __vswprintf_internal (wchar_t *string, size_t maxlen,
>     defined to 1 or 2.  Otherwise, such checks are ignored.
>  
>     PRINTF_CHK indicates, to the internal function being called, that the
> -   call is originated from one of the __*printf_chk functions.  */
> -#define PRINTF_LDBL_IS_DBL 0x0001
> -#define PRINTF_FORTIFY     0x0002
> -#define PRINTF_CHK	   0x0004
> +   call is originated from one of the __*printf_chk functions.
> +
> +   PRINTF_LDBL_USES_FLOAT128 is used on platforms where the long double
> +   format used to be different from the IEC 60559 double format *and*
> +   also different from the Quadruple 128-bits IEC 60559 format (such as
> +   the IBM Extended Precision format on powerpc or the 80-bits IEC 60559
> +   format on x86), but was later converted to the Quadruple 128-bits IEC
> +   60559 format, which is the same format that the _Float128 always has
> +   (hence the `USES_FLOAT128' suffix in the name of the flag).  When set
> +   to one, this macro indicates that long double values are to be
> +   handled as having this new format.  Otherwise, they should be handled
> +   as the previous format on that platform.  */
> +#define PRINTF_LDBL_IS_DBL		0x0001
> +#define PRINTF_FORTIFY			0x0002
> +#define PRINTF_CHK			0x0004
> +#define PRINTF_LDBL_USES_FLOAT128	0x0008
>  
>  extern size_t _IO_getline (FILE *,char *, size_t, int, int);
>  libc_hidden_proto (_IO_getline)
> diff --git a/stdio-common/printf-parse.h b/stdio-common/printf-parse.h
> index 397508638d..00f3280b8a 100644
> --- a/stdio-common/printf-parse.h
> +++ b/stdio-common/printf-parse.h
> @@ -57,6 +57,9 @@ union printf_arg
>      unsigned long long int pa_u_long_long_int;
>      double pa_double;
>      long double pa_long_double;
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +    _Float128 pa_float128;
> +#endif
>      const char *pa_string;
>      const wchar_t *pa_wstring;
>      void *pa_pointer;
> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
> index ead2b04cb9..1e98208078 100644
> --- a/stdio-common/vfprintf-internal.c
> +++ b/stdio-common/vfprintf-internal.c
> @@ -150,6 +150,42 @@ typedef wchar_t THOUSANDS_SEP_T;
>  /* Include the shared code for parsing the format string.  */
>  #include "printf-parse.h"
>  
> +static void
> +parse_float_va_arg (struct printf_info *info, union printf_arg *the_arg,
> +		    int is_long_double, unsigned int mode_flags,
> +		    va_list *ap)
> +{
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +  if (is_long_double && (mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
> +    {
> +      info->is_binary128 = 1;
> +      the_arg->pa_float128 = va_arg (*ap, _Float128);
> +    }
> +  else
> +#endif
> +    {
> +      info->is_binary128 = 0;
> +      if (is_long_double)
> +	the_arg->pa_long_double = va_arg (*ap, long double);
> +      else
> +	the_arg->pa_double = va_arg (*ap, double);
> +    }
> +}
> +
> +static void
> +setup_float128_info (struct printf_info *info, int is_long_double,
> +		     unsigned int mode_flags)
> +{
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +  if ((mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
> +    info->is_binary128 = is_long_double;
> +  else
> +    info->is_binary128 = 0;
> +#else
> +  info->is_binary128 = 0;
> +#endif
> +}
> +
>  
>  #define	outchar(Ch)							      \
>    do									      \
> @@ -771,10 +807,8 @@ static const uint8_t jump_table[] =
>  					.wide = sizeof (CHAR_T) != 1,	      \
>  					.is_binary128 = 0};		      \
>  									      \
> -	    if (is_long_double)						      \
> -	      the_arg.pa_long_double = va_arg (ap, long double);	      \
> -	    else							      \
> -	      the_arg.pa_double = va_arg (ap, double);			      \
> +	    parse_float_va_arg (&info, &the_arg, is_long_double,	      \
> +				mode_flags, &ap);			      \
>  	    ptr = (const void *) &the_arg;				      \
>  									      \
>  	    function_done = __printf_fp (s, &info, &ptr);		      \
> @@ -787,8 +821,7 @@ static const uint8_t jump_table[] =
>  		fspec->data_arg_type = PA_DOUBLE;			      \
>  		fspec->info.is_long_double = 0;				      \
>  	      }								      \
> -	    /* Not supported by *printf functions.  */			      \
> -	    fspec->info.is_binary128 = 0;				      \
> +	    setup_float128_info (&fspec->info, is_long_double, mode_flags);   \
>  									      \
>  	    function_done = __printf_fp (s, &fspec->info, &ptr);	      \
>  	  }								      \
> @@ -831,10 +864,8 @@ static const uint8_t jump_table[] =
>  					.wide = sizeof (CHAR_T) != 1,	      \
>  					.is_binary128 = 0};		      \
>  									      \
> -	    if (is_long_double)						      \
> -	      the_arg.pa_long_double = va_arg (ap, long double);	      \
> -	    else							      \
> -	      the_arg.pa_double = va_arg (ap, double);			      \
> +	    parse_float_va_arg (&info, &the_arg, is_long_double,	      \
> +				mode_flags, &ap);			      \
>  	    ptr = (const void *) &the_arg;				      \
>  									      \
>  	    function_done = __printf_fphex (s, &info, &ptr);		      \
> @@ -844,8 +875,7 @@ static const uint8_t jump_table[] =
>  	    ptr = (const void *) &args_value[fspec->data_arg];		      \
>  	    if (__glibc_unlikely ((mode_flags & PRINTF_LDBL_IS_DBL) != 0))    \
>  	      fspec->info.is_long_double = 0;				      \
> -	    /* Not supported by *printf functions.  */			      \
> -	    fspec->info.is_binary128 = 0;				      \
> +	    setup_float128_info (&fspec->info, is_long_double, mode_flags);   \
>  									      \
>  	    function_done = __printf_fphex (s, &fspec->info, &ptr);	      \
>  	  }								      \
> @@ -1869,6 +1899,10 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
>  	    args_value[cnt].pa_double = va_arg (*ap_savep, double);
>  	    args_type[cnt] &= ~PA_FLAG_LONG_DOUBLE;
>  	  }
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +	else if ((mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
> +	  args_value[cnt].pa_float128 = va_arg (*ap_savep, _Float128);
> +#endif
>  	else
>  	  args_value[cnt].pa_long_double = va_arg (*ap_savep, long double);
>  	break;
> @@ -1887,7 +1921,7 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
>  	      (args_value[cnt].pa_user, ap_savep);
>  	  }
>  	else
> -	  args_value[cnt].pa_long_double = 0.0;
> +	  memset (&args_value[cnt], 0, sizeof (args_value[cnt]));
>  	break;
>        case -1:
>  	/* Error case.  Not all parameters appear in N$ format
> -- 
> 2.14.5
>
  

LGTM, thanks.

On 05/06/2019 16:08, Gabriel F. T. Gomes wrote:
> Hi,
> 
> Any more comments on this patch?  I believe I have addressed all
> concerns, so I'm planning to merge it by the end of the week.
> 
> Thanks.
> 
> On Tue, May 21 2019, Gabriel F. T. Gomes wrote:
>> Changes since v4:
>>
>>   - Fixed typo in comment (s/macros/macro).
>>   - Replaced macros with inline functions.
>>
>> Changes since v3
>>
>>   - Update after the commit 2d9837c1fbf4 ('Set behavior of sprintf-like
>>     functions with overlapping source and destination').
>>
>> Changes since v2:
>>
>>   - Fixed style error in `do { ... } while (0)' blocks.
>>   - Zero-initialize args_value[cnt] with memset, rather than relying on
>>     the `.pa_long_double' member being the largest of the members.
>>
>> Changes since v1:
>>
>>   - Updated to the revised and integrated patches for __ldbl_is_dbl
>>     removal, i.e.: the patches in the following thread:
>>     <https://sourceware.org/ml/libc-alpha/2018-12/msg00186.html>.
>>     - Added description for the PRINTF_LDBL_USES_FLOAT128 macro.
>>     - Removed the LDBL_USES_FLOAT128 macro.
>>   - Added `do { } while (0)' to the PARSE_FLOAT_VA_ARG_EXTENDED,
>>     PARSE_FLOAT_VA_ARG, and SETUP_FLOAT128_INFO macros.  Appended
>>     expansions with `;', accordingly.
>>
>> -- 8< --
>> On powerpc64le, long double can currently take two formats: the same as
>> double (-mlong-double-64) or IBM Extended Precision (default with
>> -mlong-double-128 or explicitly with -mabi=ibmlongdouble).  The internal
>> implementation of printf-like functions is aware of these possibilities
>> and properly parses floating-point values from the variable arguments,
>> before making calls to __printf_fp and __printf_fphex.  These functions
>> are also aware of the format possibilities and know how to convert both
>> formats to string.
>>
>> When library support for TS 18661-3 was added to glibc, __printf_fp and
>> __printf_fphex were extended with support for an additional type
>> (__float128/_Float128) with a different format (binary128).  Now that
>> powerpc64le is getting support for its third long double format, and
>> taking into account that this format is the same as the format of
>> __float128/_Float128, this patch extends __vfprintf_internal to properly
>> call __printf_fp and __printf_fphex with this new format.
>>
>> Tested for powerpc64le (with additional patches to actually enable the
>> use of these preparations) and for x86_64.
>>
>> 	* libio/libioP.h (PRINTF_LDBL_USES_FLOAT128): New macro to be
>> 	used as a mask for the mode argument of __vfprintf_internal.
>> 	* stdio-common/printf-parse.h (printf_arg): New union member:
>> 	pa_float128.
>> 	* stdio-common/vfprintf-internal.c (parse_float_va_arg)
>> 	(setup_float128_info): New functions.
>> 	(process_arg): Use parse_float_va_arg and setup_float128_info.
>> 	[__HAVE_FLOAT128_UNLIKE_LDBL] (printf_positional): Write
>> 	floating-point value to the new union member, pa_float128.
>> 	(printf_positional): Zero-initialize args_value[cnt] with memset.
>> ---
>>  libio/libioP.h                   | 20 +++++++++++---
>>  stdio-common/printf-parse.h      |  3 ++
>>  stdio-common/vfprintf-internal.c | 60 +++++++++++++++++++++++++++++++---------
>>  3 files changed, 66 insertions(+), 17 deletions(-)
>>
>> diff --git a/libio/libioP.h b/libio/libioP.h
>> index 7bdec86a62..8e7e7ff9b3 100644
>> --- a/libio/libioP.h
>> +++ b/libio/libioP.h
>> @@ -708,10 +708,22 @@ extern int __vswprintf_internal (wchar_t *string, size_t maxlen,
>>     defined to 1 or 2.  Otherwise, such checks are ignored.
>>  
>>     PRINTF_CHK indicates, to the internal function being called, that the
>> -   call is originated from one of the __*printf_chk functions.  */
>> -#define PRINTF_LDBL_IS_DBL 0x0001
>> -#define PRINTF_FORTIFY     0x0002
>> -#define PRINTF_CHK	   0x0004
>> +   call is originated from one of the __*printf_chk functions.
>> +
>> +   PRINTF_LDBL_USES_FLOAT128 is used on platforms where the long double
>> +   format used to be different from the IEC 60559 double format *and*
>> +   also different from the Quadruple 128-bits IEC 60559 format (such as
>> +   the IBM Extended Precision format on powerpc or the 80-bits IEC 60559
>> +   format on x86), but was later converted to the Quadruple 128-bits IEC
>> +   60559 format, which is the same format that the _Float128 always has
>> +   (hence the `USES_FLOAT128' suffix in the name of the flag).  When set
>> +   to one, this macro indicates that long double values are to be
>> +   handled as having this new format.  Otherwise, they should be handled
>> +   as the previous format on that platform.  */
>> +#define PRINTF_LDBL_IS_DBL		0x0001
>> +#define PRINTF_FORTIFY			0x0002
>> +#define PRINTF_CHK			0x0004
>> +#define PRINTF_LDBL_USES_FLOAT128	0x0008
>>  
>>  extern size_t _IO_getline (FILE *,char *, size_t, int, int);
>>  libc_hidden_proto (_IO_getline)
>> diff --git a/stdio-common/printf-parse.h b/stdio-common/printf-parse.h
>> index 397508638d..00f3280b8a 100644
>> --- a/stdio-common/printf-parse.h
>> +++ b/stdio-common/printf-parse.h
>> @@ -57,6 +57,9 @@ union printf_arg
>>      unsigned long long int pa_u_long_long_int;
>>      double pa_double;
>>      long double pa_long_double;
>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>> +    _Float128 pa_float128;
>> +#endif
>>      const char *pa_string;
>>      const wchar_t *pa_wstring;
>>      void *pa_pointer;
>> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
>> index ead2b04cb9..1e98208078 100644
>> --- a/stdio-common/vfprintf-internal.c
>> +++ b/stdio-common/vfprintf-internal.c
>> @@ -150,6 +150,42 @@ typedef wchar_t THOUSANDS_SEP_T;
>>  /* Include the shared code for parsing the format string.  */
>>  #include "printf-parse.h"
>>  
>> +static void
>> +parse_float_va_arg (struct printf_info *info, union printf_arg *the_arg,
>> +		    int is_long_double, unsigned int mode_flags,
>> +		    va_list *ap)
>> +{
>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>> +  if (is_long_double && (mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
>> +    {
>> +      info->is_binary128 = 1;
>> +      the_arg->pa_float128 = va_arg (*ap, _Float128);
>> +    }
>> +  else
>> +#endif
>> +    {
>> +      info->is_binary128 = 0;
>> +      if (is_long_double)
>> +	the_arg->pa_long_double = va_arg (*ap, long double);
>> +      else
>> +	the_arg->pa_double = va_arg (*ap, double);
>> +    }
>> +}
>> +
>> +static void
>> +setup_float128_info (struct printf_info *info, int is_long_double,
>> +		     unsigned int mode_flags)
>> +{
>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>> +  if ((mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
>> +    info->is_binary128 = is_long_double;
>> +  else
>> +    info->is_binary128 = 0;
>> +#else
>> +  info->is_binary128 = 0;
>> +#endif
>> +}
>> +
>>  
>>  #define	outchar(Ch)							      \
>>    do									      \
>> @@ -771,10 +807,8 @@ static const uint8_t jump_table[] =
>>  					.wide = sizeof (CHAR_T) != 1,	      \
>>  					.is_binary128 = 0};		      \
>>  									      \
>> -	    if (is_long_double)						      \
>> -	      the_arg.pa_long_double = va_arg (ap, long double);	      \
>> -	    else							      \
>> -	      the_arg.pa_double = va_arg (ap, double);			      \
>> +	    parse_float_va_arg (&info, &the_arg, is_long_double,	      \
>> +				mode_flags, &ap);			      \
>>  	    ptr = (const void *) &the_arg;				      \
>>  									      \
>>  	    function_done = __printf_fp (s, &info, &ptr);		      \
>> @@ -787,8 +821,7 @@ static const uint8_t jump_table[] =
>>  		fspec->data_arg_type = PA_DOUBLE;			      \
>>  		fspec->info.is_long_double = 0;				      \
>>  	      }								      \
>> -	    /* Not supported by *printf functions.  */			      \
>> -	    fspec->info.is_binary128 = 0;				      \
>> +	    setup_float128_info (&fspec->info, is_long_double, mode_flags);   \
>>  									      \
>>  	    function_done = __printf_fp (s, &fspec->info, &ptr);	      \
>>  	  }								      \
>> @@ -831,10 +864,8 @@ static const uint8_t jump_table[] =
>>  					.wide = sizeof (CHAR_T) != 1,	      \
>>  					.is_binary128 = 0};		      \
>>  									      \
>> -	    if (is_long_double)						      \
>> -	      the_arg.pa_long_double = va_arg (ap, long double);	      \
>> -	    else							      \
>> -	      the_arg.pa_double = va_arg (ap, double);			      \
>> +	    parse_float_va_arg (&info, &the_arg, is_long_double,	      \
>> +				mode_flags, &ap);			      \
>>  	    ptr = (const void *) &the_arg;				      \
>>  									      \
>>  	    function_done = __printf_fphex (s, &info, &ptr);		      \
>> @@ -844,8 +875,7 @@ static const uint8_t jump_table[] =
>>  	    ptr = (const void *) &args_value[fspec->data_arg];		      \
>>  	    if (__glibc_unlikely ((mode_flags & PRINTF_LDBL_IS_DBL) != 0))    \
>>  	      fspec->info.is_long_double = 0;				      \
>> -	    /* Not supported by *printf functions.  */			      \
>> -	    fspec->info.is_binary128 = 0;				      \
>> +	    setup_float128_info (&fspec->info, is_long_double, mode_flags);   \
>>  									      \
>>  	    function_done = __printf_fphex (s, &fspec->info, &ptr);	      \
>>  	  }								      \
>> @@ -1869,6 +1899,10 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
>>  	    args_value[cnt].pa_double = va_arg (*ap_savep, double);
>>  	    args_type[cnt] &= ~PA_FLAG_LONG_DOUBLE;
>>  	  }
>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>> +	else if ((mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
>> +	  args_value[cnt].pa_float128 = va_arg (*ap_savep, _Float128);
>> +#endif
>>  	else
>>  	  args_value[cnt].pa_long_double = va_arg (*ap_savep, long double);
>>  	break;
>> @@ -1887,7 +1921,7 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
>>  	      (args_value[cnt].pa_user, ap_savep);
>>  	  }
>>  	else
>> -	  args_value[cnt].pa_long_double = 0.0;
>> +	  memset (&args_value[cnt], 0, sizeof (args_value[cnt]));
>>  	break;
>>        case -1:
>>  	/* Error case.  Not all parameters appear in N$ format
>> -- 
>> 2.14.5
>>
  

Hi, Adhemerval,

> > On Tue, May 21 2019, Gabriel F. T. Gomes wrote:
> >>
> >> Changes since v4:
> >>
> >>   - Replaced macros with inline functions.

While running some final tests, I noticed that this last change is
incorrect.  On x86_64, powerpc32, and maybe others, it will fail to
build (details below).

> >> +static void
> >> +parse_float_va_arg (struct printf_info *info, union printf_arg *the_arg,
> >> +		    int is_long_double, unsigned int mode_flags,
> >> +		    va_list *ap)
> >> +{
> >> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> >> +  if (is_long_double && (mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
> >> +    {
> >> +      info->is_binary128 = 1;
> >> +      the_arg->pa_float128 = va_arg (*ap, _Float128);
> >> +    }
> >> +  else
> >> +#endif
> >> +    {
> >> +      info->is_binary128 = 0;
> >> +      if (is_long_double)
> >> +	the_arg->pa_long_double = va_arg (*ap, long double);
> >> +      else
> >> +	the_arg->pa_double = va_arg (*ap, double);
> >> +    }
> >> +}
> >> +
> >>
> >> [...]
> >>
> >> -	    if (is_long_double)						      \
> >> -	      the_arg.pa_long_double = va_arg (ap, long double);	      \
> >> -	    else							      \
> >> -	      the_arg.pa_double = va_arg (ap, double);			      \
> >> +	    parse_float_va_arg (&info, &the_arg, is_long_double,	      \
> >> +				mode_flags, &ap);			      \

On x86_64 and powerpc32, I get the following errors with this version of
the patch:

  vfprintf-internal.c: In function ‘__vfprintf_internal’:
  vfprintf-internal.c:811:17: error: passing argument 5 of ‘parse_float_va_arg’ from incompatible pointer type [-Werror=incompatible-pointer-types]
       mode_flags, &ap);         \
                   ^
  vfprintf-internal.c:1674:4: note: in expansion of macro ‘process_arg’
      process_arg (((struct printf_spec *) NULL));
      ^~~~~~~~~~~
  vfprintf-internal.c:154:1: note: expected ‘__va_list_tag (*)[1]’ but argument is of type ‘__va_list_tag **’
   parse_float_va_arg (struct printf_info *info, union printf_arg *the_arg,
   ^~~~~~~~~~~~~~~~~~

After I got this, I tried to use 'va_copy' to correctly pass the va_list
(and its state) to 'parse_float_va_arg', but that doesn't work, because
'va_arg' might have been called an unknown number of times with 'ap'.

I'm sorry I didn't notice this before (my tests with v5 were not good
enough), but I think we should go back to v4 (except for the typo fix)
and use the macros that correctly call 'va_arg' on the already
initialized 'va_list'.

Does that sound reasonable to you?
  

On 11/06/2019 09:03, Gabriel F. T. Gomes wrote:
> Hi, Adhemerval,
> 
>>> On Tue, May 21 2019, Gabriel F. T. Gomes wrote:
>>>>
>>>> Changes since v4:
>>>>
>>>>   - Replaced macros with inline functions.
> 
> While running some final tests, I noticed that this last change is
> incorrect.  On x86_64, powerpc32, and maybe others, it will fail to
> build (details below).
> 
>>>> +static void
>>>> +parse_float_va_arg (struct printf_info *info, union printf_arg *the_arg,
>>>> +		    int is_long_double, unsigned int mode_flags,
>>>> +		    va_list *ap)
>>>> +{
>>>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>>>> +  if (is_long_double && (mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
>>>> +    {
>>>> +      info->is_binary128 = 1;
>>>> +      the_arg->pa_float128 = va_arg (*ap, _Float128);
>>>> +    }
>>>> +  else
>>>> +#endif
>>>> +    {
>>>> +      info->is_binary128 = 0;
>>>> +      if (is_long_double)
>>>> +	the_arg->pa_long_double = va_arg (*ap, long double);
>>>> +      else
>>>> +	the_arg->pa_double = va_arg (*ap, double);
>>>> +    }
>>>> +}
>>>> +
>>>>
>>>> [...]
>>>>
>>>> -	    if (is_long_double)						      \
>>>> -	      the_arg.pa_long_double = va_arg (ap, long double);	      \
>>>> -	    else							      \
>>>> -	      the_arg.pa_double = va_arg (ap, double);			      \
>>>> +	    parse_float_va_arg (&info, &the_arg, is_long_double,	      \
>>>> +				mode_flags, &ap);			      \
> 
> On x86_64 and powerpc32, I get the following errors with this version of
> the patch:
> 
>   vfprintf-internal.c: In function ‘__vfprintf_internal’:
>   vfprintf-internal.c:811:17: error: passing argument 5 of ‘parse_float_va_arg’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>        mode_flags, &ap);         \
>                    ^
>   vfprintf-internal.c:1674:4: note: in expansion of macro ‘process_arg’
>       process_arg (((struct printf_spec *) NULL));
>       ^~~~~~~~~~~
>   vfprintf-internal.c:154:1: note: expected ‘__va_list_tag (*)[1]’ but argument is of type ‘__va_list_tag **’
>    parse_float_va_arg (struct printf_info *info, union printf_arg *the_arg,
>    ^~~~~~~~~~~~~~~~~~
> 
> After I got this, I tried to use 'va_copy' to correctly pass the va_list
> (and its state) to 'parse_float_va_arg', but that doesn't work, because
> 'va_arg' might have been called an unknown number of times with 'ap'.
> 
> I'm sorry I didn't notice this before (my tests with v5 were not good
> enough), but I think we should go back to v4 (except for the typo fix)
> and use the macros that correctly call 'va_arg' on the already
> initialized 'va_list'.
> 
> Does that sound reasonable to you?

Right, I forgot this regarding va_list handling. Indeed the last version lgtm, thanks.