This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v4] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Thu, 9 May 2019 17:29:37 -0300
- Subject: Re: [PATCH v4] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg
- References: <20190225133946.17585-1-gabriel@inconstante.eti.br>
On 25/02/2019 10:39, Gabriel F. T. Gomes wrote:
> 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.
It at least shows no issue when applied against master, although current
tests not stress the code itself. And it seems that the code would only be
enabled for powerpc with -mabi=ieeelongdouble, so I would if it would be
better to actually push this patch along with powerpc64 -mabi=ieeelongdouble
patchset.
>
> * 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_EXTENDED): New macro.
> (PARSE_FLOAT_VA_ARG): Likewise.
> (SETUP_FLOAT128_INFO): Likewise.
> (process_arg): Use PARSE_FLOAT_VA_ARG_EXTENDED 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 | 73 +++++++++++++++++++++++++++++++++-------
> 3 files changed, 79 insertions(+), 17 deletions(-)
>
> diff --git a/libio/libioP.h b/libio/libioP.h
> index 7bdec86a62..eb86e07ad6 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 macros indicates that long double values are to be
s/macros/macro
> + 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..d9dca78fc6 100644
> --- a/stdio-common/vfprintf-internal.c
> +++ b/stdio-common/vfprintf-internal.c
> @@ -68,6 +68,57 @@
> } while (0)
> #define UNBUFFERED_P(S) ((S)->_flags & _IO_UNBUFFERED)
>
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO) \
> + do \
> + { \
> + if (is_long_double \
> + && (mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0) \
> + { \
> + INFO.is_binary128 = 1; \
> + the_arg.pa_float128 = va_arg (ap, _Float128); \
> + } \
> + else \
> + { \
> + PARSE_FLOAT_VA_ARG (INFO); \
> + } \
> + } \
> + while (0)
> +#else
> +# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO) \
> + PARSE_FLOAT_VA_ARG (INFO);
> +#endif
Do we really to continue use the macro-style to extend this code? I usually
think is is more readable to just use static inline function instead,
something as:
---
static void
parse_float_va_arg_extended (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);
}
}
---
> +
> +#define PARSE_FLOAT_VA_ARG(INFO) \
> + do \
> + { \
> + 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); \
> + } \
> + while (0)
> +
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +# define SETUP_FLOAT128_INFO(INFO) \
> + do \
> + { \
> + if ((mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0) \
> + INFO.is_binary128 = is_long_double; \
> + else \
> + INFO.is_binary128 = 0; \
> + } \
> + while (0)
> +#else
> +# define SETUP_FLOAT128_INFO(INFO) \
> + do \
> + { \
> + INFO.is_binary128 = 0; \
> + } \
> + while (0)
> +#endif
> +
Same as before.
> #define done_add(val) \
> do { \
> unsigned int _val = val; \
> @@ -771,10 +822,7 @@ 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_EXTENDED (info); \
> ptr = (const void *) &the_arg; \
> \
> function_done = __printf_fp (s, &info, &ptr); \
> @@ -787,8 +835,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); \
> \
> function_done = __printf_fp (s, &fspec->info, &ptr); \
> } \
> @@ -831,10 +878,7 @@ 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_EXTENDED (info); \
> ptr = (const void *) &the_arg; \
> \
> function_done = __printf_fphex (s, &info, &ptr); \
> @@ -844,8 +888,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); \
> \
> function_done = __printf_fphex (s, &fspec->info, &ptr); \
> } \
> @@ -1869,6 +1912,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 +1934,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
>