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

[Rich Felker <dalias@libc.org>]

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