* [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c
@ 2019-10-18 14:02 Dan Gohman
2019-10-18 14:23 ` Rich Felker
2019-10-18 14:49 ` Szabolcs Nagy
0 siblings, 2 replies; 4+ messages in thread
From: Dan Gohman @ 2019-10-18 14:02 UTC (permalink / raw)
To: musl
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c
2019-10-18 14:02 [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c Dan Gohman
@ 2019-10-18 14:23 ` Rich Felker
2019-10-18 14:49 ` Szabolcs Nagy
1 sibling, 0 replies; 4+ messages in thread
From: Rich Felker @ 2019-10-18 14:23 UTC (permalink / raw)
To: musl
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c
2019-10-18 14:02 [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c Dan Gohman
2019-10-18 14:23 ` Rich Felker
@ 2019-10-18 14:49 ` Szabolcs Nagy
2019-10-19 1:19 ` Rich Felker
1 sibling, 1 reply; 4+ messages in thread
From: Szabolcs Nagy @ 2019-10-18 14:49 UTC (permalink / raw)
To: musl
* 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c
2019-10-18 14:49 ` Szabolcs Nagy
@ 2019-10-19 1:19 ` Rich Felker
0 siblings, 0 replies; 4+ messages in thread
From: Rich Felker @ 2019-10-19 1:19 UTC (permalink / raw)
To: musl
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-19 1:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 14:02 [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c Dan Gohman
2019-10-18 14:23 ` Rich Felker
2019-10-18 14:49 ` Szabolcs Nagy
2019-10-19 1:19 ` Rich Felker
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).