Test overload_numeric failed against GCC 6

[jplesnik]

I tried to rebuild swig 3.0.8 against GCC 6, but the python testcase overload_numeric failed. I got following error:

checking python testcase overload_numeric (with run test)
Traceback (most recent call last):
  File "overload_numeric_runme.py", line 37, in <module>
    check(nums.over(float("inf")), "float")
  File "overload_numeric_runme.py", line 11, in check
    raise RuntimeError("got: " + got + " expected: " + expected)
RuntimeError: got: double expected: float
Makefile:103: recipe for target 'overload_numeric.cpptest' failed
make[1]: *** [overload_numeric.cpptest] Error 1

GCC 6 is part of the Fedora Rawhide.

The test passed with GCC 5.3.1.

[wsfulton]

which version of Python?

[ojwb]

Works for me on Debian unstable x86-64 with:

g++-6 (Debian 6-20160117-1) 6.0.0 20160117 (experimental) [trunk revision 232481]
Python 2.7.11

I wonder if it's an optimisation issue - @jplesnik: can you try:

make -C Examples/test-suite/python overload_numeric.cpptest CXXFLAGS='-fno-strict-aliasing -fwrapv'

Python says we should use these flags:

$ python-config --cflags
-I/usr/include/python2.7 -I/usr/include/x86_64-linux-gnu/python2.7  -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security  -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes

A newer GCC version might well optimise more based on alias analysis or integer overflow analysis. We should probably be asking python what flags to compile the C/C++ code with in configure.

[jplesnik]

Version of Python version is

Python 2.7.11

The version of GCC is

gcc version 6.0.0 20160212 (Red Hat 6.0.0-0.11) (GCC) 

@ojwb: I tried the command, but it failed

$ make -C Examples/test-suite/python overload_numeric.cpptest CXXFLAGS='-fno-strict-aliasing -fwrapv'
make: Entering directory '/root/swig/Examples/test-suite/python'
if [ -f ./overload_numeric_runme.py ]; then echo "checking python testcase overload_numeric (with run test)" ; else echo "checking python testcase overload_numeric" ; fi;
checking python testcase overload_numeric (with run test)
make -f ../../../Examples/Makefile SRCDIR='./' CXXSRCS='' SWIG_LIB_DIR='../../../Lib' SWIGEXE='../../../swig' INCLUDES='-I../../../Examples/test-suite' SWIGOPT='-outcurrentdir -I../../../Examples/test-suite' NOLINK=true TARGET='overload_numeric' INTERFACEDIR='../' INTERFACE='overload_numeric.i' python_cpp
make[1]: Entering directory '/root/swig/Examples/test-suite/python'
env SWIG_LIB=../../../Lib  ../../../swig -python -c++ -outcurrentdir -I../../../Examples/test-suite -o overload_numeric_wrap.cxx ./../overload_numeric.i
g++ -c -fpic -I. -I./ -fno-strict-aliasing -fwrapv overload_numeric_wrap.cxx   -I../../../Examples/test-suite  -I/usr/include/python2.7 -I/usr/lib/python2.7/config
g++ -shared  -fno-strict-aliasing -fwrapv    overload_numeric_wrap.o    -o _overload_numeric.so
make[1]: Leaving directory '/root/swig/Examples/test-suite/python'
if [ -f ./overload_numeric_runme.py ]; then env LD_LIBRARY_PATH=.:$LD_LIBRARY_PATH PYTHONPATH=.:.:$PYTHONPATH  python overload_numeric_runme.py; fi
Traceback (most recent call last):
  File "overload_numeric_runme.py", line 37, in <module>
    check(nums.over(float("inf")), "float")
  File "overload_numeric_runme.py", line 11, in check
    raise RuntimeError("got: " + got + " expected: " + expected)
RuntimeError: got: double expected: float
Makefile:103: recipe for target 'overload_numeric.cpptest' failed
make: *** [overload_numeric.cpptest] Error 1
make: Leaving directory '/root/swig/Examples/test-suite/python'

$ python-config --cflags
-I/usr/include/python2.7 -I/usr/include/python2.7 -fno-strict-aliasing -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -D_GNU_SOURCE -fPIC -fwrapv -DNDEBUG -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -D_GNU_SOURCE -fPIC -fwrapv

