1708330 - Cleanup spidermonkey config headers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Ted Campbell [:tcampbell]]

The RequiredDefines.h file only includes some old __STDC_*_MACRO defines that haven't been needed from C++11 onwards. Also, jstypes.h always includes js-config.h but a number of places include both back-to-back.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: Bug 1708330 - Remove js/RequiredDefines.h header. r?sfink!

The various _STDC*_MACRO macros it defines have not been required from
C++11 onwards so remove this complexity.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1708330 - Include jstypes.h instead of js-config.h directly. r?sfink!

Depends on D113740

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1708330 - Add missing defines to js-config.h r?sfink!

Particularly the JS_64BIT define should be there since it is a frequent
footgun for embedding. The js-config.h is the public set of autoconf
variables, while js-confdefs.h is the internal-to-js set, and
mozilla-config.h is the gecko set.. sigh.

Comment 4

[André Bargull [:anba]]

Oh nice, RequiredDefines.h can finally be removed? Last time I tried this in bug 1454348, I got errors on the CI servers because the used libc version was too old. If this is now working, we can close bug 1454348 as a dup.

Comment 5

[André Bargull [:anba]]

Hmm, https://sourceware.org/bugzilla/show_bug.cgi?id=15366 says this is supported starting with glibc 2.18, but we're currently supporting glibc 2.17 (and older) (bug 1634204). So could this lead to issues on CentOS 7. (CentOS 7 seems to be the only relevant configuration with old glibc when looking at https://repology.org/project/glibc/versions.)

Comment 6

[Ted Campbell [:tcampbell]]

Hmm.. that CentOS 7 thing is interesting. What a mess..

Comment 7

[Ted Campbell [:tcampbell]]

I see that a CentOS / RHEL errata was released in 2017 to address this. https://bugzilla.redhat.com/show_bug.cgi?id=1318877

Building Firefox on that platform would use mozilla-config.h which has the workaround so firefox will continue to build fine. This change would only break standalone shell builds on unpatched CentOS 7 versions would doesn't seem like a real use-case to worry about. Things like Gnome shell on such an unpatched system would be using much older spidermonkey. CI is green so I think this is still a reasonable change.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e522293a39d
Remove js/RequiredDefines.h header. r=sfink
https://hg.mozilla.org/integration/autoland/rev/e13ff4606a96
Include jstypes.h instead of js-config.h directly. r=sfink
https://hg.mozilla.org/integration/autoland/rev/c33f03add54d
Add missing defines to js-config.h r=sfink

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6e522293a39d
https://hg.mozilla.org/mozilla-central/rev/e13ff4606a96
https://hg.mozilla.org/mozilla-central/rev/c33f03add54d

Comment 11

[André Bargull [:anba]]

Looks like this change broke compilation on CentOS 6, because the CentOS errata was only applied for CentOS 7.

NI for :arai and :nbp because this seems relevant to their current discussion on Matrix. (So I don't have to search for my Matrix login data. #lazy :-)

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to André Bargull [:anba] from comment #11)

NI for :arai and :nbp because this seems relevant to their current discussion on Matrix.

The related discussion is available at:
https://chat.mozilla.org/#/room/#spidermonkey:mozilla.org/$eO9FZXRY3YVN63VadCI9T08wVzLWA1KJAJJXKnxSMVw