[v4] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg
Commit Message
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_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(-)
Comments
Ping. This patch [1] is pending review.
Testing this patch depends on the subsequent patches that create new
functions, e.g. __printfieee128, that actually use these changes, as
well as it depends on the subsequent patch that enables the ldbl
redirections (e.g. from printf to __printfieee128 in bits/stdio-ldbl.h).
However, such redirections can only be turned on when the functions for
all other redirections (i.e. those in *-ldbl.h headers) are ready.
Thus, I uploaded all of them to my personal branch [2], in the hopes
that someone willing to understand how this patch is used, *and tested*,
can do it easier (I haven't simply submitted all the patches from my
branch to the mailing list, because I think some of them still need a
little work from me before review).
[1] https://sourceware.org/ml/libc-alpha/2019-02/msg00582.html
[2] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/gabriel/powerpc-ieee128-printscan
On Mon, Feb 25 2019, 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.
Ping. This patch is pending review.
On Wed, Apr 10 2019, Gabriel F. T. Gomes wrote:
> Ping. This patch [1] is pending review.
>
> Testing this patch depends on the subsequent patches that create new
> functions, e.g. __printfieee128, that actually use these changes, as
> well as it depends on the subsequent patch that enables the ldbl
> redirections (e.g. from printf to __printfieee128 in bits/stdio-ldbl.h).
>
> However, such redirections can only be turned on when the functions for
> all other redirections (i.e. those in *-ldbl.h headers) are ready.
> Thus, I uploaded all of them to my personal branch [2], in the hopes
> that someone willing to understand how this patch is used, *and tested*,
> can do it easier (I haven't simply submitted all the patches from my
> branch to the mailing list, because I think some of them still need a
> little work from me before review).
>
> [1] https://sourceware.org/ml/libc-alpha/2019-02/msg00582.html
> [2] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/gabriel/powerpc-ieee128-printscan
>
> On Mon, Feb 25 2019, 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.
Ping. This patch is pending review.
https://sourceware.org/ml/libc-alpha/2019-02/msg00582.html
On Wed, Apr 24 2019, Gabriel F. T. Gomes wrote:
> Ping. This patch is pending review.
>
> On Wed, Apr 10 2019, Gabriel F. T. Gomes wrote:
> > Ping. This patch [1] is pending review.
> >
> > Testing this patch depends on the subsequent patches that create new
> > functions, e.g. __printfieee128, that actually use these changes, as
> > well as it depends on the subsequent patch that enables the ldbl
> > redirections (e.g. from printf to __printfieee128 in bits/stdio-ldbl.h).
> >
> > However, such redirections can only be turned on when the functions for
> > all other redirections (i.e. those in *-ldbl.h headers) are ready.
> > Thus, I uploaded all of them to my personal branch [2], in the hopes
> > that someone willing to understand how this patch is used, *and tested*,
> > can do it easier (I haven't simply submitted all the patches from my
> > branch to the mailing list, because I think some of them still need a
> > little work from me before review).
> >
> > [1] https://sourceware.org/ml/libc-alpha/2019-02/msg00582.html
> > [2] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/gabriel/powerpc-ieee128-printscan
> >
> > On Mon, Feb 25 2019, 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.
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
>
On Thu, May 09 2019, Adhemerval Zanella wrote:
>
> On 25/02/2019 10:39, Gabriel F. T. Gomes wrote:
> >
> > 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.
Well, I intentionally write small patches that are valid for master, so
they can be applied sooner than later. I have written many patches this
way to avoid the burden of carrying patches on personal branches.
I also provided a link to such branch where the changes in this patch
get tested for many of the functions added by subsequent patches.
I was hoping that these things combined would provide a good
justification for acceptance. With that in mind, do you have an
objection to this patch?
> > + 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
Thanks. Fixed locally.
> > +#if __HAVE_FLOAT128_UNLIKE_LDBL
> > +# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO) \
> > [...]
>
> 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:
Sounds good to me. I'll send a new version.
> 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)
Should I also add an __always_inline attribute?
> > +#define PARSE_FLOAT_VA_ARG(INFO) \
> > [...]
>
> Same as before.
OK.
On 16/05/2019 18:36, Gabriel F. T. Gomes wrote:
> On Thu, May 09 2019, Adhemerval Zanella wrote:
>>
>> On 25/02/2019 10:39, Gabriel F. T. Gomes wrote:
>>>
>>> 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.
>
> Well, I intentionally write small patches that are valid for master, so
> they can be applied sooner than later. I have written many patches this
> way to avoid the burden of carrying patches on personal branches.
>
> I also provided a link to such branch where the changes in this patch
> get tested for many of the functions added by subsequent patches.
>
> I was hoping that these things combined would provide a good
> justification for acceptance. With that in mind, do you have an
> objection to this patch?
My main objection is currently glibc still does not support -mabi=ieeelongdouble
(correct me if I wrong), so it might take some time to actually test this
code path on powerpc.
>
>>> + 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
>
> Thanks. Fixed locally.
>
>>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>>> +# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO) \
>>> [...]
>>
>> 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:
>
> Sounds good to me. I'll send a new version.
>
>> 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)
>
> Should I also add an __always_inline attribute?
Do we need it for correctness or does it show in any performance evaluation?
Otherwise I just let compiler handle it.
>
>>> +#define PARSE_FLOAT_VA_ARG(INFO) \
>>> [...]
>>
>> Same as before.
>
> OK.
>
On Mon, 20 May 2019, Adhemerval Zanella wrote:
> > Well, I intentionally write small patches that are valid for master, so
> > they can be applied sooner than later. I have written many patches this
> > way to avoid the burden of carrying patches on personal branches.
> >
> > I also provided a link to such branch where the changes in this patch
> > get tested for many of the functions added by subsequent patches.
> >
> > I was hoping that these things combined would provide a good
> > justification for acceptance. With that in mind, do you have an
> > objection to this patch?
>
> My main objection is currently glibc still does not support -mabi=ieeelongdouble
> (correct me if I wrong), so it might take some time to actually test this
> code path on powerpc.
I think this is a case where the preparatory patch is sufficiently
self-contained that it is reasonable to have is on master separately from
others that depend on it. (This is not a review or approval of the
details of the specific patch. And for some patches, even when they are
reasonable to include on their own, the review of the design of the patch
may depend on looking at the later patches in the series to see how much
the design is forced by requirements that aren't obvious from the patch in
isolation.)
On 20/05/2019 16:16, Joseph Myers wrote:
> On Mon, 20 May 2019, Adhemerval Zanella wrote:
>
>>> Well, I intentionally write small patches that are valid for master, so
>>> they can be applied sooner than later. I have written many patches this
>>> way to avoid the burden of carrying patches on personal branches.
>>>
>>> I also provided a link to such branch where the changes in this patch
>>> get tested for many of the functions added by subsequent patches.
>>>
>>> I was hoping that these things combined would provide a good
>>> justification for acceptance. With that in mind, do you have an
>>> objection to this patch?
>>
>> My main objection is currently glibc still does not support -mabi=ieeelongdouble
>> (correct me if I wrong), so it might take some time to actually test this
>> code path on powerpc.
>
> I think this is a case where the preparatory patch is sufficiently
> self-contained that it is reasonable to have is on master separately from
> others that depend on it. (This is not a review or approval of the
> details of the specific patch. And for some patches, even when they are
> reasonable to include on their own, the review of the design of the patch
> may depend on looking at the later patches in the series to see how much
> the design is forced by requirements that aren't obvious from the patch in
> isolation.)
>
Alright then, I withdrew my objection.
@@ -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
+ 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;
@@ -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
+
+#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
+
#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