525677 - e10s builds broken in nsFrameLoader, conflict of <cmath> and <math.h> with jsnum.h

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Benjamin Smedberg]

The e10s builds are broken in scratchbox because `signbit` isn't defined. `signbit` isn't defined because <cmath> is included, which does #undef signbit after declaring it in the std:: namespace.

I'm not sure why this affects scratchbox and not the standard Linux builds: the cmath undef/redeclare bits are wrapped in:

#if _GLIBCXX_USE_C99_MATH
#if !_GLIBCXX_USE_C99_FP_MACROS_DYNAMIC

(Regression from bug 505847, which is .asyncDrawXULElement and we're not going to back that out!)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

FWIW I can reproduce this with a stock Ubuntu 8.04 build (gcc 4.2.4).

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: diff of 4.2.4 and 4.3.3 headers

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: build fix

"Head in the sand" kind of patch.  I don't care why it works, it just does.

Comment 4

[Benjamin Smedberg]

Ugh. We have to have the chromium headers on top (before we include NSPR) in order to fix the Windows build bustage with mismatched #defines for uint16

Maybe we can expcitly include <math.h> before that, though?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The last patch I posted doesn't quite fix all the bustage, working on v2.  I'm just posting them for reference, not r?, as this is blocking another bug I'm working on.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: hacks v2

Sorry to hack and run, hope this is useful.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

 environment:
  CC=/tools/gcc/bin/gcc
  CVS_RSH=ssh
  CXX=/tools/gcc/bin/g++

checking for c++... c++
checking whether the C++ compiler (c++  -Wl,-rpath-link,/usr/lib:/lib) works... yes
checking whether the C++ compiler (c++  -Wl,-rpath-link,/usr/lib:/lib) is a cross-compiler... no
checking whether we are using GNU C++... yes

c++ -o nsFrameLoader.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"Linux2.6.18-53.1.19\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD  -DOS_LINUX=1 -DOS_POSIX=1  -D_IMPL_NS_LAYOUT -I/home/cltbld/build/maemo-electrolysis/electrolysis/ipc/chromium/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/home/cltbld/build/maemo-electrolysis/electrolysis/objdir/xulrunner/dist/include/nspr -I/home/cltbld/build/maemo-electrolysis/electrolysis/objdir/xulrunner/dist/include/nss      -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../events/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../xml/content/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../../layout/xul/base/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../xul/content/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../html/content/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../base/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../xbl/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../../layout/generic -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../../layout/style -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../../dom/base -I/home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/../../xml/document/src -I/home/cltbld/build/maemo-electrolysis/electrolysis/xpcom/io -I/home/cltbld/build/maemo-electrolysis/electrolysis/dom/ipc   -fPIC -frtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -fno-strict-aliasing -pthread -pipe -DNDEBUG -DTRIMMED -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0   -Os -freorder-blocks -fno-reorder-functions -finline-limit=50   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsFrameLoader.pp /home/cltbld/build/maemo-electrolysis/electrolysis/content/base/src/nsFrameLoader.cpp

I'm willing to bet folding money that these machines aren't building with gcc 4.3.  The errors here are exactly what I'm seeing on Ubuntu 8.04 with gcc 4.2.  And because of the __cxxabiv1::__class_type_info visibility bug, building against 4.2 is futile anyway.

Comment 8

[Benjamin Smedberg]

This is scratchbox, we don't have a lot of control over that toolchain right now, so we have to live with whatever we're given.

Comment 9

[Frederic Plourde (:fred23)]

Attachment: Build failure under Scratchbox with cs2007q3-glibc2.5-arm7

(In reply to comment #6)
> Sorry to hack and run, hope this is useful.

while this may fix things up under ubuntu, it doesn't under scratchbox. See, I'd come up with something very close to what you just posted for nsFrameLoader.cpp, but it's really nsCanvasRenderingContext2D.cpp that's giving me a hard time (see the attachment).

I have a fix that seems to make all platforms happy, but it's very ugly (I'll post in next comment).

Comment 10

[Frederic Plourde (:fred23)]

Attachment: very ugly hack for reference pursposes only (not be to reviewed)

Initial comment from Benjamin states the problem very clearly : <cmath> gets includes by tr1/functional (by chromium) and kills our definition of signbit needed by jsnum.h.

So I played around in jsnum.h, trying to make it use the right version of signbit... either std::signbit (when cmath was kicking in) or the former signbit when not.

Initially, I tried to condition that with "#if _GLIBCXX_USE_C99_MATH", but for some reason, it didn't work. the very stupid hack that seems to make all platforms happy for now is using "#ifndef signbit" (see attachment).

I'll sleep on that for now, and we'll see tomorrow.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v2, for realz

Gak!  Oops, I posted the same patch twice.  Not sure it's still relevant, but attached is the version I meant to post.

Comment 12

[Frederic Plourde (:fred23)]

(In reply to comment #11)

> Not sure it's still relevant, but attached is the version I meant to post.

You bet it is !  Good catch cjones, your patch makes Scratchbox happy as well !
This hack looks clean to me, I guess you could just push that if Benjamin doesn't see other problems.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/961bfa23f570.  Waiting for a green cycle before closing.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Umm ... the maemo build went green from eliminating RTTI ... interesting.