[RFC,v2,1/3] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg
Commit Message
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 possibilites
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 possibilites 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 extented 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
(LDBL_USES_FLOAT128): New macro.
(PARSE_FLOAT_VA_ARG_EXTENDED): Likewise.
(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.
---
libio/libioP.h | 5 ++--
stdio-common/printf-parse.h | 3 ++
stdio-common/vfprintf-internal.c | 62 +++++++++++++++++++++++++++++++---------
3 files changed, 55 insertions(+), 15 deletions(-)
Comments
On Tue, 5 Jun 2018, Gabriel F. T. Gomes wrote:
> +#define LDBL_USES_FLOAT128 ((mode_flags && PRINTF_LDBL_USES_FLOAT128) == 1)
That && needs to be &, at which point you need to test != 0 rather than ==
1.
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +# define SETUP_FLOAT128_INFO(INFO) \
> + if (LDBL_USES_FLOAT128) \
> + INFO.is_binary128 = 1; \
> + else \
> + INFO.is_binary128 = 0;
> +#else
> +# define SETUP_FLOAT128_INFO(INFO) \
> + INFO.is_binary128 = 0;
> +#endif
I think the first case here is wrong. As I understand it, this macro is
used in the case of positional parameters or registered printf handlers,
where the string is first parsed (via printf-parsemb.c, which has no
special handling of different long double formats), and then the arguments
are extracted in order, and then the format specifiers are processed with
the arguments that were extracted. In particular, the code using it
> @@ -789,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) \
> \
> function_done = __printf_fp (s, &fspec->info, &ptr); \
> } \
(and likewise for hex floats) executes for a double argument, as well as
long double. So what I think you actually need to do is copy the
is_long_double setting to is_binary128 in the LDBL_USES_FLOAT128 case,
rather than setting it to 1 unconditionally.
This illustrates the need for more thorough tests of vfprintf, which
should test double arguments as well as long double, and test the use of
positional arguments. (Given such thorough tests for one printf-family
function, other printf-family functions could then have less detailed
tests that suffice to verify the correct long double format gets handled.)
> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> + else if (LDBL_USES_FLOAT128)
> + args_value[cnt].pa_float128 = va_arg (*ap_savep, __float128);
> +#endif
I think you should generally be using _Float128, not __float128, in such
code.
@@ -687,8 +687,9 @@ extern int __vswprintf_internal (wchar_t *string, size_t maxlen,
unsigned int mode_flags);
/* Flags for __v*printf_internal. */
-#define PRINTF_LDBL_IS_DBL 0x0001
-#define PRINTF_FORTIFY 0x0002
+#define PRINTF_LDBL_IS_DBL 0x0001
+#define PRINTF_FORTIFY 0x0002
+#define PRINTF_LDBL_USES_FLOAT128 0x0004
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;
@@ -68,8 +68,43 @@
} while (0)
#define UNBUFFERED_P(S) ((S)->_flags & _IO_UNBUFFERED)
#define LDBL_IS_DBL (__glibc_unlikely ((mode_flags & PRINTF_LDBL_IS_DBL) != 0))
+#define LDBL_USES_FLOAT128 ((mode_flags && PRINTF_LDBL_USES_FLOAT128) == 1)
#define DO_FORTIFY ((mode_flags & PRINTF_FORTIFY) != 0)
+#if __HAVE_FLOAT128_UNLIKE_LDBL
+# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO) \
+ if (LDBL_USES_FLOAT128) \
+ { \
+ INFO.is_binary128 = 1; \
+ the_arg.pa_float128 = va_arg (ap, __float128); \
+ } \
+ else \
+ { \
+ PARSE_FLOAT_VA_ARG (INFO) \
+ }
+#else
+# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO) \
+ PARSE_FLOAT_VA_ARG (INFO)
+#endif
+
+#define PARSE_FLOAT_VA_ARG(INFO) \
+ 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 __HAVE_FLOAT128_UNLIKE_LDBL
+# define SETUP_FLOAT128_INFO(INFO) \
+ if (LDBL_USES_FLOAT128) \
+ INFO.is_binary128 = 1; \
+ else \
+ INFO.is_binary128 = 0;
+#else
+# define SETUP_FLOAT128_INFO(INFO) \
+ INFO.is_binary128 = 0;
+#endif
+
#define done_add(val) \
do { \
unsigned int _val = val; \
@@ -773,10 +808,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); \
@@ -789,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) \
\
function_done = __printf_fp (s, &fspec->info, &ptr); \
} \
@@ -833,10 +864,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); \
@@ -846,8 +874,7 @@ static const uint8_t jump_table[] =
ptr = (const void *) &args_value[fspec->data_arg]; \
if (LDBL_IS_DBL) \
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); \
} \
@@ -1871,6 +1898,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 (LDBL_USES_FLOAT128)
+ 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;
@@ -1889,7 +1920,12 @@ 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;
+ {
+ args_value[cnt].pa_long_double = 0.0;
+#if __HAVE_FLOAT128_UNLIKE_LDBL
+ args_value[cnt].pa_float128 = 0;
+#endif
+ }
break;
case -1:
/* Error case. Not all parameters appear in N$ format