Last Comment Bug 640494 - error: 'signbit' was not declared in this scope when building js shell with g++ -std=c++0x
: error: 'signbit' was not declared in this scope when building js shell with g...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla6
Assigned To: Jason Orendorff [:jorendorff]
:
:
Mentors:
Depends on: 653056 653777 653898 654664
Blocks: 653322 617115 650304
  Show dependency treegraph
 
Reported: 2011-03-10 01:12 PST by Mike Hommey [:glandium]
Modified: 2011-09-07 09:42 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use std::signbit when using js headers in C++ (728 bytes, patch)
2011-03-10 01:12 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Use cmath instead of math.h when using js headers in C++ (1.23 KB, patch)
2011-03-10 03:40 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Use cmath instead of math.h when using js headers in C++ (1.51 KB, patch)
2011-03-10 11:29 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
v1 - checked in (1.18 KB, patch)
2011-03-22 07:34 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Splinter Review
The bitwise approach, v1 (3.52 KB, patch)
2011-03-28 08:25 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review
bitwise, v2 (3.33 KB, patch)
2011-04-25 02:54 PDT, Mike Hommey [:glandium]
jimb: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2011-03-10 01:12:00 PST
Created attachment 518310 [details] [diff] [review]
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] 2011-03-10 03:40:31 PST
Created attachment 518324 [details] [diff] [review]
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] 2011-03-10 11:29:13 PST
Created attachment 518458 [details] [diff] [review]
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) 2011-03-10 11:39:15 PST
clang on linux is fine with the patch. Will try OS X too.
Comment 4 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-03-10 12:32:13 PST
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] 2011-03-10 12:46:10 PST
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 2011-03-10 13:28:42 PST
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] 2011-03-10 13:45:02 PST
(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] 2011-03-10 14:07:50 PST
(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] 2011-03-10 14:09:00 PST
(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] 2011-03-10 14:12:25 PST
Whee, <algorithm>. Cc'ing Jason.

/be
Comment 11 Jason Orendorff [:jorendorff] 2011-03-22 07:34:49 PDT
Created attachment 520913 [details] [diff] [review]
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] 2011-03-22 07:47:00 PDT
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] 2011-03-22 07:51:12 PDT
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] 2011-03-22 08:14:29 PDT
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] 2011-03-22 10:55:17 PDT
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] 2011-03-25 09:07:03 PDT
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] 2011-03-25 09:33:30 PDT
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] 2011-03-25 10:48:30 PDT
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] 2011-03-26 03:26:12 PDT
(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] 2011-03-26 03:28:59 PDT
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] 2011-03-26 03:29:45 PDT
(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] 2011-03-26 03:45:04 PDT
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] 2011-03-28 08:03:20 PDT
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] 2011-03-28 08:25:44 PDT
Created attachment 522370 [details] [diff] [review]
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] 2011-03-28 11:22:38 PDT
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 2011-03-28 17:59:21 PDT
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 2011-03-28 18:00:24 PDT
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] 2011-03-28 23:41:24 PDT
(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 2011-03-29 11:04:11 PDT
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] 2011-03-29 11:23:52 PDT
(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] 2011-04-22 02:47:59 PDT
In the end, which one do we land?
Comment 32 Jim Blandy :jimb 2011-04-22 15:18:15 PDT
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] 2011-04-22 23:50:50 PDT
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] 2011-04-23 04:02:03 PDT
This appears to have caused a perma-orange in Windows opt builds.
Comment 35 Mike Hommey [:glandium] 2011-04-23 04:11:07 PDT
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] 2011-04-23 10:19:33 PDT
Great... I pushed the same patch to try, with additional stuff to debug and... jsreftest didn't fail...
Comment 37 Mike Hommey [:glandium] 2011-04-24 04:42:03 PDT
It looks like the problem comes from the JSDOUBLE_IS_NaN function.
Comment 38 Mike Hommey [:glandium] 2011-04-24 08:12:14 PDT
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] 2011-04-25 02:45:51 PDT
It appears PGO is responsible for the JSDOUBLE_IS_NaN breakage.
Comment 40 Mike Hommey [:glandium] 2011-04-25 02:54:54 PDT
Created attachment 528075 [details] [diff] [review]
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 2011-04-26 11:36:59 PDT
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] 2011-04-26 11:42:57 PDT
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] 2011-04-26 23:43:08 PDT
http://hg.mozilla.org/mozilla-central/rev/83e8315c5f69

Note You need to log in before you can comment on or make changes to this bug.