svn commit: r326342 - head/lib/libc/mips/gen

[Bruce Evans]

On Tue, 28 Nov 2017, Alex Richardson wrote:

> Log:
>  Fix fabs() for MIPS when used on -0.0

Only arm and mips have this in C in libc.  arm still has the broken version,

libm has better versions.  Only 1 in C and none in asm, where libc has
many C and asm versions with at least 1 still wrong.  The version in
libc is almost never used, ust like the versions in libc, since the
builtin versions are usually used automatically.

>  It would previously return negative zero for -0.0 since -0.0 does not
>  compare less than 0. The issue was discovered when running the libc++
>  test suite on softfloat MIPS64.

Since the builtin is usually used, test programs usually don't find this
bug.  Presumably softfloat turns off the builtin, but how does your fix
work then (see below)?  For testing libm, I prevent use of builtins and
even in functions in the standard library by renaming functions in a copy
of the source code and testing the renamed functions.  -fno-builtin
should work for turning off builtins.

>  I have verified that both clang and GCC generate sensible code for the
>  builtin. For soft float they clear the sign bit using integer operations
>  and in hard float mode they use abs.d.

Seems good enough for a single arch.  Did you test O0 -fno-builtin?  -O0
should be supported for debugging, but it is not so reasonable to compile
libc with -fno-builtin.

> Modified: head/lib/libc/mips/gen/fabs.c
> ==============================================================================
> --- head/lib/libc/mips/gen/fabs.c	Tue Nov 28 19:57:16 2017	(r326341)
> +++ head/lib/libc/mips/gen/fabs.c	Tue Nov 28 20:37:27 2017	(r326342)
> @@ -42,7 +42,6 @@ __FBSDID("$FreeBSD$");
> double
> fabs(double x)
> {
> -	if (x < 0)
> -		x = -x;
> -	return(x);
> +
> +	return (__builtin_fabs(x));
> }

Why doesn't the libc++ test suite on softfloat MIPS64 hide the bug by using
the builtin instead of this function?  If the builtin is turned off completely,
then the above either doesn't compile or generates a call to the nonexistent
public function __builtin_fabs().

The builtin isn't useable in general because some compilers don't have it,
and ones that have it might have a fake version which calls the non-builtin
version which gives endless recursion.  This normally calls the function of
the same name (fabs here) instead of a function with the underscored name.

libm's semi-portable implementation for all arches is:

X double
X fabs(double x)
X {
X 	u_int32_t high;
X 	GET_HIGH_WORD(high,x);
X 	SET_HIGH_WORD(x,high&0x7fffffff);
X         return x;
X }

This works on any arch with IEEE754 doubles.

It works by zeroing the sign bit.  Compilers should turn this into the
same code as the builtin, and clang on amd64 does (clang on i386 is not
so good).  However, older compilers tend to pessimize by copying through
32-bit integer registers or the stack just like the code asks for.

Bruce