[PATCH] Use fabsl instead of fabs on long double operand in floatscan.c

[PATCH] Use fabsl instead of fabs on long double operand in floatscan.c

[Dan Gohman]

[-- 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

Re: [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c

[Rich Felker]

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
> 

Re: [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c

[Szabolcs Nagy]

* 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
> 

Re: [PATCH] Use fabsl instead of fabs on long double operand in floatscan.c

[Rich Felker]

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