833254 - Fix gcc version checks for future major versions and enum bug fix

Status: RESOLVED FIXED

(Core :: MFBT, defect, P3)

Description

[Chris Peterson [:cpeterson]]

Attachment: gcc-version-check.patch (ignoring whitespace to make diff more readable)

1. This patch combines MFBT's gcc major and minor version checks into a MOZ_GCC_VERSION macro as suggested by the GCC manual. This macro definition probably belongs in Util.h, but Attributes.h is its primary user and is not supposed to #include other header files.

http://gcc.gnu.org/onlinedocs/gcc-4.6.0/cpp/Common-Predefined-Macros.html

2. mfbt/Attributes.h does not handle a hypothetical "gcc 5.0" version. Future versions of gcc would support MOZ_HAVE_CXX11_ENUM_TYPE and MOZ_HAVE_CXX11_STRONG_ENUMS, but we #define those macros only for __GNUC__ <= 4.

3. And just in time to use MOZ_GCC_VERSION's patchlevel check: gcc's enum operators were fixed in gcc 4.5.1, not 4.5.0, according to gcc's Bugzilla:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38064

Here is a green try build:
https://tbpl.mozilla.org/?tree=Try&rev=b789dfc2ee84

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: gcc-version-check.patch (with whitespace)

Comment 2

[(no longer active)]

Comment on attachment 704782 [details] [diff] [review]
gcc-version-check.patch (ignoring whitespace to make diff more readable)

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

I wouldn't dare review MFBT code!  :-)

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 704782 [details] [diff] [review]
gcc-version-check.patch (ignoring whitespace to make diff more readable)

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

The reason for Attributes.h not depending on anything else actually no longer matters any more.  But we're slowly trying to get rid of Util.h, because it's totally non-obvious from the name what it gives you, and whether it would give you what you want.  So let's add a new Compiler.h header to hold this gcc-version-testing stuff, and other compiler testing stuff as it comes in handy.

Looking at these numbers, I'm never going to remember just how many zeroes go in each component, and/or those numbers are going to be hard to read without well-placed separators.  Let's make the interface MOZ_GCC_AT_LEAST(major, minor, patch) instead.

::: mfbt/Attributes.h
@@ +94,3 @@
>  #        define MOZ_HAVE_CXX11_OVERRIDE
>  #        define MOZ_HAVE_CXX11_FINAL     final
>  #      endif

Doesn't this need to be unindented a level?  Same for the others.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: gcc-version-check-v2.patch

patch v2: I added the MOZ_GCC_VERSION_AT_LEAST macro and moved it to a new Compiler.h header file.

Sorry about the whitespace confusion in the previous patch; I exported the patch using `hg qdiff -w` in an attempt to make it easier to review the important changes.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 707675 [details] [diff] [review]
gcc-version-check-v2.patch

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

Looks good modulo the extra macros Compiler.h exposes -- I'll poke you about those on IRC and we can discussify before you push this.

We should file a followup (Core::General, wouldn't be an mfbt change) to replace manual GCC version tests in the rest of Mozilla.

::: mfbt/Compiler.h
@@ +7,5 @@
> +
> +#ifndef mozilla_Compiler_h_
> +#define mozilla_Compiler_h_
> +
> +#if !defined(__clang__) && defined(__GNUC__)

What use case do you envision for exposing MOZ_MAKE_GCC_VERSION and MOZ_GCC_VERSION?  It seem it'd be better to me to expose only MOZ_GCC_VERSION_AT_LEAST.  If people want more things to test versions more precisely, we should add separate macros for those specific cases, rather than exposing these other things that people might (mis)use directly.

@@ +15,5 @@
> +    * gcc 4.5.1's MOZ_GCC_VERSION would be 40501.
> +    */
> +#  define MOZ_MAKE_GCC_VERSION(major, minor, patchlevel) ((major) * 10000 \
> +                                                          + (minor) * 100 \
> +                                                          + (patchlevel))

More readable as:

#  define MOZ_MAKE_GCC_VERSION(major, minor, patchlevel) \
     ((major) * 10000 + (minor) * 100 + (patchlevel))

@@ +18,5 @@
> +                                                          + (minor) * 100 \
> +                                                          + (patchlevel))
> +#  define MOZ_GCC_VERSION MOZ_MAKE_GCC_VERSION(__GNU__, \
> +                                               __GNUC_MINOR__, \
> +                                               __GNUC_PATCHLEVEL__)

#  define MOZ_GCC_VERSION \
     MOZ_MAKE_GCC_VERSION(...)

@@ +20,5 @@
> +#  define MOZ_GCC_VERSION MOZ_MAKE_GCC_VERSION(__GNU__, \
> +                                               __GNUC_MINOR__, \
> +                                               __GNUC_PATCHLEVEL__)
> +#  define MOZ_GCC_VERSION_AT_LEAST(major, minor, patchlevel) \
> +            (MOZ_GCC_VERSION >= MOZ_MAKE_GCC_VERSION(major, minor, patchlevel))

We indent macro bodies two spaces, so

#  define MOZ_... \
     (MOZ_GCC_VERSION >= ...)

@@ +23,5 @@
> +#  define MOZ_GCC_VERSION_AT_LEAST(major, minor, patchlevel) \
> +            (MOZ_GCC_VERSION >= MOZ_MAKE_GCC_VERSION(major, minor, patchlevel))
> +#endif
> +
> +#endif  /* mozilla_Compiler_h_ */

As a followup, it might be nice to document our compiler requirements at the end of this file, as a quick backstop against configure-checks not happening:

#if !MOZ_GCC_VERSION_AT_LEAST(4, 4, 0)
#  error "mfbt (and Gecko) require at least gcc 4.4 to build."
#endif

Same things might be nice for MSVC, too, after we add version-checking macros for it too.  (Not for clang, tho, since clang deliberately doesn't have reliable version macros, even for Subversion revision number -- only feature-testing macros.)  (And of course the right numbers for this stuff -- I never remember them.)  But a separate bug.

Comment 6

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdbffdf275a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ecdbffdf275a

Comment 8

[Timothy Nikkel (:tnikkel)]

Could this be uplifted to aurora? I want to fix bug 827946 on aurora and the patch uses the MOZ_GCC_VERSION_AT_LEAST macro.

Comment 9

[Jeff Walden [:Waldo]]

Seems an easy enough uplift, I wouldn't object.  Although if it were me, I'd just to write out the longhand comparison and call it a day.  :-)

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 707675 [details] [diff] [review]
gcc-version-check-v2.patch

Hope I'm not stepping on toes by requesting this on someone else's patch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: I'd like to uplift this so I can use the GCC version macro to fix bug 827946
Testing completed (on m-c, etc.): been on m-c for a few weeks
Risk to taking this patch (and alternatives if risky): not very risky. we could just expand the macro out manually on aurora instead like Jeff suggests
String or UUID changes made by this patch: none

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 707675 [details] [diff] [review]
gcc-version-check-v2.patch

Approving since we definitely want the fix for bug 827946

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 707675 [details] [diff] [review]
gcc-version-check-v2.patch

Turns out there's been some churn in these files lately, enough changes are needed to make the patch apply that I'm just going to go with checking the gcc version stuff without the help of the new macro like Jeff suggested.

Sorry for the bother.