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 | expand |
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.
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