Bug 21930 - C-only gcc builtins used in <math.h> isinf
Summary: C-only gcc builtins used in <math.h> isinf
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: math (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: 2.27
Assignee: Gabriel F. T. Gomes
URL:
Keywords:
: 21983 22650 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-08-09 13:40 UTC by Leah Neukirchen
Modified: 2018-01-01 00:46 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Leah Neukirchen 2017-08-09 13:40:26 UTC
Since glibc 2.26, <math.h> uses __builtin_types_compatible_p and __typeof for isinf, which are GCC extensions that GCC 6.3 only provides in C mode.  This means <math.h> cannot be used from C++ programs.

Whether this in itself is harmful is questionable, but it certainly is a regression and implies at least one serious implication: the GCC 6.3 libstdc++v3 configure check "for ISO C99 support in <math.h> for C++98" now fails, resulting in a libstdc++ <cmath> header that simply includes <math.h>, which makes every C++ program fail to compile that correctly uses <cmath>.
Comment 1 Joseph Myers 2017-08-09 17:41:14 UTC
Gabriel, I had thought this usage - and the definition of __HAVE_GENERIC_SELECTION, which relates to a C-only feature but is not conditional on C in any way - was harmless because C++ doesn't actually use these macros (libstdc++ undefines them in its headers and defines its own function overloads).  Or, at least, that it was harmless except for issignaling, which libstdc++ doesn't handle.  But given the configure test, maybe that needs revisiting.

(a) Don't define __HAVE_GENERIC_SELECTION for C++.

(b) The use of __builtin_choose_expr is unnecessary, a conditional expression should suffice.  (__typeof *is* supported for C++, it's 
__builtin_choose_expr and __builtin_types_compatible_p that aren't.)

(c) Use some suitable C++ magic in place of __builtin_types_compatible_p (remembering that __builtin_types_compatible_p explicitly removes type qualifiers, and alternative C++ magic needs to do likewise).
Comment 2 Gabriel F. T. Gomes 2017-08-09 19:41:39 UTC
(In reply to Joseph Myers from comment #1)

> (a) Don't define __HAVE_GENERIC_SELECTION for C++.

OK.

> (b) The use of __builtin_choose_expr is unnecessary, a conditional
> expression should suffice.  (__typeof *is* supported for C++, it's 
> __builtin_choose_expr and __builtin_types_compatible_p that aren't.)

OK.

> (c) Use some suitable C++ magic in place of __builtin_types_compatible_p
> (remembering that __builtin_types_compatible_p explicitly removes type
> qualifiers, and alternative C++ magic needs to do likewise).

Since typeid also removes qualifiers implicitly (http://en.cppreference.com/w/cpp/language/typeid), would something in the lines of the following be acceptable for c++98:

if (typeid(f) == typeid(float)) isinff (f)

Likewise for the other types and for the other functions (defined through __MATH_TG)?


For C++11, we could use is_same, but we would need to remove the qualifiers explicitly... Something in the lines of:

if (std::is_same<std::remove_cv<typeof(f)>::type, float>::value)

This seems more complex, so I wonder if I'm missing something in the proposed test for c++98.
Comment 3 joseph@codesourcery.com 2017-08-09 20:22:52 UTC
On Wed, 9 Aug 2017, gftg at linux dot vnet.ibm.com wrote:

> > (c) Use some suitable C++ magic in place of __builtin_types_compatible_p
> > (remembering that __builtin_types_compatible_p explicitly removes type
> > qualifiers, and alternative C++ magic needs to do likewise).
> 
> Since typeid also removes qualifiers implicitly
> (http://en.cppreference.com/w/cpp/language/typeid), would something in the
> lines of the following be acceptable for c++98:
> 
> if (typeid(f) == typeid(float)) isinff (f)

No, because that involves a runtime comparison of type_info objects.  We 
want something (to go in a __MATH_TG macro definition for the "C++ and 
distinct float128" case - and, similarly, an isinf definition for when C++ 
is combined with the present conditions for handling GCC < 7 with 
_Float128) that's optimized away at compile time - something where the 
conditionals are always folded to true or false.  (And preferably not 
depending on including any C++ headers from <math.h>; I don't know what 
the rules are for such header inclusion in the C++ standard library.)

Maybe we need to understand further what cases are the problem.  Various 
macros may be used in that configure test, but most of them are using 
type-generic built-in functions which work fine for C++ (given that 
libstdc++ is not built with sNaNs enabled).  The problem for the configure 
test might only be the "__builtin_isinf_sign is broken for float128 only 
before GCC 7.0." definition of isinf, which uses 
__builtin_types_compatible_p.  Separately, with the installed compiler, 
most of the macros should be undefined in the libstdc++ headers, leaving 
issignaling as the likely problem case using __MATH_TG (issubnormal is 
defined using fpclassify; iszero has a separate C++ template definition, 
to avoid problems with macros).

Possible solutions for isinf include:

* Do not use the special-case isinf definition with 
__builtin_types_compatible_p for C++.  It's possible this suffices to make 
the configure test work (and exactly what the isinf macro does doesn't 
matter when using an installed C++ compiler because the libstdc++ headers 
undefine that macro, and the configure test doesn't use it with float128).

* Add a C++ variant of that definition, which would only be used for the 
configure test but which still has the right semantics.

And for issignaling:

* Change __MATH_TG to have a fully functional C++ variant.  float and 
double can be distinguished with sizeof; it's only long double / float128 
where you need a replacement for __builtin_types_compatible_p (__typeof 
(TG_ARG), long double) using C++ features.

* Add template versions of issignaling for C++, like iszero.
Comment 4 Gabriel F. T. Gomes 2017-08-15 17:56:14 UTC
(In reply to joseph@codesourcery.com from comment #3)

> * Do not use the special-case isinf definition with 
> __builtin_types_compatible_p for C++.  It's possible this suffices to make 
> the configure test work

Indeed, this is enough for the configure test to work.  I sent this change as:

https://sourceware.org/ml/libc-alpha/2017-08/msg00588.html


The other emails in the thread address the other concerns raised in this bug report.
Comment 5 cvs-commit@gcc.gnu.org 2017-08-18 15:28:04 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  6913ad65e00bb32417ad39c41d292b976171e27e (commit)
       via  47a67213a9f51c5f8816d240500b10db605d8b77 (commit)
      from  c647fb885cb678471f6b6a66f394b4ca5515a016 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=6913ad65e00bb32417ad39c41d292b976171e27e

commit 6913ad65e00bb32417ad39c41d292b976171e27e
Author: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
Date:   Mon Aug 14 17:51:51 2017 -0300

    Do not use generic selection in C++ mode
    
    The logic to protect the use of generic selection (_Generic) does not
    check for C or C++ mode, however, generic selection is a C-only
    feature.
    
    Tested for powerpc64le.
    
    	* misc/sys/cdefs.h (__HAVE_GENERIC_SELECTION): Define to 0, if
    	in C++ mode.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=47a67213a9f51c5f8816d240500b10db605d8b77

commit 47a67213a9f51c5f8816d240500b10db605d8b77
Author: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
Date:   Fri Aug 11 14:29:06 2017 -0300

    Do not use __builtin_types_compatible_p in C++ mode (bug 21930)
    
    The logic to define isinf for float128 depends on the availability of
    __builtin_types_compatible_p, which is only available in C mode,
    however, the conditionals do not check for C or C++ mode.  This lead to
    an error in libstdc++ configure, as reported by bug 21930.
    
    This patch adds a conditional for C mode in the definition of isinf for
    float128.  No definition is provided in C++ mode, since libstdc++
    headers undefine isinf.
    
    Tested for powerpc64le (glibc test suite and libstdc++-v3 configure).
    
    	[BZ #21930]
    	* math/math.h (isinf): Check if in C or C++ mode before using
    	__builtin_types_compatible_p, since this is a C mode feature.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog        |   11 +++++++++++
 math/math.h      |    8 ++++++--
 misc/sys/cdefs.h |   19 ++++++++++---------
 3 files changed, 27 insertions(+), 11 deletions(-)
Comment 6 Andreas Schwab 2017-08-21 10:19:35 UTC
*** Bug 21983 has been marked as a duplicate of this bug. ***
Comment 7 Andreas Schwab 2017-08-21 10:21:30 UTC
There are more uses of __builtin_types_compatible_p, enabled with -fsignaling-nans.
Comment 8 cvs-commit@gcc.gnu.org 2017-08-21 12:41:24 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.26/master has been updated
       via  5e989c36934d0f0cf13b7a53ef2fa440bce39210 (commit)
       via  c2921b17a37e887b8a5ca9d84b875b9ba702b79c (commit)
      from  645b7635ba8fd58062245419e8bb668ab90cd3ec (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=5e989c36934d0f0cf13b7a53ef2fa440bce39210

commit 5e989c36934d0f0cf13b7a53ef2fa440bce39210
Author: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
Date:   Mon Aug 14 17:51:51 2017 -0300

    Do not use generic selection in C++ mode
    
    The logic to protect the use of generic selection (_Generic) does not
    check for C or C++ mode, however, generic selection is a C-only
    feature.
    
    Tested for powerpc64le.
    
    	* misc/sys/cdefs.h (__HAVE_GENERIC_SELECTION): Define to 0, if
    	in C++ mode.
    
    (cherry picked from commit 6913ad65e00bb32417ad39c41d292b976171e27e)

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=c2921b17a37e887b8a5ca9d84b875b9ba702b79c

commit c2921b17a37e887b8a5ca9d84b875b9ba702b79c
Author: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
Date:   Mon Aug 21 14:23:27 2017 +0200

    Do not use __builtin_types_compatible_p in C++ mode (bug 21930)
    
    The logic to define isinf for float128 depends on the availability of
    __builtin_types_compatible_p, which is only available in C mode,
    however, the conditionals do not check for C or C++ mode.  This lead to
    an error in libstdc++ configure, as reported by bug 21930.
    
    This patch adds a conditional for C mode in the definition of isinf for
    float128.  No definition is provided in C++ mode, since libstdc++
    headers undefine isinf.
    
    Tested for powerpc64le (glibc test suite and libstdc++-v3 configure).
    
    	[BZ #21930]
    	* math/math.h (isinf): Check if in C or C++ mode before using
    	__builtin_types_compatible_p, since this is a C mode feature.
    
    (cherry picked from commit 47a67213a9f51c5f8816d240500b10db605d8b77)

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog        |   11 +++++++++++
 NEWS             |    1 +
 math/math.h      |    8 ++++++--
 misc/sys/cdefs.h |   19 ++++++++++---------
 4 files changed, 28 insertions(+), 11 deletions(-)
Comment 9 joseph@codesourcery.com 2017-08-21 13:37:49 UTC
Most of the macros, including fpclassify, should be irrelevant because 
libstdc++ headers undefine the macros and replace them with function 
overloads (as required by the C++ standard).  The problem with iszero is 
specifically that the C++ iszero definition in the C math.h is using the 
macro fpclassify definition (this is before libstdc++ gets a chance to 
undefine it), and, in the case (__SUPPORT_SNAN__ and float128 supported), 
that definition is using __builtin_types_compatible_p.  So some approach 
is needed for iszero to work in that place - without conflicting with 
libstdc++'s own fpclassify overloads.
Comment 10 joseph@codesourcery.com 2017-08-21 13:46:11 UTC
I will add, for completeness: I don't consider the uses of 
__builtin_types_compatible_p in tgmath.h for float128 to be a bug.  For 
C++11 and later libstdc++'s tgmath.h does not include the C library 
header, and the C macros in tgmath.h rely on details of C type conversions 
which don't work for C++ anyway.
Comment 11 Gabriel F. T. Gomes 2017-08-21 14:04:30 UTC
For iszero, I could write a patch similar to the one I wrote (and that is still under review) for issignaling (https://sourceware.org/ml/libc-alpha/2017-08/msg00859.html).  That would make iszero use the correct __fpclassify* function for each type, instead of relying on the type-generic fpclassify macro.

Does that sound like a good approach for this problem?
Comment 12 joseph@codesourcery.com 2017-08-21 14:16:47 UTC
That approach to iszero seems reasonable (provided it's only for the 
__SUPPORT_SNAN__ case, the case where iszero just compares with zero can 
stay with the template).
Comment 13 Gabriel F. T. Gomes 2017-08-25 11:14:20 UTC
I posted a patch [1] to libc-alpha that addesses the problem with iszero and -fsignaling-nans (if I'm not mistaken, this is the only problem mentioned in this bug report that has not yet being dealt with).

[1] https://sourceware.org/ml/libc-alpha/2017-08/msg01163.html
Comment 14 cvs-commit@gcc.gnu.org 2017-08-28 19:26:35 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  42496114ec0eb7d6d039d05d4262e109951c600c (commit)
      from  56bc7f43603b5d28437496efb32df40997c62cb4 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=42496114ec0eb7d6d039d05d4262e109951c600c

commit 42496114ec0eb7d6d039d05d4262e109951c600c
Author: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
Date:   Tue Aug 22 16:34:42 2017 -0300

    Provide a C++ version of iszero that does not use __MATH_TG (bug 21930)
    
    When signaling nans are enabled (with -fsignaling-nans), the C++ version
    of iszero uses the fpclassify macro, which is defined with __MATH_TG.
    However, when support for float128 is available, __MATH_TG uses the
    builtin __builtin_types_compatible_p, which is only available in C mode.
    
    This patch refactors the C++ version of iszero so that it uses function
    overloading to select between the floating-point types, instead of
    relying on fpclassify and __MATH_TG.
    
    Tested for powerpc64le, s390x, x86_64, and with build-many-glibcs.py.
    
    	[BZ #21930]
    	* math/math.h [defined __cplusplus && defined __SUPPORT_SNAN__]
    	(iszero): New C++ implementation that does not use
    	fpclassify/__MATH_TG/__builtin_types_compatible_p, when
    	signaling nans are enabled, since __builtin_types_compatible_p
    	is a C-only feature.
    	* math/test-math-iszero.cc: When __HAVE_DISTINCT_FLOAT128 is
    	defined, include ieee754_float128.h for access to the union and
    	member ieee854_float128.ieee.
    	[__HAVE_DISTINCT_FLOAT128] (do_test): Call check_float128.
    	[__HAVE_DISTINCT_FLOAT128] (check_float128): New function.
    	* sysdeps/powerpc/powerpc64le/Makefile [subdir == math]
    	(CXXFLAGS-test-math-iszero.cc): Add -mfloat128 to the build
    	options of test-math-zero on powerpc64le.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                            |   17 +++++++
 math/math.h                          |   33 ++++++++++++--
 math/test-math-iszero.cc             |   79 ++++++++++++++++++++++++++++++++++
 sysdeps/powerpc/powerpc64le/Makefile |    3 +-
 4 files changed, 127 insertions(+), 5 deletions(-)
Comment 15 Gabriel F. T. Gomes 2017-08-28 19:35:23 UTC
Fixed for 2.27.
Comment 16 cvs-commit@gcc.gnu.org 2017-08-31 19:25:02 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, release/2.26/master has been updated
       via  58270c0049404ef2f878fdd45df55f17f0b8c1f7 (commit)
       via  35dded99a89db873b06270ca7f21245a0faf712a (commit)
       via  ef8566d72af5e03c1b82cf02efb794268a347f8c (commit)
      from  6043d77a47de297b62084c1c261cdada082bf09c (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=58270c0049404ef2f878fdd45df55f17f0b8c1f7

commit 58270c0049404ef2f878fdd45df55f17f0b8c1f7
Author: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
Date:   Tue Aug 22 16:34:42 2017 -0300

    Provide a C++ version of iszero that does not use __MATH_TG (bug 21930)
    
    When signaling nans are enabled (with -fsignaling-nans), the C++ version
    of iszero uses the fpclassify macro, which is defined with __MATH_TG.
    However, when support for float128 is available, __MATH_TG uses the
    builtin __builtin_types_compatible_p, which is only available in C mode.
    
    This patch refactors the C++ version of iszero so that it uses function
    overloading to select between the floating-point types, instead of
    relying on fpclassify and __MATH_TG.
    
    Tested for powerpc64le, s390x, x86_64, and with build-many-glibcs.py.
    
    	[BZ #21930]
    	* math/math.h [defined __cplusplus && defined __SUPPORT_SNAN__]
    	(iszero): New C++ implementation that does not use
    	fpclassify/__MATH_TG/__builtin_types_compatible_p, when
    	signaling nans are enabled, since __builtin_types_compatible_p
    	is a C-only feature.
    	* math/test-math-iszero.cc: When __HAVE_DISTINCT_FLOAT128 is
    	defined, include ieee754_float128.h for access to the union and
    	member ieee854_float128.ieee.
    	[__HAVE_DISTINCT_FLOAT128] (do_test): Call check_float128.
    	[__HAVE_DISTINCT_FLOAT128] (check_float128): New function.
    	* sysdeps/powerpc/powerpc64le/Makefile [subdir == math]
    	(CXXFLAGS-test-math-iszero.cc): Add -mfloat128 to the build
    	options of test-math-zero on powerpc64le.
    
    (cherry picked from commit 42496114ec0eb7d6d039d05d4262e109951c600c)

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=35dded99a89db873b06270ca7f21245a0faf712a

commit 35dded99a89db873b06270ca7f21245a0faf712a
Author: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
Date:   Wed Aug 23 10:16:54 2017 -0300

    Fix the C++ version of issignaling when __NO_LONG_DOUBLE_MATH is defined
    
    When __NO_LONG_DOUBLE_MATH is defined, __issignalingl is not available,
    thus issignaling with long double argument should call __issignaling,
    instead.
    
    Tested for powerpc64le.
    
    	* math/math.h [defined __cplusplus] (issignaling): In the long
    	double case, call __issignalingl only if __NO_LONG_DOUBLE_MATH
    	is not defined.  Call __issignaling, otherwise.
    
    (cherry picked from commit 3d7b66f66cb223e899a7ebc0f4c20f13e711c9e0)

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=ef8566d72af5e03c1b82cf02efb794268a347f8c

commit ef8566d72af5e03c1b82cf02efb794268a347f8c
Author: Gabriel F. T. Gomes <gftg@linux.vnet.ibm.com>
Date:   Mon Aug 14 13:46:15 2017 -0300

    Provide a C++ version of issignaling that does not use __MATH_TG
    
    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.
    
    Tested for powerpc64le, s390x, and x86_64.
    
    	* math/math.h [defined __cplusplus] (issignaling): Provide a C++
    	definition for issignaling that does not rely on __MATH_TG,
    	since __MATH_TG uses __builtin_types_compatible_p, which is only
    	available in C mode.
    	(CFLAGS-test-math-issignaling.cc): New variable.
    	* math/Makefile [CXX] (tests): Add test-math-issignaling.
    	* math/test-math-issignaling.cc: New test for C++ implementation
    	of type-generic issignaling.
    	* sysdeps/powerpc/powerpc64le/Makefile [subdir == math]
    	(CXXFLAGS-test-math-issignaling.cc): Add -mfloat128 to the build
    	options of test-math-issignaling on powerpc64le.
    
    (cherry picked from commit a16e8bc08edca84d507715c66d6cddbbc7ed3b62)

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                          |   37 +++++++++
 math/Makefile                                      |    3 +-
 math/math.h                                        |   60 ++++++++++++++-
 ...est-math-iszero.cc => test-math-issignaling.cc} |   78 +++++++++++++------
 math/test-math-iszero.cc                           |   79 ++++++++++++++++++++
 sysdeps/powerpc/powerpc64le/Makefile               |    4 +-
 6 files changed, 229 insertions(+), 32 deletions(-)
 copy math/{test-math-iszero.cc => test-math-issignaling.cc} (52%)
Comment 17 Joseph Myers 2018-01-01 00:46:42 UTC
*** Bug 22650 has been marked as a duplicate of this bug. ***