[ojwb]

I noticed there was a newer GCC 6 snapshot available in Debian experimental than the one I already had installed - with the newer version I get the error.

g++-6 (Debian 6-20160205-1) 6.0.0 20160205 (experimental) [trunk revision 233183]

So this seems to be related to some change between these versions:

g++-6 (Debian 6-20160117-1) 6.0.0 20160117 (experimental) [trunk revision 232481]
g++-6 (Debian 6-20160205-1) 6.0.0 20160205 (experimental) [trunk revision 233183]

Looking at the generated code, the issue seems to be that the SWIG_isfinite macro doesn't get defined.

I think the root cause is that isfinite(x) is a function for C++11 (for C99 it's a macro). So probably the test should be:

# if defined(isfinite) || (defined __cplusplus && __cplusplus >= 201103L)

[wsfulton]

As we are including math.h instead of cmath, I think g++ should have this as a macro, but as it isn't, you'd better put in your proposed change.

[ojwb]

Hmm, that failed in travis (but was OK for me). If I decode the error correctly, I think we need to use std::isfinite().

[ojwb]

Hmm, I noted this as a fix for Python in CHANGES.current, but I notice the change is actually to a generic typemap file. @wsfulton - am I right in thinking this is likely to fix similar issues for several languages?

[wsfulton]

Yes indeed.

[wsfulton]

@ojwb, could you correct the CHANGES file? We can then close this issue.

[ojwb]

I'm not really sure what to put instead, so I just deleted [Python]. I wonder if you had more in mind, as it would have been less effort for you just to change that than to ask me to do it...

Anyway, feel free to improve.

[wsfulton]

That's what I do too if it affects many languages.

[GitAClue]

The original fix was good, but the follow-up commit is incorrect and causes failures like the one below with Clang 3.8 as compiler. Since the header include is math.h, isfinite is not in the std namespace. You must include cmath if you are going to use std::isfinite. I had to revert 75510a1 locally to correct this problem.

modperl/ZNC.cpp:2573:9: error: no member named 'isfinite' in namespace 'std'; 
did you mean simply 'isfinite'?
    if (SWIG_Float_Overflow_Check(v)) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
modperl/ZNC.cpp:2561:73: note: expanded from macro 'SWIG_Float_Overflow_Check'
# define SWIG_Float_Overflow_Check(X) ((X < -FLT_MAX || X > FLT_MAX) && 
SWIG_isfinite(X))

^~~~~~~~~~~~~~~~
modperl/ZNC.cpp:2549:29: note: expanded from macro 'SWIG_isfinite'
#  define SWIG_isfinite(X) (std::isfinite(X))
                            ^~~~~
/usr/include/c++/v1/math.h:380:1: note: 'isfinite' declared here
isfinite(_A1 __lcpp_x) _NOEXCEPT
^
2 warnings and 1 error generated.

For full context, see https://www.mail-archive.com/freebsd-pkg-fallout@freebsd.org/msg313198.html https://www.mail-archive.com/freebsd-pkg-fallout@freebsd.org/msg311258.html https://www.mail-archive.com/freebsd-pkg-fallout@freebsd.org/msg311264.html

[ojwb]

The follow-up commit was needed for the GCC6 snapshot package that was in Debian experimental, though it doesn't seem to be needed for the 6.1.1 package now in Debian unstable.

Since the header include is math.h, isfinite is not in the std namespace.

The issue was that at least for some versions of GCC, #include <math.h> actually gave you std::isfinite but not isfinite - while that's a bug in GCC, breaking compilation because of it isn't helpful so I'm hesitant to simply revert this commit without at least knowing if this affected released versions of GCC. I wonder if we're better off including <cmath> instead of <math.h> here - as you say that should actually ensure we have std::isfinite().

[AMDmi3]

I wonder if we're better off including <cmath> instead of here - as you say that should actually ensure we have std::isfinite().

Definitely. I won't call it `better off', it's actually the only correct way of getting access to std::isfinite().

[ojwb]

That's "better off than reverting the second commit"...

