Provide a C++ version of signbit that does not use __MATH_TG (bug 22296)

  The macro __MATH_TG contains the logic to select between long double and
_Float128, when these types are ABI-distinct.  This logic relies on
__builtin_types_compatible_p, which is not available in C++ mode.

On the other hand, C++ function overloading provides the means to
distinguish between the floating-point types.  The overloading
resolution will match the correct parameter regardless of type
qualifiers, i.e.: const and volatile.

This is the same fix as for issignaling [1] but the issue appear
only with gcc < 6.x. Since GCC 6.0, __builtin_signbit is type-generic,
so signbit() is defined without __MATH_TG.

Tested for x86_64.

	* math/math.h [defined __cplusplus] (signbit): Provide a C++
	definition for signbit that does not rely on __MATH_TG,
	since __MATH_TG uses __builtin_types_compatible_p, which is only
	available in C mode.

[1] a16e8bc08edca84d507715c66d6cddbbc7ed3b62

Cc: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
---
---
 ChangeLog   |  8 ++++++++
 math/math.h | 15 +++++++++++++++
 2 files changed, 23 insertions(+)
  

On Sun, 15 Oct 2017, Romain Naour wrote:

>--- a/math/math.h
>+++ b/math/math.h
>@@ -448,6 +448,21 @@ enum
> /* Return nonzero value if sign of X is negative.  */
> # if __GNUC_PREREQ (6,0)
> #  define signbit(x) __builtin_signbit (x)
>+# elif defined __cplusplus

I believe that the comment [1] about fpclassify applies to signbit, too,
because it is also the job of libstdc++ to provide it in C++11.

[1] https://sourceware.org/ml/libc-alpha/2017-09/msg00787.html

Adding a check for the standard in use (__cpluplus < 201103L) would
probably be safer and it would solve the problem with mesa, as described
in bug 22296.  However, it would not help with the configure check in
libstdc++ ("checking for ISO C99 support in <math.h> for C++11").

Moreover, I'm not sure if defining signbit as a function in math.h is
correct, even if it's only for C++11.  So, I suggest we wait for comments
from other, more experienced developers.  :)

>+inline int signbit (long double __val) { return __signbitl (__val); }

If defining signbit as a function in math.h is actually OK, then you need
to check for __NO_LONG_DOUBLE_MATH before calling __signbitl, since it is
only defined when long double is not the same as double.  Otherwise, call
__signbit.
  

On Mon, 16 Oct 2017, Gabriel F. T. Gomes wrote:

> On Sun, 15 Oct 2017, Romain Naour wrote:
> 
> >--- a/math/math.h
> >+++ b/math/math.h
> >@@ -448,6 +448,21 @@ enum
> > /* Return nonzero value if sign of X is negative.  */
> > # if __GNUC_PREREQ (6,0)
> > #  define signbit(x) __builtin_signbit (x)
> >+# elif defined __cplusplus
> 
> I believe that the comment [1] about fpclassify applies to signbit, too,
> because it is also the job of libstdc++ to provide it in C++11.

Yes, this case is like isinf and fpclassify: it's the job of libstdc++ to 
undefine these macros, and redefine as functions if necessary.  None of 
the C99 type-generic macros should be defined by glibc as functions for 
C++, because that's libstdc++'s job; the glibc definitions should only 
ever be used for C++ when running the libstdc++ configure test.  The TS 
18661 macros like issignaling are different because they aren't in 
standard C++ and libstdc++ doesn't provide its own versions of them.

My suggestion for signbit would be just to call __builtin_signbitl for the 
case of (C++, !__GNUC_PREREQ (6,0)), which may involve unnecessary 
conversions to long double when called at runtime (which shouldn't happen 
anyway) but should be sufficient for the configure test.
  

Hi Gabriel, Joseph,

Le 16/10/2017 à 19:18, Joseph Myers a écrit :
> On Mon, 16 Oct 2017, Gabriel F. T. Gomes wrote:
> 
>> On Sun, 15 Oct 2017, Romain Naour wrote:
>>
>>> --- a/math/math.h
>>> +++ b/math/math.h
>>> @@ -448,6 +448,21 @@ enum
>>> /* Return nonzero value if sign of X is negative.  */
>>> # if __GNUC_PREREQ (6,0)
>>> #  define signbit(x) __builtin_signbit (x)
>>> +# elif defined __cplusplus
>>
>> I believe that the comment [1] about fpclassify applies to signbit, too,
>> because it is also the job of libstdc++ to provide it in C++11.
> 
> Yes, this case is like isinf and fpclassify: it's the job of libstdc++ to 
> undefine these macros, and redefine as functions if necessary.  None of 
> the C99 type-generic macros should be defined by glibc as functions for 
> C++, because that's libstdc++'s job; the glibc definitions should only 
> ever be used for C++ when running the libstdc++ configure test.  The TS 
> 18661 macros like issignaling are different because they aren't in 
> standard C++ and libstdc++ doesn't provide its own versions of them.
> 
> My suggestion for signbit would be just to call __builtin_signbitl for the 
> case of (C++, !__GNUC_PREREQ (6,0)), which may involve unnecessary 
> conversions to long double when called at runtime (which shouldn't happen 
> anyway) but should be sufficient for the configure test.

Thanks for the review.

I'm not very familiar on the glibc code and how it interact with gcc build
system... But here is the updated patch following your advice:

https://sourceware.org/ml/libc-alpha/2017-10/msg00725.html

Best regards,
Romain