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]
:
: 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: 2016-10-19 18:13 PDT (History)
8 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Brendan Eich [:brendan] 2011-03-10 14:12:25 PST
Whee, <algorithm>. Cc'ing Jason.

/be
Comment 11 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Mike Hommey [:glandium] 2011-04-22 02:47:59 PDT
In the end, which one do we land?
Comment 32 User image 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 User image 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 User image Bill Gianopoulos [:WG9s] 2011-04-23 04:02:03 PDT
This appears to have caused a perma-orange in Windows opt builds.
Comment 35 User image 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 User image 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 User image Mike Hommey [:glandium] 2011-04-24 04:42:03 PDT
It looks like the problem comes from the JSDOUBLE_IS_NaN function.
Comment 38 User image 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 User image Mike Hommey [:glandium] 2011-04-25 02:45:51 PDT
It appears PGO is responsible for the JSDOUBLE_IS_NaN breakage.
Comment 40 User image 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 User image 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 User image 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 User image 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.