The patch in #788 results in #include <math.h> in the "header" section (from the chained fragment) then #include <cmath> conditionally in the expanded code, which doesn't seem ideal. I'm not sure if/how you can conditionally pull in a fragment though. Perhaps it'd have to have a corresponding header fragment with the same conditional, or something.

[AMDmi3]

This may be silly idea as I know nothing on what's going on with this code, but why do you need <math.h> there at all? Maybe change it to <cmath>?

[ojwb]

It needs to work as C code too, and C doesn't have <cmath>.

[wsfulton]

isfinite is a C99 and C11 macro in math.h:

7.12.3.2 The isfinite macro
Synopsis

#include <math.h>
int isfinite(real-floating x);

C++11 states (17.6.1.2 Headers):

In the C++ standard library, however, the declarations (except for names which are defined as macros in C) are within namespace scope (3.3.6) of the namespace std. It is unspecified whether these names are first declared within the global namespace scope and are then injected into namespace std by explicit using-declarations (7.3.3).
5 Names which are defined as macros in C shall be defined as macros in the C++ standard library, even if C grants license for implementation as functions. [ Note: The names defined as macros in C include the following: assert, offsetof, setjmp, va_arg, va_end, and va_start. — end note ]
6 Names that are defined as functions in C shall be defined as functions in the C++ standard library.

and D.5:

2 Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (7.3.3).
3 [ Example: The header assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header <stdlib.h> assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. It may also provide these names within the namespace std. — end example ]

I read this as isfinite should be in the global namespace if math.h is the included header and it should be a macro. Anyone got a few versions of g++ around to check conformance with and without flags -std=c++11? I think we should code for the standard and use __GNUC__ version checking to work around bugs that don't conform.

This does need fixing asap for the next release.

[wsfulton]

There is more explicitly about cmath, from the C++11 standard:

26.8 C library [c.math]
Tables 119 and 120 describe headers and , respectively.
Table 119 — Header synopsis
Classification/comparison Functions:
...
isfinite
...
4 The contents of these headers are the same as the Standard C library headers <math.h> and <stdlib.h> respectively, with the following changes:
...
10 The classification/comparison functions behave the same as the C macros with the corresponding names defined in 7.12.3, Classification macros, and 7.12.14, Comparison macros in the C Standard. Each function is overloaded for the three floating-point types:

bool isfinite(float x);
...
bool isfinite(double x);
...
bool isfinite(long double x);

So with this info, it is becomes murky. The way to interpret this is cmath defines 3 overloaded versions of std::isfinite and these are functions. Then math.h makes it behave as these functions are in the global namespace. Whether or not math.h provides isfinite as a macro or a function is rather hazy. In my g++-5.4 it is a macro even with -std=c++11, but gcc-6.2 has it as a function for all versions of c++. My clang++-3.8 has it as a global function for all versions of c++.

So whether or not it is a function or macro is not clear and implemented differently, but we must conclude that isfinite must be available in the global namespace when using math.h. I suggest we code this up as the expectation for c++11 and above and then add in compiler specific workarounds.

Whether or not it should be in the std namespace is unclear and my experiments show that sometimes it is not.

[ojwb]

This does need fixing asap for the next release.

Indeed. Sorry, I've been failing to find the time to sit down and sort it out properly, and I'd much prefer to address this once such that it works for anything standard conforming plus anything in common use which isn't quite conforming, and that's going to need some care with implementation and especially testing (or else there's a real risk we fix this for some cases but break it for others which currently work).

[ojwb]

OK, I've reviewed all this and attempted a fix (see #849). CI will give it some testing, but please test more widely.

This ought to work whether <math.h> gives us an isfinite macro, isfinite() as a function in the global namespace, or std::isfinite(), without having to go down the route of compiler-specific special casing.

The alternative approach I can see is to change the <math.h> fragment to use <cmath> under C++, but that seems likely to cause fall out from code assuming that maths functions are in the global namespace, and I've also hit issues in the past due to the C++ c-prefix version of a C header only providing functions specified by the C++ standard, whereas the C header provides various extensions and functions specified by other standards. Any such code in SWIG we can at least try to find and update, but it's likely to also affect some user interface files. Including both <math.h> and <cmath> is a variant on this, but I'm not sure if we can rely on how that behaves.