932991 - build broken on OpenBSD/sparc64 after header reordering in bug 898274

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Landry Breuil (:gaston)]

I don't get why it surfaces now, but it seems https://hg.mozilla.org/mozilla-central/diff/200fae26b271/js/src/jscpucfg.h broke OpenBSD, see http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/595

eg++ -o testIntTypesABI.o -c  -fvisibility=hidden -DEXPORT_JS_API -DIMPL_MFBT -DMOZ_GLUE_IN_PROGRAM -DNO_NSPR_10_SUPPORT -I/home/buildslave/mozilla-central-sparc64/build/js/src -I.. -I/home/buildslave/mozilla-central-sparc64/build/js/src/jsapi-tests -I. -I../../../dist/include  -I/data/obj/buildslave/m-c/dist/include/nspr       -fPIC   -DMOZILLA_CLIENT -include ../js-confdefs.h -MD -MP -MF .deps/testIntTypesABI.o.pp  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -O -fomit-frame-pointer  /home/buildslave/mozilla-central-sparc64/build/js/src/jsapi-tests/testIntTypesABI.cpp
testJSEvaluateScript.o
In file included from /usr/include/machine/endian.h:7:0,
                 from /home/buildslave/mozilla-central-sparc64/build/js/src/jscpucfg.h:56,
                 from /home/buildslave/mozilla-central-sparc64/build/js/src/jsapi-tests/testIntTypesABI.cpp:12:
/usr/include/sys/endian.h:162:1: error: '__uint64_t' does not name a type
 __uint64_t htobe64(__uint64_t);
 ^
/usr/include/sys/endian.h:163:1: error: '__uint32_t' does not name a type
 __uint32_t htobe32(__uint32_t);
 ^
/usr/include/sys/endian.h:164:1: error: '__uint16_t' does not name a type


I dont know why it surfaces now since a build from two days ago was successful (see http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/593) .. maybe because i was disabling the helper thread to workaround issue #912168 ?

Will try reverting the offending rev to see if it fixes the build at least.

Comment 1

[Nicholas Nethercote [inactive]]

jscpucfg.h gets included in many places.  It's odd that this compile error would only surface now.  Could there have been some kind of header update on the machine with the breakage?

Reverting the order may break the include ordering check done by config/check_spidermonkey_style.py.  (Run |make check| in js/src/ to see.)  If so, you'll need to add jscpucfg.h to the |included_inclnames_to_ignore| list.  And note that you'll need to modify both config/check_spidermonkey_style.py and js/src/config/check_spidermonkey_style.py.

Comment 2

[Landry Breuil (:gaston)]

To be honest, i dont really trust an automated process moving includes around on a NPOTB platform, since it has probably no idea of the needed ordering on the said platform. machine/endian.h is only seen on BSDs (since bug #714312).

And the corresponding buildslave hasnt been updated recently, so no env change between a successful and an ok build.

Comment 3

[Landry Breuil (:gaston)]

Reverting the header reordering fixes the build for me.

 /* BSDs */
 #elif defined(JS_HAVE_MACHINE_ENDIAN_H)
+# include <sys/types.h>
 # include <machine/endian.h>
-# include <sys/types.h>

Comment 4

[Landry Breuil (:gaston)]

If i do it this way (ie adding jscpucfg.h to included_inclnames_to_ignore) it doesnt seem to take the change into account:

--- check_spider_monkey_style.py expected output
+++ check_spider_monkey_style.py actual output
@@ -1,3 +1,6 @@
+js/src/jscpucfg.h:56:57: error:
+    <sys/types.h> should be included after <machine/endian.h>
+

To me it seems the checking should be relaxed in check_includes_order, not check_includes_statement. And i'm not sure adding <sys/types.h> to oddly_ordered_inclnames is the best solution...

Comment 5

[Landry Breuil (:gaston)]

Attachment: add machine/endian.h to oddly_ordered_inclnames + partly rever 898274.

That version works, gmake check-style is happy with it.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 826705 [details] [diff] [review]
add machine/endian.h to oddly_ordered_inclnames + partly rever 898274.

Review of attachment 826705 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  I can't remember if you have commit access;  please mark the checkin-needed flag if not.

::: config/check_spidermonkey_style.py
@@ +94,5 @@
>  oddly_ordered_inclnames = set([
>      'ctypes/typedefs.h',        # Included multiple times in the body of ctypes/CTypes.h
>      'jsautokw.h',               # Included in the body of frontend/TokenStream.h
>      'jswin.h',                  # Must be #included before <psapi.h>
> +    'machine/endian.h',         # breaks NPOTB platform if reorders includes (#932991)

I'd change the comment to this:

  # Must be included after <sys/types.h> on OpenBSD

::: js/src/config/check_spidermonkey_style.py
@@ +94,5 @@
>  oddly_ordered_inclnames = set([
>      'ctypes/typedefs.h',        # Included multiple times in the body of ctypes/CTypes.h
>      'jsautokw.h',               # Included in the body of frontend/TokenStream.h
>      'jswin.h',                  # Must be #included before <psapi.h>
> +    'machine/endian.h',         # breaks NPOTB platform if reorders includes (#932991)

Ditto.

Comment 7

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9765059205 (with comment fixed)

Comment 8

[Landry Breuil (:gaston)]

Seems mergetool didnt mark this as FIXED when it migrated to central..

https://hg.mozilla.org/mozilla-central/rev/7b9765059205