[v5] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg
Commit Message
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(-)
Comments
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.
@@ -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)
@@ -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;
@@ -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