640494 - error: 'signbit' was not declared in this scope when building js shell with g++ -std=c++0x

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Use std::signbit when using js headers in C++

The failure looks like this:
In file included from ../../../../js/src/jsobj.h:64:0,
                 from ../../../../js/src/jsstr.h:56,
                 from ../../../../js/src/jsatom.h:52,
                 from ../../../../js/src/jscntxt.h:59,
                 from ../../../../js/src/shell/jsworkers.cpp:49:
../../../../js/src/jsvalue.h: In function 'int JSDOUBLE_IS_NEGZERO(jsdouble)':
../../../../js/src/jsvalue.h:108:32: error: 'signbit' was not declared in this scope
make[2]: *** [jsworkers.o] Error 1

The attached patch works with gcc on Linux. I'm pushing to try to see if that works properly everywhere.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Use cmath instead of math.h when using js headers in C++

This apparently works everywhere, except Android, where, to add to the fun, std::signbit is not defined, except, cf. bug 617115, with NDK r5...

It should be tested with Xcode 4 and/or clang/llvm, too.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Use cmath instead of math.h when using js headers in C++

This one works on all our current platforms, and should work with Android NDK r5 as well.

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

clang on linux is fine with the patch. Will try OS X too.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

The 32 bit build on OS X with clang finished, the 64 bit build is going just fine, so I think the patch is OK.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 518458 [details] [diff] [review]
Use cmath instead of math.h when using js headers in C++

Punting, I'm about to travel and Jim is better for this. If it builds, it's good, to first approx.

/be

Comment 6

[Jim Blandy :jimb]

This could be improved:

+#if (defined(__GNUC__) && !defined(ANDROID)) || (__GNUC__ == 4 && __GNUC_MINOR__ < 4)

I don't think you really want the ||'s second operand to be tested under non-GCC compilers. I think you meant:

#if defined(__GNUC__) && (!defined(ANDROID) || (__GNUC__ == 4 && __GNUC_MINOR__ < 4))

But the best way to address stuff like this is to check for it in the configure script. Playing compiler version games in preprocessor conditionals is not a great strategy.

But, backing up, I'm not able to infer the problem from what you posted. On what platform did you get the error message in comment 0? I'm running g++ 4.4 on Ubuntu, and I can build jsworkers.o fine with -std=c++0x.

And why does jsworkers.cpp not build, but everything else that #includes jscntxt.h does?

Comment 7

[Mike Hommey [:glandium]]

