[-- Attachment #1.1: Type: text/plain, Size: 494 bytes --] This fixes a compiler warning with clang: floatscan.c:304:13: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value]. This does change the behavior of the expression because the value is no longer rounded to double, however from my reading of the code, the rounding doesn't seem intended. However, if it is, I suggest introducing an explicit cast, to document the intent. Dan [-- Attachment #1.2: Type: text/html, Size: 617 bytes --] [-- Attachment #2: 0001-Use-fabsl-instead-of-fabs-on-long-double-in-floatsca.patch --] [-- Type: text/x-patch, Size: 1059 bytes --] From 1fecc521dc43b25366cd4a3062964ff3abc7506e Mon Sep 17 00:00:00 2001 From: Dan Gohman <sunfish@mozilla.com> Date: Fri, 18 Oct 2019 06:22:49 -0700 Subject: [PATCH] Use `fabsl` instead of `fabs` on long double in floatscan.c This fixes a compiler warning: floatscan.c:304:13: warning: absolute value function 'fabs' given an argument of type 'long double' but has parameter of type 'double' which may cause truncation of value [-Wabsolute-value] --- src/internal/floatscan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/floatscan.c b/src/internal/floatscan.c index 278bf250..99a1ec29 100644 --- a/src/internal/floatscan.c +++ b/src/internal/floatscan.c @@ -301,7 +301,7 @@ static long double decfloat(FILE *f, int c, int bits, int emin, int sign, int po y -= bias; if ((e2+LDBL_MANT_DIG & INT_MAX) > emax-5) { - if (fabs(y) >= CONCAT(0x1p, LDBL_MANT_DIG)) { + if (fabsl(y) >= CONCAT(CONCAT(0x1p, LDBL_MANT_DIG), l)) { if (denormal && bits==LDBL_MANT_DIG+e2-emin) denormal = 0; y *= 0.5; -- 2.17.1
On Fri, Oct 18, 2019 at 07:02:11AM -0700, Dan Gohman wrote: > This fixes a compiler warning with clang: > > floatscan.c:304:13: warning: absolute value function 'fabs' given an > argument of type 'long double' but has parameter of type 'double' which may > cause truncation of value [-Wabsolute-value]. > > This does change the behavior of the expression because the value is no > longer rounded to double, however from my reading of the code, the rounding > doesn't seem intended. However, if it is, I suggest introducing an explicit > cast, to document the intent. > > Dan > From 1fecc521dc43b25366cd4a3062964ff3abc7506e Mon Sep 17 00:00:00 2001 > From: Dan Gohman <sunfish@mozilla.com> > Date: Fri, 18 Oct 2019 06:22:49 -0700 > Subject: [PATCH] Use `fabsl` instead of `fabs` on long double in floatscan.c > > This fixes a compiler warning: > > floatscan.c:304:13: warning: absolute value function 'fabs' given an argument > of type 'long double' but has parameter of type 'double' which may cause > truncation of value [-Wabsolute-value] If correct, the needed commit message here is not the warning but the behavioral fix the commit makes, if any. I haven't looked in detail yet, but I suspect there may be an issue with rounding and spurious raising of flags here, so I think the patch is correct and does matter. > --- > src/internal/floatscan.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/internal/floatscan.c b/src/internal/floatscan.c > index 278bf250..99a1ec29 100644 > --- a/src/internal/floatscan.c > +++ b/src/internal/floatscan.c > @@ -301,7 +301,7 @@ static long double decfloat(FILE *f, int c, int bits, int emin, int sign, int po > y -= bias; > > if ((e2+LDBL_MANT_DIG & INT_MAX) > emax-5) { > - if (fabs(y) >= CONCAT(0x1p, LDBL_MANT_DIG)) { > + if (fabsl(y) >= CONCAT(CONCAT(0x1p, LDBL_MANT_DIG), l)) { The double CONCAT definitely isn't needed. Comparison of fabsl(y) against 0x1p[LDBL_MANT_DIG] is well-defined, and has the same result. > if (denormal && bits==LDBL_MANT_DIG+e2-emin) > denormal = 0; > y *= 0.5; > -- > 2.17.1 >
* Dan Gohman <sunfish@mozilla.com> [2019-10-18 07:02:11 -0700]: > This fixes a compiler warning with clang: > > floatscan.c:304:13: warning: absolute value function 'fabs' given an > argument of type 'long double' but has parameter of type 'double' which may > cause truncation of value [-Wabsolute-value]. > > This does change the behavior of the expression because the value is no > longer rounded to double, however from my reading of the code, the rounding > doesn't seem intended. However, if it is, I suggest introducing an explicit > cast, to document the intent. > > Dan > From 1fecc521dc43b25366cd4a3062964ff3abc7506e Mon Sep 17 00:00:00 2001 > From: Dan Gohman <sunfish@mozilla.com> > Date: Fri, 18 Oct 2019 06:22:49 -0700 > Subject: [PATCH] Use `fabsl` instead of `fabs` on long double in floatscan.c > > This fixes a compiler warning: > > floatscan.c:304:13: warning: absolute value function 'fabs' given an argument > of type 'long double' but has parameter of type 'double' which may cause > truncation of value [-Wabsolute-value] the bug can cause spurious errno=ERANGE setting when converting decimal string to long double on targets where LDBL_MANT_DIG > DBL_MANT_DIG. i believe there is no other side-effect. example reproducer (should print 0 errno, current musl prints 34 on aarch64): #include <stdio.h> #include <stdlib.h> #include <errno.h> #include <float.h> int main() { char buf[100]; sprintf(buf, "%.42Le", LDBL_MAX); errno = 0; if (strtold(buf, 0) != LDBL_MAX) return -1; printf("%s %d\n", buf, errno); } > --- > src/internal/floatscan.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/internal/floatscan.c b/src/internal/floatscan.c > index 278bf250..99a1ec29 100644 > --- a/src/internal/floatscan.c > +++ b/src/internal/floatscan.c > @@ -301,7 +301,7 @@ static long double decfloat(FILE *f, int c, int bits, int emin, int sign, int po > y -= bias; > > if ((e2+LDBL_MANT_DIG & INT_MAX) > emax-5) { > - if (fabs(y) >= CONCAT(0x1p, LDBL_MANT_DIG)) { > + if (fabsl(y) >= CONCAT(CONCAT(0x1p, LDBL_MANT_DIG), l)) { > if (denormal && bits==LDBL_MANT_DIG+e2-emin) > denormal = 0; > y *= 0.5; > -- > 2.17.1 >
On Fri, Oct 18, 2019 at 04:49:09PM +0200, Szabolcs Nagy wrote:
> * Dan Gohman <sunfish@mozilla.com> [2019-10-18 07:02:11 -0700]:
> > This fixes a compiler warning with clang:
> >
> > floatscan.c:304:13: warning: absolute value function 'fabs' given an
> > argument of type 'long double' but has parameter of type 'double' which may
> > cause truncation of value [-Wabsolute-value].
> >
> > This does change the behavior of the expression because the value is no
> > longer rounded to double, however from my reading of the code, the rounding
> > doesn't seem intended. However, if it is, I suggest introducing an explicit
> > cast, to document the intent.
> >
> > Dan
>
> > From 1fecc521dc43b25366cd4a3062964ff3abc7506e Mon Sep 17 00:00:00 2001
> > From: Dan Gohman <sunfish@mozilla.com>
> > Date: Fri, 18 Oct 2019 06:22:49 -0700
> > Subject: [PATCH] Use `fabsl` instead of `fabs` on long double in floatscan.c
> >
> > This fixes a compiler warning:
> >
> > floatscan.c:304:13: warning: absolute value function 'fabs' given an argument
> > of type 'long double' but has parameter of type 'double' which may cause
> > truncation of value [-Wabsolute-value]
>
> the bug can cause spurious errno=ERANGE setting when
> converting decimal string to long double on targets
> where LDBL_MANT_DIG > DBL_MANT_DIG.
>
> i believe there is no other side-effect.
>
> example reproducer (should print 0 errno, current musl
> prints 34 on aarch64):
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <errno.h>
> #include <float.h>
> int main()
> {
> char buf[100];
> sprintf(buf, "%.42Le", LDBL_MAX);
> errno = 0;
> if (strtold(buf, 0) != LDBL_MAX) return -1;
> printf("%s %d\n", buf, errno);
> }
>
>
> > ---
> > src/internal/floatscan.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/internal/floatscan.c b/src/internal/floatscan.c
> > index 278bf250..99a1ec29 100644
> > --- a/src/internal/floatscan.c
> > +++ b/src/internal/floatscan.c
> > @@ -301,7 +301,7 @@ static long double decfloat(FILE *f, int c, int bits, int emin, int sign, int po
> > y -= bias;
> >
> > if ((e2+LDBL_MANT_DIG & INT_MAX) > emax-5) {
> > - if (fabs(y) >= CONCAT(0x1p, LDBL_MANT_DIG)) {
> > + if (fabsl(y) >= CONCAT(CONCAT(0x1p, LDBL_MANT_DIG), l)) {
> > if (denormal && bits==LDBL_MANT_DIG+e2-emin)
> > denormal = 0;
> > y *= 0.5;
> > --
> > 2.17.1
> >
Thanks for checking this. I'm committing with the double-concat
replaced with the 2/LDBL_EPSILON idiom used in printf; rather than
worrying about whether this part of the change is needed it seems to
make more sense just to get rid of the dependency on being able to do
this hackish token concatenaton.
Rich