| Message ID | 001301d09d57$c0bfdf60$423f9e20$@com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2 Jun 2015, Wilco Dijkstra wrote: > The printf code contains a few uses of __isinf where the sign is used - > replace these with separate isinf and signbit. Why? Isn't the point of glibc's isinf returning a signed value that you can do such combined operations? I'd also really recommend separating safe cleanups that don't change the stripped installed shared libraries at all (other than assertion line numbers), such as moving to use the type-generic macros internally, from any other changes that might affect the installed binaries. And when something shouldn't change the (stripped) installed binaries, verify that it doesn't, and state the platform on which you did such verification.
On 02/06/15 18:15, Wilco Dijkstra wrote: > The printf code contains a few uses of __isinf where the sign is used - replace these with separate > isinf and signbit. Also change __isnan into isnan. > i suspect it slightly changes semantics on x86 (for the better). i386 __isnanl does not seem to handle invalid ld80 representations. if exponent != 0 and significand>>63 == 0 then it's a "pseudo normal" or "pseudo infinite" value that the fpu would categorize as nan, but not __isnanl. (passing around long doubles with invalid representation is ub, but probably __isnanl should be consistent with fcom instruction to be safe when it is used on random bytes). > OK for commit? > > Wilco > > 2015-06-02 Wilco Dijkstra <wdijkstr@arm.com> > > * stdio-common/printf_fp.c (___printf_fp): > Use signbit to get the sign. Use isinf/isnan macros. > * stdio-common/printf_fphex.c (__printf_fphex): Likewise. > * stdio-common/printf_size.c (__printf_size): Likewise. > > --- > stdio-common/printf_fp.c | 17 +++++++---------- > stdio-common/printf_fphex.c | 14 +++++--------- > stdio-common/printf_size.c | 23 +++++++++++------------ > 3 files changed, 23 insertions(+), 31 deletions(-) > > diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c > index 575842b..11f3145 100644 > --- a/stdio-common/printf_fp.c > +++ b/stdio-common/printf_fp.c > @@ -332,8 +332,7 @@ ___printf_fp (FILE *fp, > fpnum.ldbl = *(const long double *) args[0]; > > /* Check for special values: not a number or infinity. */ > - int res; > - if (__isnanl (fpnum.ldbl)) > + if (isnan (fpnum.ldbl)) > { > is_neg = signbit (fpnum.ldbl); > if (isupper (info->spec)) > @@ -347,9 +346,9 @@ ___printf_fp (FILE *fp, > wspecial = L"nan"; > } > } > - else if ((res = __isinfl (fpnum.ldbl))) > + else if (isinf (fpnum.ldbl)) > { > - is_neg = res < 0; > + is_neg = signbit (fpnum.ldbl); > if (isupper (info->spec)) > { > special = "INF"; > @@ -377,11 +376,9 @@ ___printf_fp (FILE *fp, > fpnum.dbl = *(const double *) args[0]; > > /* Check for special values: not a number or infinity. */ > - int res; > - if (__isnan (fpnum.dbl)) > + if (isnan (fpnum.dbl)) > { > - union ieee754_double u = { .d = fpnum.dbl }; > - is_neg = u.ieee.negative != 0; > + is_neg = signbit (fpnum.dbl); > if (isupper (info->spec)) > { > special = "NAN"; > @@ -393,9 +390,9 @@ ___printf_fp (FILE *fp, > wspecial = L"nan"; > } > } > - else if ((res = __isinf (fpnum.dbl))) > + else if (isinf (fpnum.dbl)) > { > - is_neg = res < 0; > + is_neg = signbit (fpnum.dbl); > if (isupper (info->spec)) > { > special = "INF"; > diff --git a/stdio-common/printf_fphex.c b/stdio-common/printf_fphex.c > index ba0639f..0627bea 100644 > --- a/stdio-common/printf_fphex.c > +++ b/stdio-common/printf_fphex.c > @@ -165,7 +165,7 @@ __printf_fphex (FILE *fp, > fpnum.ldbl = *(const long double *) args[0]; > > /* Check for special values: not a number or infinity. */ > - if (__isnanl (fpnum.ldbl)) > + if (isnan (fpnum.ldbl)) > { > if (isupper (info->spec)) > { > @@ -180,7 +180,7 @@ __printf_fphex (FILE *fp, > } > else > { > - if (__isinfl (fpnum.ldbl)) > + if (isinf (fpnum.ldbl)) > { > if (isupper (info->spec)) > { > @@ -202,9 +202,8 @@ __printf_fphex (FILE *fp, > fpnum.dbl.d = *(const double *) args[0]; > > /* Check for special values: not a number or infinity. */ > - if (__isnan (fpnum.dbl.d)) > + if (isnan (fpnum.dbl.d)) > { > - negative = fpnum.dbl.ieee.negative != 0; > if (isupper (info->spec)) > { > special = "NAN"; > @@ -218,8 +217,7 @@ __printf_fphex (FILE *fp, > } > else > { > - int res = __isinf (fpnum.dbl.d); > - if (res) > + if (isinf (fpnum.dbl.d)) > { > if (isupper (info->spec)) > { > @@ -231,11 +229,9 @@ __printf_fphex (FILE *fp, > special = "inf"; > wspecial = L"inf"; > } > - negative = res < 0; > } > - else > - negative = signbit (fpnum.dbl.d); > } > + negative = signbit (fpnum.dbl.d); > } > > if (special) > diff --git a/stdio-common/printf_size.c b/stdio-common/printf_size.c > index 6ee753f..216f170 100644 > --- a/stdio-common/printf_size.c > +++ b/stdio-common/printf_size.c > @@ -108,7 +108,7 @@ __printf_size (FILE *fp, const struct printf_info *info, > fpnum; > const void *ptr = &fpnum; > > - int fpnum_sign = 0; > + int is_neg = 0; > > /* "NaN" or "Inf" for the special cases. */ > const char *special = NULL; > @@ -117,7 +117,6 @@ __printf_size (FILE *fp, const struct printf_info *info, > struct printf_info fp_info; > int done = 0; > int wide = info->wide; > - int res; > > /* Fetch the argument value. */ > #ifndef __NO_LONG_DOUBLE_MATH > @@ -126,15 +125,15 @@ __printf_size (FILE *fp, const struct printf_info *info, > fpnum.ldbl = *(const long double *) args[0]; > > /* Check for special values: not a number or infinity. */ > - if (__isnanl (fpnum.ldbl)) > + if (isnan (fpnum.ldbl)) > { > special = "nan"; > wspecial = L"nan"; > - // fpnum_sign = 0; Already zero > + // is_neg = 0; Already zero > } > - else if ((res = __isinfl (fpnum.ldbl))) > + else if (isinf (fpnum.ldbl)) > { > - fpnum_sign = res; > + is_neg = signbit (fpnum.ldbl); > special = "inf"; > wspecial = L"inf"; > } > @@ -151,15 +150,15 @@ __printf_size (FILE *fp, const struct printf_info *info, > fpnum.dbl.d = *(const double *) args[0]; > > /* Check for special values: not a number or infinity. */ > - if (__isnan (fpnum.dbl.d)) > + if (isnan (fpnum.dbl.d)) > { > special = "nan"; > wspecial = L"nan"; > - // fpnum_sign = 0; Already zero > + // is_neg = 0; Already zero > } > - else if ((res = __isinf (fpnum.dbl.d))) > + else if (isinf (fpnum.dbl.d)) > { > - fpnum_sign = res; > + is_neg = signbit (fpnum.dbl.d); > special = "inf"; > wspecial = L"inf"; > } > @@ -175,14 +174,14 @@ __printf_size (FILE *fp, const struct printf_info *info, > { > int width = info->prec > info->width ? info->prec : info->width; > > - if (fpnum_sign < 0 || info->showsign || info->space) > + if (is_neg || info->showsign || info->space) > --width; > width -= 3; > > if (!info->left && width > 0) > PADN (' ', width); > > - if (fpnum_sign < 0) > + if (is_neg) > outchar ('-'); > else if (info->showsign) > outchar ('+'); >
On Tue, 2 Jun 2015, Szabolcs Nagy wrote: > On 02/06/15 18:15, Wilco Dijkstra wrote: > > The printf code contains a few uses of __isinf where the sign is used - replace these with separate > > isinf and signbit. Also change __isnan into isnan. > > i suspect it slightly changes semantics on x86 (for the better). It shouldn't. isnan, the type-generic macro, just calls __isnanf / __isnan / __isnanl depending on the type of the argument. > i386 __isnanl does not seem to handle invalid ld80 representations. > > if exponent != 0 and significand>>63 == 0 then it's a "pseudo normal" > or "pseudo infinite" value that the fpu would categorize as nan, but > not __isnanl. > > (passing around long doubles with invalid representation is ub, > but probably __isnanl should be consistent with fcom instruction > to be safe when it is used on random bytes). glibc policy on invalid long double values (both ldbl-96 and I think ldbl-128ibm) is bounded undefined behavior (unspecified result, unspecified exceptions, no expectation of any consistency, but should avoid crashing the program / out-of-bounds stores).
> Joseph Myers wrote: > On Tue, 2 Jun 2015, Wilco Dijkstra wrote: > > > The printf code contains a few uses of __isinf where the sign is used - > > replace these with separate isinf and signbit. > > Why? Isn't the point of glibc's isinf returning a signed value that you > can do such combined operations? We discussed this a while back (and I thought it was agreed on going forward like this) - to recap, it is not required by any standard (so no standard compliant software may rely on this GLIBC specific behaviour), and is actually incompatible with C++ which requires a boolean to be returned. So the idea is to only support the old behaviour in GNUC mode for backwards compatibility, but not in C99/C++. It would be possible to introduce a new macro isinf_sign if someone can make the argument it is a good interface. I don't see how it could be as getting the sign is just a single compare or shift once you've transferred the FP value to the integer side - in other words isinf_sign (x) will be slower than using isinf (x) plus signbit (x)... > I'd also really recommend separating safe cleanups that don't change the > stripped installed shared libraries at all (other than assertion line > numbers), such as moving to use the type-generic macros internally, from > any other changes that might affect the installed binaries. And when > something shouldn't change the (stripped) installed binaries, verify that > it doesn't, and state the platform on which you did such verification. I have a separate patch with all the __isxxx macros renamed across GLIBC, so I'll add the __isnan cases in printf too. I'll check in when I've confirmed there are no diffs. Wilco
On Wed, 3 Jun 2015, Wilco Dijkstra wrote: > > Joseph Myers wrote: > > On Tue, 2 Jun 2015, Wilco Dijkstra wrote: > > > > > The printf code contains a few uses of __isinf where the sign is used - > > > replace these with separate isinf and signbit. > > > > Why? Isn't the point of glibc's isinf returning a signed value that you > > can do such combined operations? > > We discussed this a while back (and I thought it was agreed on going forward > like this) - to recap, it is not required by any standard (so no standard I don't recall any such discussion or agreement. The discussion in bug 15367 is on the basis of using __builtin_isinf_sign to implement the header macro, not of changing the requirements and sometimes using __builtin_isinf. > compliant software may rely on this GLIBC specific behaviour), and is > actually incompatible with C++ which requires a boolean to be returned. C++ is the responsibility of libstdc++ (and defines overloaded functions, not type-generic macros; libstdc++ needs to override things done in the C header anyway). > So the idea is to only support the old behaviour in GNUC mode for backwards > compatibility, but not in C99/C++. glibc is built with _GNU_SOURCE and can freely use all GNU features (subject to link-time namespace issues). I see no need to change the interface to this macro in glibc. Presumably the aim should be to make the GCC __builtin_isinf / __builtin_isinf_sign at least as efficient as the out-of-line functions, and safe for all inputs, make the isinf macro use __builtin_isinf_sign, and make GCC optimize __builtin_isinf_sign calls so it's as efficient as __builtin_isinf if the sign isn't used, and as efficient as __builtin_isinf + __builtin_signbit if the sign is used.
> Joseph Myers wrote: > On Wed, 3 Jun 2015, Wilco Dijkstra wrote: > > > > Joseph Myers wrote: > > > On Tue, 2 Jun 2015, Wilco Dijkstra wrote: > > > > > > > The printf code contains a few uses of __isinf where the sign is used - > > > > replace these with separate isinf and signbit. > > > > > > Why? Isn't the point of glibc's isinf returning a signed value that you > > > can do such combined operations? > > > > We discussed this a while back (and I thought it was agreed on going forward > > like this) - to recap, it is not required by any standard (so no standard > > I don't recall any such discussion or agreement. The discussion in bug > 15367 is on the basis of using __builtin_isinf_sign to implement the > header macro, not of changing the requirements and sometimes using > __builtin_isinf. Well it was a while back. However the GCC built-ins need further optimization and fixes so just using the builtins is not a good fix either... > > compliant software may rely on this GLIBC specific behaviour), and is > > actually incompatible with C++ which requires a boolean to be returned. > > C++ is the responsibility of libstdc++ (and defines overloaded functions, > not type-generic macros; libstdc++ needs to override things done in the C > header anyway). > > > So the idea is to only support the old behaviour in GNUC mode for backwards > > compatibility, but not in C99/C++. > > glibc is built with _GNU_SOURCE and can freely use all GNU features > (subject to link-time namespace issues). I see no need to change the > interface to this macro in glibc. Presumably the aim should be to make > the GCC __builtin_isinf / __builtin_isinf_sign at least as efficient as > the out-of-line functions, and safe for all inputs, make the isinf macro > use __builtin_isinf_sign, and make GCC optimize __builtin_isinf_sign calls > so it's as efficient as __builtin_isinf if the sign isn't used, and as > efficient as __builtin_isinf + __builtin_signbit if the sign is used. In principle GLIBC could continue to use this feature internally, but do you have any objections to removing all internal uses like my patch does? In terms of interface, why do you think it is useful to continue a non-standard interface indefinitely rather than making it GNUC specific? Other than backwards compatibility there seems to be no compelling reason to keep it. Note I am also planning to remove all internal uses of __isinf_ns and replace them with isinf - no reason to keep using this non-standard interface either! Yes it would be possible to expand isinf as: (__isinf_ns (x) ? (signbit (x) ? -1 : 1) : 0) This should make if (isinf (x)) as efficient as if (__isinf_ns (x)), however if the result is assigned to a variable then it would always have additional overhead. This is obvious if the sign is not used, but if the sign is used it would still not be as efficient as separate isinf and signbit calls. Wilco
On Wed, 3 Jun 2015, Wilco Dijkstra wrote: > > > We discussed this a while back (and I thought it was agreed on going forward > > > like this) - to recap, it is not required by any standard (so no standard > > > > I don't recall any such discussion or agreement. The discussion in bug > > 15367 is on the basis of using __builtin_isinf_sign to implement the > > header macro, not of changing the requirements and sometimes using > > __builtin_isinf. > > Well it was a while back. However the GCC built-ins need further optimization > and fixes so just using the builtins is not a good fix either... I've grepped my libc-alpha mailboxes for the past 15 years without finding anything obvious in this area. > > glibc is built with _GNU_SOURCE and can freely use all GNU features > > (subject to link-time namespace issues). I see no need to change the > > interface to this macro in glibc. Presumably the aim should be to make > > the GCC __builtin_isinf / __builtin_isinf_sign at least as efficient as > > the out-of-line functions, and safe for all inputs, make the isinf macro > > use __builtin_isinf_sign, and make GCC optimize __builtin_isinf_sign calls > > so it's as efficient as __builtin_isinf if the sign isn't used, and as > > efficient as __builtin_isinf + __builtin_signbit if the sign is used. > > In principle GLIBC could continue to use this feature internally, but do > you have any objections to removing all internal uses like my patch does? Yes, in the absence of evidence that the two produce equivalent code or the new one produces better code than the old one (at least for current GCC). I'd expect that to most likely be the case once the GCC built-in functions have been appropriately optimized, made to use integer arithmetic and are in use when building with current GCC. It might well be the case with appropriate inline integer expansions in glibc headers, but I think the GCC inline improvements should take priority with any glibc inline expansions being added afterwards as fallbacks for older GCC versions. > In terms of interface, why do you think it is useful to continue a > non-standard interface indefinitely rather than making it GNUC specific? > Other than backwards compatibility there seems to be no compelling > reason to keep it. It's pretty much universally the case in glibc that feature test macros only protect the definitions of identifiers, rather than affecting the semantics of those identifiers. The exceptions are cases where extensions actually conflict with supported standards (e.g. basename, or scanf %a). The glibc extension does not conflict with the supported standards and doesn't seem otherwise problematic - and there's a clear route through GCC work for making it not pessimize code for programs that only care about the boolean return value of isinf. > Yes it would be possible to expand isinf as: > > (__isinf_ns (x) ? (signbit (x) ? -1 : 1) : 0) > > This should make if (isinf (x)) as efficient as if (__isinf_ns (x)), > however if the result is assigned to a variable then it would always > have additional overhead. This is obvious if the sign is not used, but > if the sign is used it would still not be as efficient as separate isinf > and signbit calls. I'd expect it to be just as efficient, given that the relevant functions have the const attribute so calls to them can be optimized out if the results are not used. It's rather less obvious how the efficiency would compare with a direct call to __isinf producing both bits of information, until everything is expanded inline. However, I don't think we really want to add __isinf_ns (an internal function at present, not exported from any shared library) to the public ABI, given the preferred direction involves compiler support for expanding all these calls efficiently inline.
diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c index 575842b..11f3145 100644 --- a/stdio-common/printf_fp.c +++ b/stdio-common/printf_fp.c @@ -332,8 +332,7 @@ ___printf_fp (FILE *fp, fpnum.ldbl = *(const long double *) args[0]; /* Check for special values: not a number or infinity. */ - int res; - if (__isnanl (fpnum.ldbl)) + if (isnan (fpnum.ldbl)) { is_neg = signbit (fpnum.ldbl); if (isupper (info->spec)) @@ -347,9 +346,9 @@ ___printf_fp (FILE *fp, wspecial = L"nan"; } } - else if ((res = __isinfl (fpnum.ldbl))) + else if (isinf (fpnum.ldbl)) { - is_neg = res < 0; + is_neg = signbit (fpnum.ldbl); if (isupper (info->spec)) { special = "INF"; @@ -377,11 +376,9 @@ ___printf_fp (FILE *fp, fpnum.dbl = *(const double *) args[0]; /* Check for special values: not a number or infinity. */ - int res; - if (__isnan (fpnum.dbl)) + if (isnan (fpnum.dbl)) { - union ieee754_double u = { .d = fpnum.dbl }; - is_neg = u.ieee.negative != 0; + is_neg = signbit (fpnum.dbl); if (isupper (info->spec)) { special = "NAN"; @@ -393,9 +390,9 @@ ___printf_fp (FILE *fp, wspecial = L"nan"; } } - else if ((res = __isinf (fpnum.dbl))) + else if (isinf (fpnum.dbl)) { - is_neg = res < 0; + is_neg = signbit (fpnum.dbl); if (isupper (info->spec)) { special = "INF"; diff --git a/stdio-common/printf_fphex.c b/stdio-common/printf_fphex.c index ba0639f..0627bea 100644 --- a/stdio-common/printf_fphex.c +++ b/stdio-common/printf_fphex.c @@ -165,7 +165,7 @@ __printf_fphex (FILE *fp, fpnum.ldbl = *(const long double *) args[0]; /* Check for special values: not a number or infinity. */ - if (__isnanl (fpnum.ldbl)) + if (isnan (fpnum.ldbl)) { if (isupper (info->spec)) { @@ -180,7 +180,7 @@ __printf_fphex (FILE *fp, } else { - if (__isinfl (fpnum.ldbl)) + if (isinf (fpnum.ldbl)) { if (isupper (info->spec)) { @@ -202,9 +202,8 @@ __printf_fphex (FILE *fp, fpnum.dbl.d = *(const double *) args[0]; /* Check for special values: not a number or infinity. */ - if (__isnan (fpnum.dbl.d)) + if (isnan (fpnum.dbl.d)) { - negative = fpnum.dbl.ieee.negative != 0; if (isupper (info->spec)) { special = "NAN"; @@ -218,8 +217,7 @@ __printf_fphex (FILE *fp, } else { - int res = __isinf (fpnum.dbl.d); - if (res) + if (isinf (fpnum.dbl.d)) { if (isupper (info->spec)) { @@ -231,11 +229,9 @@ __printf_fphex (FILE *fp, special = "inf"; wspecial = L"inf"; } - negative = res < 0; } - else - negative = signbit (fpnum.dbl.d); } + negative = signbit (fpnum.dbl.d); } if (special) diff --git a/stdio-common/printf_size.c b/stdio-common/printf_size.c index 6ee753f..216f170 100644 --- a/stdio-common/printf_size.c +++ b/stdio-common/printf_size.c @@ -108,7 +108,7 @@ __printf_size (FILE *fp, const struct printf_info *info, fpnum; const void *ptr = &fpnum; - int fpnum_sign = 0; + int is_neg = 0; /* "NaN" or "Inf" for the special cases. */ const char *special = NULL; @@ -117,7 +117,6 @@ __printf_size (FILE *fp, const struct printf_info *info, struct printf_info fp_info; int done = 0; int wide = info->wide; - int res; /* Fetch the argument value. */ #ifndef __NO_LONG_DOUBLE_MATH @@ -126,15 +125,15 @@ __printf_size (FILE *fp, const struct printf_info *info, fpnum.ldbl = *(const long double *) args[0]; /* Check for special values: not a number or infinity. */ - if (__isnanl (fpnum.ldbl)) + if (isnan (fpnum.ldbl)) { special = "nan"; wspecial = L"nan"; - // fpnum_sign = 0; Already zero + // is_neg = 0; Already zero } - else if ((res = __isinfl (fpnum.ldbl))) + else if (isinf (fpnum.ldbl)) { - fpnum_sign = res; + is_neg = signbit (fpnum.ldbl); special = "inf"; wspecial = L"inf"; } @@ -151,15 +150,15 @@ __printf_size (FILE *fp, const struct printf_info *info, fpnum.dbl.d = *(const double *) args[0]; /* Check for special values: not a number or infinity. */ - if (__isnan (fpnum.dbl.d)) + if (isnan (fpnum.dbl.d)) { special = "nan"; wspecial = L"nan"; - // fpnum_sign = 0; Already zero + // is_neg = 0; Already zero } - else if ((res = __isinf (fpnum.dbl.d))) + else if (isinf (fpnum.dbl.d)) { - fpnum_sign = res; + is_neg = signbit (fpnum.dbl.d); special = "inf"; wspecial = L"inf"; } @@ -175,14 +174,14 @@ __printf_size (FILE *fp, const struct printf_info *info, { int width = info->prec > info->width ? info->prec : info->width; - if (fpnum_sign < 0 || info->showsign || info->space) + if (is_neg || info->showsign || info->space) --width; width -= 3; if (!info->left && width > 0) PADN (' ', width); - if (fpnum_sign < 0) + if (is_neg) outchar ('-'); else if (info->showsign) outchar ('+');