Re: CURRENT: CLANG 3.3 and -stad=c++11 and -stdlib=libc++: isnan()/isninf() oddity

On Thu, 11 Jul 2013, David Chisnall wrote:

> You're joining in this discussion starting in the middle, so you probably missed the earlier explanation.

I was mainly addressing a C99 point.  I know little about C++ or C11.

> On 11 Jul 2013, at 05:21, Bruce Evans <brde_at_optusnet.com.au> wrote:
>
>> I don't see how any conforming program can access the isnan() function
>> directly.  It is just as protected as __isnan() would be.  (isnan)()
>> gives the function (the function prototype uses this), but conforming
>> programs can't do that since the function might not exist.  Maybe some
>> non-conforming program like autoconfig reads <math.h> or libm.a and
>> creates a bug for C++.
>
> The cmath header defines a template function isnan that invokes the isnan macro, but then undefines the isnan macro.  This causes a problem because when someone does something along the lines of using namespace std then they end up with two functions called isnan and the compiler gets to pick the one to use.  Unfortunately, std::isnan() returns a bool, whereas isnan() returns an int.
>
> The C++ headers are not required to be conforming C code, because they are not C, and our math.h causes namespace pollution in C++ when included from <cmath>.

<math.h> is also not required to be conforming C code, let alone C++ code,
so there is only a practical requirement that it works when included
in the C++ implementation.

>> The FreeBSD isnan() implementation would be broken by removing the
>> isnan() function from libm.a or ifdefing it in <math.h>.  Changing the
>> function to __isnan() would cause compatibility problems.  The function
>> is intentionally named isnan() to reduce compatibility problems.
>
> On OS X this is avoided because their isnan() macro expands to call one of the __-prefixed inline functions (which adopt your suggestion of being implemented as x != x, for all types).  I am not sure that this is required for standards conformance, but it is certainly cleaner.  Your statement that having the function not called isnan() causes compatibility problems is demonstrably false, as neither OS X nor glibc has a function called isnan() and, unlike us, they do not experience problems with this macro.

The compatibility that I'm talking about is with old versions of FreeBSD.
isnan() is still in libc as a function since that was part of the FreeBSD
ABI and too many things depended on getting it from there.  It was recently
removed from libc.so, but is still in libm.a.  This causes some
implementation problems in libm that are still not completely solved.  I
keep having to edit msun/src/s_isnan.c the msun sources are more portable.
Mostly I need to kill the isnan() there so that it doesn't get in the
way of the one in libc.  This mostly works even if there is none in libc,
since the builtins result in neither being used.  isnanf() is more of a
problem, since it is mapped to __isnanf() and there is no builtin for
__isnanf().  The old functions have actually been removed from libc.a
too.  They only in libc_pic.a.  libc.a still has isnan.o, but that is bogus
since isnan.o is now empty.

> It would also be nice to implement these macros using _Generic when compiling in C11 mode, as it will allow the compiler to produce more helpful warning messages.  I would propose this implementation:

> #if __has_builtin(__builtin_isnan)

This won't work for me, since I develop and test msun with old compilers
that don't support __has_builtin().  Much the same set of compilers also
don't have enough FP builtins.

It also doesn't even work.  clang has squillions of builtins that
aren't really builtines so they reduce to libcalls.  gcc has fewer
builtins, but still many that reduce to libcalls.  An example is fma().
__has_builtin(__builtin_fma) is true for clang on amd64 (freefall),
but at least freefalls's CPU doesn't support fma in hardware, so the
builtin can't really work, and in fact it doesn't -- it reduces to a
libcall.  This might change if the hardware supports fma, but then
__has_builtin(__builtin_fma) would be even more useless for telling
if fma is worth using.  C99 has macros FP_FAST_FMA[FL] whose
implementation makes them almost equally useless.  For example, ia64
has fma in hardware and the implementation defines all of
FP_FAST_FMA[FL] for ia64.  But fma is implemented as an extern function,
partly because there is no way to tell if __builtin_fma is any good
(but IIRC, __builtin_fma is no good on ia64 either, since it reduces
to the same extern function).  The extern function is slow (something
like 20 cycles instead of 1 for the fma operation).  But if you ignore
the existence of the C99 fma API and just write expressions of the
form (a*x + b), then gcc on ia64 will automatically use the hardware
fma, although this is technically wrong in some fenv environments.

For gcc-4.2.1, __has_builtin(__builtin_fma) is a syntax error.  I
test with gcc-3.x.  It is also missing __builtin_isnan().

The msun implementation knows that isnan() and other classification
macros are too slow to actually use, and rarely uses them.  Sometimes
it is inherent that it can do better, because it already knows the
bits in the macros and can test the bits directly.  I have had limited
tests with arranging access macros so that loads of the bits can be
shared.

> #define isnan(x) __builtin_isnan(x)
> #else
> static __inline int
> __isnanf(float __x)
> {
>        return (__x != __x);
> }

Here we can do better in most cases by hard-coding this without the ifdef.

> static __inline int
> __isnand(double __x)
> {
>        return (__x != __x);
> }

__isnand() is a strange name, and doesn't match compiler conventions for
builtins.  Compilers use __builtin_isnan() and map this to the libcall
__isnan().

> static __inline int
> __isnanl(long double __x)
> {
>        return (__x != __x);
> }
> #if __STDC_VERSION__ >= 201112L
> #define isnan(x) _Generic((x),     \
>        float: __isnanf(x),        \
>        double: __isnand(x),       \
>        long double: __isnanl(x))

Does _Generic() have no side effects, like sizeof()?

> #else
> #define isnan(x)                                                \
>        ((sizeof (x) == sizeof (float)) ? __isnanf(x)           \
>             : (sizeof (x) == sizeof (double)) ? __isnand(x)    \
>             : __isnanl(x))
> #endif
> #endif

Both cases need to use __builtin_isnan[fl]() and depend on compiler
magic to have any chance of avoiding side effects from loads and
parameter passing.

Generic stuff doesn't seem to work right for either isnan() or
__builtin_isnan(), though it could for at least the latter.  According
to a quick grep of strings $(which clang), __builtin_classify() is
generic but __builtin_isnan*() isn't (the former has no type suffixes
but the latter does, and testing shows that the latter doesn't work
without the suffices).  It is the most useful FP builtins like
__builtin_fabs*() that aren't generic.  C99 probably prevents fabs*()
being generic, but I think any builtin can be generic, and the isnan()
macro is specified to be generic, so if compilers understood it directly
their builtin for it needs to be and can be generic.

> For a trivial example of why this is an improvement in terms of error reporting, consider this trivial piece of code:
>
> int is(int x)
> {
>        return isnan(x);
> }
>
> With our current implementation, this compiles and links without any warnings, although it will have somewhat interesting results at run time.  With the __builtin_isnan() version, clang reports this error:
>
> isnan.c:35:15: error: floating point classification requires argument of
>      floating point type (passed in 'int')
>        return isnan(x);
>                     ^
> (and then some more about the macro expansion)
>
> With the C11 version, it reports this error:
>
> isnan.c:35:15: error: controlling expression type 'int' not compatible with any
>      generic association type
>        return isnan(x);
>                     ^

The error message for the __builtin_isnan() version is slightly better up
to where it says more.

The less-unportable macro can do more classification and detect problems
at compile time using __typeof().

isnan(integer) = 0 with no warnings is quite good though.  Integers are
never NaNs, and converting an integer to a floating point type should
never give a NaN.

Bruce