Last Comment Bug 836078 - Replace all gcc __GNUC__ version checks with MOZ_GCC_VERSION_AT_LEAST() macro
: Replace all gcc __GNUC__ version checks with MOZ_GCC_VERSION_AT_LEAST() macro
Status: RESOLVED FIXED
[mentor=cpeterson][lang=c++]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: mozilla23
Assigned To: Stephen Kraemer
:
Mentors:
Depends on: 833254 1234033
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-29 15:40 PST by Chris Peterson [:cpeterson]
Modified: 2015-12-20 03:09 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch removing checks with __GNUC_MINOR__ (5.65 KB, patch)
2013-02-01 19:56 PST, Stephen Kraemer
no flags Details | Diff | Splinter Review
Updated Patch (6.47 KB, patch)
2013-02-01 21:03 PST, Stephen Kraemer
cpeterson: feedback+
Details | Diff | Splinter Review
Fixed spacing and removed changes in cairo (3.26 KB, patch)
2013-02-04 17:03 PST, Stephen Kraemer
no flags Details | Diff | Splinter Review
Check if compiler is GCC before using MOZ_GCC_VERSION_AT_LEAST (5.04 KB, patch)
2013-02-22 08:24 PST, Stephen Kraemer
dvander: review+
Details | Diff | Splinter Review
Proposed patch (5.75 KB, patch)
2013-04-24 19:40 PDT, Stephen Kraemer
no flags Details | Diff | Splinter Review
Check if compiler is GCC before using MOZ_GCC_VERSION_AT_LEAST (5.25 KB, patch)
2013-04-30 16:54 PDT, Kevin Brosnan [:kbrosnan]
no flags Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2013-01-29 15:40:30 PST
Searching for __GNUC_MINOR__ is a good place to start:

https://mxr.mozilla.org/mozilla-central/ident?i=__GNUC_MINOR__
Comment 1 Stephen Kraemer 2013-01-31 15:56:50 PST
Seems easy enough to me. I can work on this.
Comment 2 Chris Peterson [:cpeterson] 2013-01-31 16:12:02 PST
Thanks, sbkraeme!

Note that the new MOZ_GCC_VERSION_AT_LEAST() macro landed on the mozilla-inbound branch just this afternoon. MOZ_GCC_VERSION_AT_LEAST() won't be merged to mozilla-central branch for about 1 day.

Also, we only want to change Mozilla code. We don't want to change any __GNUC_MINOR__ checks in the following directories because I think they are third-party code:

  dbm/
  dom/plugins/base/
  gfx/harfbuzz/
  gfx/skia/
  ipc/chromium/
  js/src/assember/wtf/
  js/src/ctypes/
  media/libtheora/
  media/libopus/
  media/webrtc/
  modules/freetype2/
  nsprpub/
  security/
  toolkit/components/protobuf/
  toolkit/crashreporter/google-breakpad/
Comment 3 Stephen Kraemer 2013-02-01 19:56:43 PST
Created attachment 709333 [details] [diff] [review]
Patch removing checks with __GNUC_MINOR__
Comment 4 Stephen Kraemer 2013-02-01 21:03:24 PST
Created attachment 709336 [details] [diff] [review]
Updated Patch

Updated patch to include fix in jsutil.h as well.
Comment 5 Chris Peterson [:cpeterson] 2013-02-04 11:05:05 PST
Comment on attachment 709336 [details] [diff] [review]
Updated Patch

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

Thanks, Stephen! These changes look good to me if you fix the minor issue about argument spacing. When you post a patch with the fixed spaces, you should request a review from module owners familiar with the cairo and js modules. Just set the patch attachment's "review" flag to "?" and list the reviewers' email addresses. I suggest asking Robert O'Callahan for the cairo files and Jeff Walden (Waldo) for the js files.

::: gfx/cairo/cairo/src/cairo-compiler-private.h
@@ +84,4 @@
>   * don't need to be hidden and re-exported using the slim hidden
>   * macros.
>   */
> +#if MOZ_GCC_VERSION_AT_LEAST(3, 0, 0) && defined(__ELF__) && !defined(__sun)

