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

Message ID 20190521170829.20796-1-gabriel@inconstante.eti.br
State New
Headers show
Series
  • [v5] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg
Related show

Commit Message

Gabriel F. T. Gomes May 21, 2019, 5:08 p.m.
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(-)

-- 
2.14.5

Comments

Gabriel F. T. Gomes June 5, 2019, 7:08 p.m. | #1
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

>
Adhemerval Zanella June 6, 2019, 5:44 p.m. | #2
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

>>
Gabriel F. T. Gomes June 11, 2019, 12:03 p.m. | #3
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?
Adhemerval Zanella June 11, 2019, 5:44 p.m. | #4
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.

Patch

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