(In reply to comment #6)
> But, backing up, I'm not able to infer the problem from what you posted. On
> what platform did you get the error message in comment 0? I'm running g++ 4.4
> on Ubuntu, and I can build jsworkers.o fine with -std=c++0x.

g++ 4.5 on Debian. And apparently Android NDK r5 has the same kind of problem too, cf. bug 617115.

> And why does jsworkers.cpp not build, but everything else that #includes
> jscntxt.h does?

Probably a case of include ordering.

Comment 8

[Mike Hommey [:glandium]]

(In reply to comment #7)
> > And why does jsworkers.cpp not build, but everything else that #includes
> > jscntxt.h does?
> 
> Probably a case of include ordering.

So the full answer here is because jsworker.cpp includes algorithm.
algorithm includes stl_algo, which, in c++0x mode, includes random, which includes cmath.
cmath, in c++0x mode, #undefs signbit, so even pre-including math.h doesn't work.

Comment 9

[Mike Hommey [:glandium]]

(In reply to comment #8)
> so even pre-including math.h doesn't work.

One of the first things in cmath is to include math.h anyways.

Comment 10

[Brendan Eich [:brendan]]

Whee, <algorithm>. Cc'ing Jason.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: v1 - checked in

No use of STL goes unpunished. What century is this again? :-P

Fortunately, we have our own js::Reverse template now. Mike, does this patch fix the problem?

Comment 12

[Jason Orendorff [:jorendorff]]

JSDOUBLE_IS_NEGZERO uses signbit-the-macro.

<cmath> contains a function template named std::signbit. According to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44611 the template and the macro can't coexist.

So #including any of the standard C++ <cheader>s in c++0x mode could cause the macro to be #undef'd. This puts JSDOUBLE_IS_NEGZERO in a precarious spot. Maybe the real answer is to encapsulate signbit like this:

#ifdef HAVE_STD_SIGNBIT
# define JS_SIGNBIT(f) (::std::signbit(f))
#else
# define JS_SIGNBIT(f) (signbit(f))
#endif

jimb, if you like that better, say so and I'll whip up the patch. I'm licensed to drive autoconf under these controlled conditions. :)

Comment 13

[Mike Hommey [:glandium]]

Are these headers (jsvalue.h and jsnum.h) supposed to be included by third party ? If so, then the latter is maybe better, though HAVE_STD_SIGNBIT might not work well.

Comment 14

[Jason Orendorff [:jorendorff]]

jsvalue.h, which defines JSDOUBLE_IS_NEGZERO, is purportedly for JS internal use only... but some Gecko .cpp files include it anyway. (Third-party applications do so at their own risk; we do not worry about breaking those guys.)

So the tradeoff between v1 and comment 12 is like this:

The v1 approach is to ban mixing <cheaders> and purportedly internal js headers. Everyone would have to avoid it.

The comment 12 approach is to mix <cheaders> and purportedly internal js headers ourselves, by having jsvalue.h #include <cmath>, in some configurations. The folks using or maintaining those configurations would have to cope with the consequences.

It's up to jimb.

Comment 15

[Mike Hommey [:glandium]]

FWIW, applying your jsworker.cpp patch reports the problem elsewhere:
In file included from ../../dist/include/mozilla/jsipc/ObjectWrapperParent.h:46:0,
                 from ../../dist/include/mozilla/jsipc/ObjectWrapperChild.h:47,
                 from ../../dist/include/mozilla/jsipc/ContextWrapperChild.h:45,
                 from ../../../ipc/testshell/TestShellChild.cpp:38:
../../dist/include/jsvalue.h: In function 'int JSDOUBLE_IS_NEGZERO(jsdouble)':
../../dist/include/jsvalue.h:108:32: error: 'signbit' was not declared in this scope

Comment 16

[Jason Orendorff [:jorendorff]]

jimb marked this patch r+, but as comment 15 points out, it's insufficient.

Here is a new thing that scares me about the comment 12 approach. Our `#include <cmath>` passes this same problem onto our users. For example:

  #include <math.h>
  #include "jsvalue.h"

After this, users with Mike's patch, users couldn't call signbit() themselves, even though they included <math.h> and not <cmath>.

I'm starting to think maybe the answer here is to suck it up and write our own signbit function. "Standard" library indeed--we've already got a mess of #ifdef's. Maybe it's time to cut our losses.

Comment 17

[Jason Orendorff [:jorendorff]]

Is it really this simple?

JS_ALWAYS_INLINE JSBool
JSDOUBLE_IS_NEGZERO(jsdouble d)
{
    union {
        jsdouble d;
        uint64 bits;
    } x;
    x.d = d;
    return x.bits == (uint64(1) << 63);
}

Comment 18

[Jason Orendorff [:jorendorff]]

Currently, we aren't even inlining this at every call site. I think we need to
force-inline it, if this is going to be in jsapi.h (see bug 644210).

So here's MSVC x86-32 opt build assembly...

Current code plus JS_ALWAYS_INLINE:

    fld         qword ptr [edi]  
    fld         st(0)  
    fldz  
    fucompp  
    fnstsw      ax  
    test        ah,44h  
    jp          js_ValueToSource+0A4h (0FFE784h)  
    sub         esp,8  
    fstp        qword ptr [esp]  
    call        dword ptr [__imp___fpclass (10B30D8h)]  
    add         esp,8  
    test        al,20h  
    je          js_ValueToSource+0A6h (0FFE786h) 

The new code:

    fld         qword ptr [eax]
    fstp        qword ptr [esp]
    cmp         dword ptr [esp],0
    jne         js_ValueToSource+8Fh (12CDF2Fh)
    cmp         dword ptr [esp+4],80000000h
    jne         js_ValueToSource+8Fh (12CDF2Fh)

I imagine that is what the code will look like on other platforms as well. We
can live with that, right?

Comment 19

[Mike Hommey [:glandium]]

(In reply to comment #17)
> Is it really this simple?
> 
> JS_ALWAYS_INLINE JSBool
> JSDOUBLE_IS_NEGZERO(jsdouble d)
> {
>     union {
>         jsdouble d;
>         uint64 bits;
>     } x;
>     x.d = d;
>     return x.bits == (uint64(1) << 63);
> }

Theorically, it is this simple.

Comment 20

[Mike Hommey [:glandium]]

Even better (as in not relying on bit tricks, and thus more readable):

JS_ALWAYS_INLINE JSBool
JSDOUBLE_IS_NEGZERO(jsdouble d)
{
    union {
        double d;
        uint64_t bits;
    } x, y;
    x.d = d;
    y.d = -0.0;
    return x.bits == y.bits;
}

This compiles to the same assembly with gcc.

Comment 21

[Mike Hommey [:glandium]]

(In reply to comment #20)
>         double d;
>         uint64_t bits;

Make that jsdouble and uint64. I was testing outside js, so I needed standard types.

Comment 22

[Mike Hommey [:glandium]]

Note that:
    union {
        jsdouble d;
        uint64 bits;
    } x, y;
    x.d = d;
    y.d = -0.0;
    return x.bits & y.bits ? 1 : 0;

yields better assembly with gcc than JSDOUBLE_IS_NEGZERO(d) || d < 0 for JSDOUBLE_IS_NEG

Comment 23

[Jason Orendorff [:jorendorff]]

I did notice that.

I feel awkward writing this patch because I don't want to dig through the x86 and x86-64 architecture guides and find out if this code is really fast or not. But I'll finish it up and post it in a couple hours.

Comment 24

[Jason Orendorff [:jorendorff]]

Attachment: The bitwise approach, v1

JS_ALWAYS_INLINE is not added in this patch, since we don't need it for this bug. Bug 644210 can add it if needed.

Comment 25

[Luke Wagner [:luke]]

Comment on attachment 522370 [details] [diff] [review]
The bitwise approach, v1

Looks good.  I assume you've measured no change in SS/V8, but other than that I agree we can hold off finding the super best asm until someone finds it hot in a loop.

At the risk of contradicting what I just said, the one definition that sticks out to me is:

> JSDOUBLE_IS_NEG(jsdouble d)
> {
>+    jsdpun u;
>+    u.d = d;
>+    return (u.s.hi & JSDOUBLE_HI32_SIGNBIT) != 0;
> }

which seems like, for a dim compiler, might not generate as good code (particularly on x64) as just working with 64-bit constants like the rest of your patch.

bool isneg(double d) {
    return (u.s.hi32 & JSDOUBLE_HI32_SIGNBIT) != 0;
}

Linux x86, gcc -O3 -fomit-frame-pointer:

  sub    $0xc,%esp
  fldl   0x10(%esp)       // w
  fstpl  (%esp)           // t
  mov    0x4(%esp),%eax   // f
  add    $0xc,%esp
  shr    $0x1f,%eax
  ret 

OSX 10.6, gcc -O3:

  movd   %xmm0,%rax
  shrq   $0x20,%rax
  shrl   $0x1f,%eax
  ret

bool isneg(double d) {
    return (p.u64 & JSDOUBLE_SIGNBIT) != 0;
}

Linux x86, gcc -O3 -fomit-frame-pointer

  mov    0x8(%esp),%eax
  shr    $0x1f,%eax
  ret    

OSX 10.6, gcc -O3 -fomit-frame-pointer

  movd   %xmm0,%rax
  shrq   $0x3f,%rax
  ret

So 2 out of 2 dim compilers agree ;-)

Comment 26

[Jim Blandy :jimb]

I'm still recovering from the shock that one can't use an advertised C header feature from C++ if one includes any of an ill-defined set of C++ headers.

If we have a non-signbit-using definition in place, then that's fine with me. But it seems that the fundamental problem is that our headers are dual-language, and we can't assume that language-mandated headers will have the same effects in each language. The direct way to respond to that is with stuff like this:

#ifdef __cplusplus
#include <cmath>
#else
#include <math.h>
#endif

...

#ifdef __cplusplus
... ::std::signbit
#else
... signbit
#endif

If this sort of situation (using a C feature in a C-and-C++ header thwarted by odd standards requirements) arises again, I guess that's what we should do.

Comment 27

[Jim Blandy :jimb]

In other words, I'd r+ Mike's first patch if it were dependent only on C vs. C++, not on compilers and versions thereof.

Comment 28

[Mike Hommey [:glandium]]

(In reply to comment #27)
> In other words, I'd r+ Mike's first patch if it were dependent only on C vs.
> C++, not on compilers and versions thereof.

The reason my first patch wasn't dependent only on C vs. C++ is that some C++ compilers/headers combinations don't make it so that including cmath results in signbit being under the std namespace.

IOW, on some compilers/headers combinations, the following doesn't work:
#include <cmath>
using ::std::signbit

Comment 29

[Jim Blandy :jimb]

It seems that many GNU C++ <cfoo> headers #include the corresponding <foo.h> header, and then #undef various macros that the <foo.h> header defines. In every case, this means that adding an #inclusion of <cfoo> can break code which includes <foo.h> and uses its facilities. And because the <cfoo>'s inclusion of <foo.h> defines <foo.h>'s multiple-inclusion-protection macros, one can't fix things up by #including <foo.h> again.

Since things like <algorithm> randomly bring in things like <cstdlib>, which #undefs definitions from <stdlib.h>, I think that, if we want our public headers to be friendly to clients, they should stay away from C++ headers altogether. (This contradicts what I wrote in comment 26.)

While this gives us a policy for being embedder-friendly, it doesn't give us a recipe for using contested definitions. We can't trust signbit to appear in ::std; nor can we assume it'll be present as a macro. So I think rolling our own is probably the best we can do.

I'll admit, I'm not sure how much of a problem this is in practice, because the <cfoo> headers seem to just #undef everything they're going to define in std::, whether or not they're actually macros in <foo.h>. And I haven't bothered to go through and check the definitions.

(In reply to comment #28)
> IOW, on some compilers/headers combinations, the following doesn't work:
> #include <cmath>
> using ::std::signbit

Wow. The whole point of <cmath> is to get you ::std::signbit, isn't it? Is this a Windows, Mac, or Linux compiler, or something second-tier?

Comment 30

[Mike Hommey [:glandium]]

(In reply to comment #29)
> (In reply to comment #28)
> > IOW, on some compilers/headers combinations, the following doesn't work:
> > #include <cmath>
> > using ::std::signbit
> 
> Wow. The whole point of <cmath> is to get you ::std::signbit, isn't it? Is this
> a Windows, Mac, or Linux compiler, or something second-tier?

This is with Android NDK before r5.

Comment 31

[Mike Hommey [:glandium]]

In the end, which one do we land?

Comment 32

[Jim Blandy :jimb]

I think we should take Jason's reimplementation of signbit. Just removing the dependency altogether is probably going to be the least trouble.

I think we should also take Jason's removal of the <algorithm> dependency, since we've already got a reverse in jstl.h.

Comment 33

[Mike Hommey [:glandium]]

Landed on m-c knowing there would be a merge in tm on tuesday.

http://hg.mozilla.org/mozilla-central/rev/0cd3666fd5c9
http://hg.mozilla.org/mozilla-central/rev/d29e1e37e7c2

Comment 34

[Bill Gianopoulos [:WG9s]]

This appears to have caused a perma-orange in Windows opt builds.

Comment 35

[Mike Hommey [:glandium]]

Backed out the signbit part, but kept the algorithm removal.
http://hg.mozilla.org/mozilla-central/rev/296792b874b9

This caused the following errors on Win opt:
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[0] wrong value item 29
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[1] wrong value item 30
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[2] wrong value item 31
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[3] wrong value item 32
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[4] wrong value item 33
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[5] wrong value item 34
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[6] wrong value item 35
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[7] wrong value item 36
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[8] wrong value item 37
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[9] wrong value item 38
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[11] wrong value item 40
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[12] wrong value item 41
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.4.5-3.js | testarr3[13] wrong value item 42

I'm tempted to say this triggers a compiler bug :(

Comment 36

[Mike Hommey [:glandium]]

Great... I pushed the same patch to try, with additional stuff to debug and... jsreftest didn't fail...

Comment 37

[Mike Hommey [:glandium]]

It looks like the problem comes from the JSDOUBLE_IS_NaN function.

Comment 38

[Mike Hommey [:glandium]]

Interestingly, this also fails with
  return (d != d);

as an implementation for JSDOUBLE_IS_NaN, which is supposed to work. It also fails with a variant of the bitwise check (splitting hi and lo 32-bits checks).

Comment 39

[Mike Hommey [:glandium]]

It appears PGO is responsible for the JSDOUBLE_IS_NaN breakage.

Comment 40

[Mike Hommey [:glandium]]

Attachment: bitwise, v2

I'm proposing this to go forward:
- use _isnan() for JSDOUBLE_IS_NaN on WIN32
- use the bitwise approach on other platforms

I also modified the bitwise version of JSDOUBLE_IS_NaN to something that should be slightly more efficient on 64 bits architectures, and equally efficient on 32 bits architectures.

Comment 41

[Jim Blandy :jimb]

It would be nice to put a brief comment in the JSDOUBLE_IS_NaN implementation saying "Windows miscompiles this definition when using PGO".

Comment 42

[Mike Hommey [:glandium]]

Will file a followup bug when landing, which I'll do after double checking that gcc pgo, that we'll enable on thursday, is not screwing up like visual studio's.

Comment 43

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/83e8315c5f69