Some of your MOZ_GCC_VERSION_AT_LEAST() calls have spaces between the arguments and some don't. We should be consistent. I usually prefer spaces between arguments, but in this case, I think the calls without spaces read better.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-04 11:29:39 PST
...do we modify our imported version of Cairo?  I would guess we shouldn't be modifying gfx/cairo unless it's an active bugfix/feature-fix, which this isn't.  Someone gfxy could answer that question.
Comment 7 Chris Peterson [:cpeterson] 2013-02-04 12:38:53 PST
I didn't realize Cairo was imported code. In that case, I don't think we should alter it cosmetically.
Comment 8 Stephen Kraemer 2013-02-04 17:03:25 PST
Created attachment 709979 [details] [diff] [review]
Fixed spacing and removed changes in cairo
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-06 17:03:18 PST
Comment on attachment 709979 [details] [diff] [review]
Fixed spacing and removed changes in cairo

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

My review queue is pretty swollen right now, and I have one high-priority set of reviews for which I really should at this point be basically dropping almost everything else.  So ideally I shouldn't be on the hook to review the next iteration of this.  Chris, any chance you could suggest someone?

::: js/public/Utility.h
@@ +11,4 @@
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Scoped.h"
> +#include "mozilla/Compiler.h"

Alphabetize this among the other mozilla/* includes, please.

@@ +228,4 @@
>  # define js_bitscan_clz64(val)  __BitScanReverse64(val)
>  # define JS_HAS_BUILTIN_BITSCAN64
>  #endif
> +#elif MOZ_GCC_VERSION_AT_LEAST(3,4,0)

(3, 4, 0) please, for more readable spacing.  This applies throughout.

::: js/src/jsutil.h
@@ +13,4 @@
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/GuardObjects.h"
> +#include "mozilla/Compiler.h"

Same here.

::: xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp
@@ +12,5 @@
>  #if !defined(__arm__) && !(defined(LINUX) || defined(ANDROID))
>  #error "This code is for Linux ARM only. Check that it works on your system, too.\nBeware that this code is highly compiler dependent."
>  #endif
>  
> +#if MOZ_GCC_VERSION_AT_LEAST(4,5,0)                                     \

Don't you still need the defined(__GNUC__) check here?
Comment 10 Chris Peterson [:cpeterson] 2013-02-06 17:26:27 PST
Stephen, after you have addressed Jeff's feedback, I suggest asking David Anderson (dvander) for a review. He is peer reviewer for the JS module.
Comment 11 Stephen Kraemer 2013-02-07 19:00:30 PST
regarding requiring the defined(__GNUC__) part, wouldn't it make more sense for this macro to include that check in itself? Currently, it would end up being undefined in that case. Would it make more sense for this macro to be returning false instead of being undefined?
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-07 19:23:13 PST
I think the common case would be to version-check for gcc just after having checked for gcc.  It's not meaningful to ask if you're at least gcc x.y.z if you're not even gcc!  Or at least, there are no obvious semantics that should be applied.  Perhaps we should build the must-be-gcc requirement into the macro, so it can't be forgotten.  Perhaps make MOZ_GCC_VERSION_AT_LEAST expand to the current expression, divided by defined(__GNUC__)?
Comment 13 Stephen Kraemer 2013-02-07 19:48:29 PST
If the desired result is a compile time error when this macro is used rather than to have it fail, then it will already fail because it is undefined when __GNUC__ is undefined.

However, I'm not too sure as to whether this result is desirable, since the intent of this macro seems to be to make something simpler, rather than to add functionality. It becomes much simpler if the check for defined(__GNUC__) is incorporated within the macro itself. The only real problem I see with this is if someone were to take the compliment of this value, although to me this would seem to be a rare case, and acceptable in this case to have to do a check for defined(__GNUC__).

The macro could also be renamed to make this a bit more obvious, but something like MOZ_IS_GCC_AND_VERSION_AT_LEAST() seems a bit awkward to me.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-07 20:11:47 PST
(In reply to Stephen Kraemer from comment #13)
> If the desired result is a compile time error when this macro is used rather
> than to have it fail, then it will already fail because it is undefined when
> __GNUC__ is undefined.

Untrue.  Per C++98 16.1p4:

  After all replacements due to macro expansion and the defined unary operator
  have been performed, all remaining identifiers and keywords , except for
  true and false, are replaced with the pp-number 0, and then each
  preprocessing token is converted into a token.

So what'll happen for

#  define MOZ_GCC_VERSION_AT_LEAST(major, minor, patchlevel)          \
     ((__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) \
      >= ((major) * 10000 + (minor) * 100 + (patchlevel)))

is you'll expand semantically to (0 >= number derived from arguments), and the check will always be false.

> the intent of this macro seems to be to make something simpler, rather than
> to add functionality. It becomes much simpler if the check for
> defined(__GNUC__) is incorporated within the macro itself.

You're assuming something about all uses that I don't think necessarily holds: that they'll all be fine with the not-at-least case being desirable for non-gcc compilers.  I am uncomfortable making this assumption.  Rather, it is better to make users be explicit in their intent to use GCC-less-than code, if that is indeed what's desired.  Better not to conflate the two semantically-distinct cases, but rather to require users to be explicit that they've considered both gcc and non-gcc compilers.

> The only real problem I see with this is if someone were to take the
> compliment of this value, although to me this would seem to be a rare
> case, and acceptable in this case to have to do a check for
> defined(__GNUC__).

Either way, I expect someone will get this wrong.  Explicit is better than implicit.  It's not obvious that a user wanting certain behavior if gcc is at least some version, will want the opposite behavior if a totally different compiler is used.

> The macro could also be renamed to make this a bit more obvious, but
> something like MOZ_IS_GCC_AND_VERSION_AT_LEAST() seems a bit awkward to me.

Yeah, let's not go there.
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-07 20:14:21 PST
Er, sorry, I misunderstood what you meant about it being undefined when __GNUC__ is undefined.  The rest of the comment still holds, tho -- the macro should be used only when __GNUC__ has already been verified true.
Comment 16 Stephen Kraemer 2013-02-09 09:48:03 PST
Another thing I just noticed with this macro is that it is going to be undefined if clang is being used. Is clang supported as well? If so, all of these version checks are probably going to need to include some sort of check to see if clang is compatible with whichever compiler feature is being used.
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-13 11:41:33 PST
clang defines __GNUC__ because it's more like gcc than like a non-gcc compiler -- they want to opt into better language features.  (Pretty much like how IE has Mozilla/5.0 in their UA, I think.)  But it is a mistake to consider clang as if it were gcc, because it has different bugs and supports different features.  The non-clang support with this macro is deliberate.

But you raise a worthwhile point: that checking for __GNUC__ isn't enough to use this macro.  Which means we probably need some sort of macro to test for gcc, to be used instead of checking for defined(__GNUC__).
Comment 18 Stephen Kraemer 2013-02-13 14:05:17 PST
Yes, my main concern though isn't so much for checking that we're not using clang, but in what would appear to be previously incorrect usages checking for __GNUC__ and __GNUC_MINOR__, that may have passed for clang without being noticed. These would have to be replaced with something to check if features actually are compatible with clang. I believe clang has a __has_builtin() macro to check some of these, but it would have to be evaluated on a case-by-case basis.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-13 14:27:05 PST
Yeah, we already use the __has_builtin/__has_feature/__has_extension stuff for various clang feature-testing things -- mfbt/Attributes.h has a bunch.  I think it's mostly in the places where we're testing gcc version to work around compiler bugs that we ordinarily test gcc version.  I actually doubt we're testing gcc version and expecting it to make sense for clang, in very many places.
Comment 20 Stephen Kraemer 2013-02-22 08:24:36 PST
Created attachment 717157 [details] [diff] [review]
Check if compiler is GCC before using MOZ_GCC_VERSION_AT_LEAST

Sorry I haven't posted an update on this recently, I've been rather busy the past couple weeks.

Anyway, I have revised this patch to check that the compiler is gcc before using this macro. Unfortunately, using:

> #if defined(SOME_MACRO) && SOME_MACRO(X, Y, Z)

appears to not compile (at least not on gcc) when SOME_MACRO is undefined, as it will expand SOME_MACRO to 0, but leave the arguments unexpanded. Because of this, I had to expand the checks into 2 separate if's, which make things look particularly messy, especially when an else case is needed afterwards.

I also added a check for clang in js/public/Utility.h, to see if the builtin function is there.
Comment 21 Mike Hoye [:mhoye] 2013-04-24 12:37:47 PDT
Can you confirm that you're still working on this bug?
Comment 22 Stephen Kraemer 2013-04-24 15:12:28 PDT
I guess I kind of forgot about this. I put a patch up for review, and David seems to have changed the review flag on it there, although I don't actually see any review comments or anything on it anywhere. Are there any comments on it that I'm missing somewhere?
Comment 23 Kevin Brosnan [:kbrosnan] 2013-04-24 16:48:17 PDT
Looks like it needs checkin. Uploading a patch with the following data would help assist with including the code in the tree. 

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 24 Stephen Kraemer 2013-04-24 19:40:11 PDT
Created attachment 741643 [details] [diff] [review]
Proposed patch

Ok, here is the patch. I was able to compile this in when I first wrote it, but it seems as though I am having an error now since having upgraded to Ubuntu 13.04 (when running the build script I get an error when a python script tries to import MAXREPEAT. I feel like it may be related to: https://bugs.launchpad.net/ubuntu/+source/python2.7/+bug/1157687 since the build system seems to generate its own copy of these libraries, which may not have this fix yet) so it might be a good idea for someone to make sure this isn't going to break the build before uploading it.
Comment 25 Kevin Brosnan [:kbrosnan] 2013-04-25 16:01:49 PDT
Pushed it to try https://tbpl.mozilla.org/?tree=Try&rev=e22c44fe6665 if it is green add checkin-needed to the keywords.
Comment 26 Chris Peterson [:cpeterson] 2013-04-25 16:33:28 PDT
Comment on attachment 741643 [details] [diff] [review]
Proposed patch

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

We may want to reconsider whether MOZ_GCC_VERSION_AT_LEAST should be false instead of undefined for clang builds.

The C preprocessor must expand all the macros on a line before evaluating the expression, so clang's preprocessor must not evaluate code like `#elif defined(__GNUC__) && MOZ_GCC_VERSION_AT_LEAST(4, 6, 0)`, even if the #ifdef'd code would be supported by both clang and gcc 4.6+. This leads to duplicate or convoluted definitions for clang, recent gcc, and old gcc. For example, consider the redundancy in the definition of MOZ_HAVE_BUILTIN_BYTESWAP16():

  https://mxr.mozilla.org/mozilla-central/source/mfbt/Endian.h#145

::: js/public/Utility.h
@@ +227,5 @@
>  # define js_bitscan_ctz64(val)  __BitScanForward64(val)
>  # define js_bitscan_clz64(val)  __BitScanReverse64(val)
>  # define JS_HAS_BUILTIN_BITSCAN64
>  #endif
> +#elif defined(__GNUC__) && MOZ_GCC_VERSION_AT_LEAST(3, 4, 0)

This will not compile on clang where __GNUC__ is #defined, but MOZ_GCC_VERSION_AT_LEAST() is purposely NOT #defined for clang.
Comment 27 Stephen Kraemer 2013-04-30 15:58:26 PDT
Shoot, that was totally the wrong patch, sorry. Is there a way I can easily turn the diff I attached earlier (#717157) into a patch? I only have access to my netbook right now, and it would likely take at least a day or two to get everything setup on this.

With regards to making MOZ_GCC_VERSION_AT_LEAST return false for clang, there was discussion earlier in this bug report on that, and the conclusion was that having it undefined would make more sense since you really shouldn't be using that macro when the compiler isn't GCC, and because it would be more logical when not-ing the macro/in the else case.
Comment 28 Kevin Brosnan [:kbrosnan] 2013-04-30 16:54:24 PDT
Created attachment 743951 [details] [diff] [review]
Check if compiler is GCC before using MOZ_GCC_VERSION_AT_LEAST

All you need was to add the first 4 lines from patch 2. I've taken care of that and pushed that patch to try as https://tbpl.mozilla.org/?tree=Try&rev=0c0789ed88ec
Comment 29 Kevin Brosnan [:kbrosnan] 2013-05-01 12:24:47 PDT
r+ from dvander on https://bugzilla.mozilla.org/attachment.cgi?id=717157&action=edit 

Comment 28 is the same patch with patch author/bug details.
Comment 30 Ryan VanderMeulen [:RyanVM] 2013-05-01 12:47:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4412438e83c4
Comment 31 Phil Ringnalda (:philor) 2013-05-01 20:32:02 PDT
https://hg.mozilla.org/mozilla-central/rev/4412438e83c4

Note You need to log in before you can comment on or make changes to this bug.