Bug 59249 – Colliding isinf/isnan between C99 and C++11 with GCC >=4.6

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xan Lopez]

Compilation of WebKit with GCC 4.6.0 C++0x mode fails for various reasons, but the only interesting/complicated one is related to the new isnan/isinf functions introduced in C++0x. Trying to compile with -std=c++0x gives the following:

In file included from ../../Source/JavaScriptCore/runtime/JSValue.h:31:0,
                 from ../../Source/JavaScriptCore/runtime/CallData.h:32,
                 from ../../Source/JavaScriptCore/runtime/Executable.h:29,
                 from ../../Source/JavaScriptCore/bytecode/EvalCodeCache.h:32,
                 from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:33,
                 from ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:33,
                 from ../../Source/JavaScriptCore/jsc.cpp:25:
../../Source/JavaScriptCore/wtf/MathExtras.h:243:12: error: ‘isinf’ is already declared in this scope
../../Source/JavaScriptCore/wtf/MathExtras.h:244:12: error: ‘isnan’ is already declared in this scope

This happens because in C++0x mode (but not otherwise) there's a conflicting extern declaration in bits/mathcalls.h, something like:

extern int isinf(double);

so we get a collision in the global namespace. According to the C++ standard it's undefined whether the functions will be present in the global namespace or not, so I don't think this can be considered a bug in GCC.

We can protect those two lines in MathExtras.h with a "am I using C++0x in GCC" #ifdef, like:

diff --git a/Source/JavaScriptCore/wtf/MathExtras.h b/Source/JavaScriptCore/wtf/MathExtras.h
index fac187c..78eb467 100644
--- a/Source/JavaScriptCore/wtf/MathExtras.h
+++ b/Source/JavaScriptCore/wtf/MathExtras.h
@@ -240,8 +240,10 @@ inline int clampToInteger(unsigned value)

 #if !COMPILER(MSVC) && !(COMPILER(RVCT) && PLATFORM(BREWMP)) && !OS(SOLARIS) && !OS(SYMBIAN)
 using std::isfinite;
+#ifndef __GXX_EXPERIMENTAL_CXX0X__
 using std::isinf;
 using std::isnan;
+#endif
 using std::signbit;
 #endif

but then the next problem arises:

../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp: In member function ‘JSC::RegisterID* JSC::BytecodeGenerator::emitLoad(JSC::RegisterID*, double)’:
../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1109:21: error: call of overloaded ‘isnan(double&)’ is ambiguous
../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1109:21: note: candidates are:
/usr/include/bits/mathcalls.h:235:1: note: int isnan(double)
/usr/lib/gcc/x86_64-redhat-linux/4.6.0/../../../../include/c++/4.6.0/cmath:558:3: note: bool std::isnan(long double)
/usr/lib/gcc/x86_64-redhat-linux/4.6.0/../../../../include/c++/4.6.0/cmath:554:3: note: bool std::isnan(double)
/usr/lib/gcc/x86_64-redhat-linux/4.6.0/../../../../include/c++/4.6.0/cmath:550:3: note: bool std::isnan(float)

The issue here is that this file (and others) have a "using namespace std;" and use isinf/isnan without the std:: prefix, so the compiler cannot tell whether the user wants the C99 version in the global namespace or the C++0x version in std. If the line is removed the ambiguity is gone, although now we'll be using the C99 functions if we just write "isinf" (we'd use the C++ ones if we wrote std::isinf). This is not ideal, I suppose, but at least it works.

I think with some changes in WebKit we could get a better solution, namely one that ends up using the C++ methods throughout *and* compiles correctly under GCC 4.6.0 C++0x. We can:

- Remove the "using std::isinf/isnan/etc" from MathExtras.h for GCC C++0x, at least.
- Remove math.h from all of WebKit
- Change all uses of unprefixed isnan/isinf in WebKit to be "std::isnan/isinf". This way the function call is not ambiguous and we get the C++ version, as we want. This would not only work in modern GCC versions, but also in the old ones and the ARM compiler, where the cmath functions are never present in the global namespace.

[Xan Lopez]

CCing Darin, Daniel has told me he has worked on this area before.

[Xan Lopez]

Created attachment 90789 [details]
namespacestdisnan.diff

Before we do bigger changes, this is the minimal patch needed to make things compile under GCC 4.6.0 + C++0x.

[WebKit Review Bot]

Attachment 90789 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/WebCore/html/NumberInputType.cpp:51:  Use 'using namespace std;' instead of 'using std::numeric_limits;'.  [build/using_std] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Created attachment 90792 [details]
namespacestdisnan.diff

OK style bot, have it your way.

[Alexey Proskuryakov]

Comment on attachment 90792 [details]
namespacestdisnan.diff

View in context: https://bugs.webkit.org/attachment.cgi?id=90792&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        Don't use "using namespace std;" in files where isnan or isinf are
> +        used, since it can lead to ambiguity in C++0x compilers. See the

This is not a very maintainable rule, and following it makes the code base less consistent. I hope that there is a better solution.

Is this a gcc bug, or expected behavior?

[Xan Lopez]

(In reply to comment #5)
> (From update of attachment 90792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90792&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        Don't use "using namespace std;" in files where isnan or isinf are
> > +        used, since it can lead to ambiguity in C++0x compilers. See the
> 
> This is not a very maintainable rule, and following it makes the code base less consistent. I hope that there is a better solution.

I can't really think of any, but there might be. My proposed long-term solution of getting rid of math.h and always using std:: in front of the functions also requires not to use "using namespace std;", at least in the files where isnan/isinf are called, so it wouldn't be a huge improvement in that regard, I suppose.

> 
> Is this a gcc bug, or expected behavior?

The spec, AFAICU, leaves undefined whether or not the functions are also present in the global namespace (on top of being in std::), so I don't think it's a bug but just the way it happens to be on GCC. For details see section 17.6.1.2 (4) of http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf

[Evan Martin]

It seems the core goal is that WebKit wants to use a namespace-unqualified isinf/isnan.  Can't we hack the headers to make this just work?

E.g. in C++0x mode, in MathExtras.h, #include the file that might define the C99 isinf.  Then test for it with #ifdef, and if it's *not* present do the "using" line to get the std:: one.

That way you always exit MathExtras.h with at most one isinf in scope.

[Darin Adler]

Comment on attachment 90792 [details]
namespacestdisnan.diff

View in context: https://bugs.webkit.org/attachment.cgi?id=90792&action=review

>>> Source/JavaScriptCore/ChangeLog:9
>>> +        used, since it can lead to ambiguity in C++0x compilers. See the
>> 
>> This is not a very maintainable rule, and following it makes the code base less consistent. I hope that there is a better solution.
>> 
>> Is this a gcc bug, or expected behavior?
> 
> I can't really think of any, but there might be. My proposed long-term solution of getting rid of math.h and always using std:: in front of the functions also requires not to use "using namespace std;", at least in the files where isnan/isinf are called, so it wouldn't be a huge improvement in that regard, I suppose.

The right thing to do is to find a fix in MathExtras.h. The goal is to be able to call isinf and isnan directly; there must be way to do that.

Please do *not* start making these changes to all the files using MathExtras.h.

[Alexis Menard (darktears)]

Created attachment 92342 [details]
Another patch

[Alexis Menard (darktears)]

(In reply to comment #8)
> (From update of attachment 90792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90792&action=review
> 
> >>> Source/JavaScriptCore/ChangeLog:9
> >>> +        used, since it can lead to ambiguity in C++0x compilers. See the
> >> 
> >> This is not a very maintainable rule, and following it makes the code base less consistent. I hope that there is a better solution.
> >> 
> >> Is this a gcc bug, or expected behavior?
> > 
> > I can't really think of any, but there might be. My proposed long-term solution of getting rid of math.h and always using std:: in front of the functions also requires not to use "using namespace std;", at least in the files where isnan/isinf are called, so it wouldn't be a huge improvement in that regard, I suppose.
> 
> The right thing to do is to find a fix in MathExtras.h. The goal is to be able to call isinf and isnan directly; there must be way to do that.
> 
> Please do *not* start making these changes to all the files using MathExtras.h.

Hi Darin,

If you look at my approach it limits the modifications but there is still an issue.

I have to specify std in some places otherwise I have :

../../../webkit/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1109:21: error: call of overloaded 'isnan(double&)' is ambiguous
../../../webkit/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1109:21: note: candidates are:
/usr/include/bits/mathcalls.h:235:12: note: int isnan(double)
/usr/lib/gcc/x86_64-unknown-linux-gnu/4.6.0/../../../../include/c++/4.6.0/cmath:558:3: note: bool std::isnan(long double)
/usr/lib/gcc/x86_64-unknown-linux-gnu/4.6.0/../../../../include/c++/4.6.0/cmath:554:3: note: bool std::isnan(double)
/usr/lib/gcc/x86_64-unknown-linux-gnu/4.6.0/../../../../include/c++/4.6.0/cmath:550:3: note: bool std::isnan(float)

because /usr/include/bits/mathcalls.h:235:12: note: int isnan(double) is in the global namespace and in a if condition it becomes ambiguous.

Btw I didn't bother doing a changelog.

[Evan Martin]

It appears at least one of those files has a "using namespace std" in it.
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:39:using namespace std;

I wonder how hard it would be to remove that.

Unfortunately, there are 392 instances of 'using namespace std' in the WebKit codebase.

[Build Bot]

Attachment 92342 [details] did not build on win:
Build output: http://queues.webkit.org/results/8557732

[Alexis Menard (darktears)]

(In reply to comment #11)
> It appears at least one of those files has a "using namespace std" in it.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:39:using namespace std;
> 
> I wonder how hard it would be to remove that.
> 
> Unfortunately, there are 392 instances of 'using namespace std' in the WebKit codebase.

A solution could be to get rid of using namespace std and fix the code accordingly and when isnan and friends are in the std then it's fine and we just namespace the WebKit implementation of isnan and friends into "std". Globally using namespace std always lead to name conflicts at some points (based on my little experience).

[Darin Adler]

(In reply to comment #13)
> A solution could be to get rid of using namespace std

We don’t want to do this for the whole project just because of the math headers being screwed up on some platform. There should not be a isnan function both in the global namespace and in namespace std on any platform. Platforms that have that have a broken C++ library and we should find a workaround for that brokenness.

> Globally using namespace std always lead to name conflicts at some points (based on my little experience).

What do you mean by “globally using”? Are you saying no one can ever use “using namespace std”? That does not correspond to my experience.

[Alexis Menard (darktears)]

(In reply to comment #14)
> (In reply to comment #13)
> > A solution could be to get rid of using namespace std
> 
> We don’t want to do this for the whole project just because of the math headers being screwed up on some platform. There should not be a isnan function both in the global namespace and in namespace std on any platform. Platforms that have that have a broken C++ library and we should find a workaround for that brokenness.

Well this I agree, it's stupid.

> 
> > Globally using namespace std always lead to name conflicts at some points (based on my little experience).
> 
> What do you mean by “globally using”? Are you saying no one can ever use “using namespace std”? That does not correspond to my experience.

I'm saying that if you do :

using namespace std;
using namespace other;

you may have name collisions. That's why I always used std::X but I'm fine with what you say. I'll try to come up with an acceptable workaround.

[Darin Adler]

(In reply to comment #15)
> I'm saying that if you do :
> 
> using namespace std;
> using namespace other;
> 
> you may have name collisions.

Yes, exactly. And if you do, then you can’t use using namespace for both.

But that’s not what’s happening here.

[Alexis Menard (darktears)]

(In reply to comment #16)
> (In reply to comment #15)
> > I'm saying that if you do :
> > 
> > using namespace std;
> > using namespace other;
> > 
> > you may have name collisions.
> 
> Yes, exactly. And if you do, then you can’t use using namespace for both.
> 
> But that’s not what’s happening here.

So I investigated a bit with a simple example like this :

#include <stdlib.h>
#include <cmath>
#include <stdio.h>

using namespace std;

int main(int argc, char** argv)
{
    double number = 0;
    if (isnan(number))
    {
        printf("Nan\n");
    }
    return 0;
}

and to compile call g++ main.cpp -std=c++0x -std=gnu++0x -o test.

It reproduces the issue. Now if I turn off the c++0x support it compiles fine.

The real issue is that the c++0x standard removes the prohibition on C++ headers declaring C names in the global namespace. Our problem here (even in the example) is that indirectly math.h is included therefore the error comes.

I'm not really sure how the compiler can solve that. The float overloads seems new in gcc 4.6.0 that's why it only fails with 4.6.0.

[Darin Adler]

Yes, clearly a library bug. We need to work around it. The GCC people got this wrong. That result can’t possibly be correct!

[Alexis Menard (darktears)]

(In reply to comment #18)
> Yes, clearly a library bug. We need to work around it. The GCC people got this wrong. That result can’t possibly be correct!

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48891

Just so that they help us...

[Allan Sandfeld Jensen]

See bug #86465 for the same problem with GCC 4.7, but solved.

Not that many files needed to have 'using namespace std' removed, and the few that did always only used std::min, std::max or std::numeric_limits.

My patch in bug #86465, solves the problem for the problematic files, but moving forward I would suggest adding:
using std::min
using std::max
using std::numeric_limits

to MathExtra.h so many files might avoid having to include the whole std namespace.

Also note that the style-checker currently is trying to enforce 'using namespace std' (why??) and thus trying to make the situation worse instead of better.

[Allan Sandfeld Jensen]

I hadn't taken GCC 4.6 into account when making the patch for GCC 4.7. 

I suggest making the check in MathExtra.h into:
#if !COMPILER(MSVC) && !COMPILER(RVCT) && !OS(SOLARIS)
using std::isfinite;
#if !COMPILER_SUPPORTS(CXX_ISINF_ISNAN)
using std::isinf;
using std::isnan;
#endif
using std::signbit;
#endif

And setting WTF_COMPILER_SUPPORTS_CXX_ISINF_ISNAN for compilers that has isinf and isnan in the global namespace.

[Darin Adler]

MathExtras.h is supposed to deal with functions from <math.h>. Since min, max, and numeric_limits are not from that file, I don’t think they belong in here.

[Allan Sandfeld Jensen]

Created attachment 142978 [details]
Patch

[WebKit Review Bot]

Attachment 142978 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Co..." exit_code: 1
Source/WTF/wtf/MathExtras.h:292:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WTF/wtf/MathExtras.h:298:  Use 'using namespace std;' instead of 'using std::wtf_isinf;'.  [build/using_std] [4]
Source/WTF/wtf/MathExtras.h:299:  Use 'using namespace std;' instead of 'using std::wtf_isnan;'.  [build/using_std] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Allan Sandfeld Jensen]

Created attachment 142982 [details]
Patch

[WebKit Review Bot]

Comment on attachment 142982 [details]
Patch

Clearing flags on attachment: 142982

Committed r117779: <http://trac.webkit.org/changeset/117779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke GCC build for some versions, bug 88721.