Closed Bug 836078 Opened 7 years ago Closed 6 years ago
Replace all gcc __GNUC
__ version checks with MOZ _GCC _VERSION _AT _LEAST() macro
Searching for __GNUC_MINOR__ is a good place to start: https://mxr.mozilla.org/mozilla-central/ident?i=__GNUC_MINOR__
Component: MFBT → General
OS: Mac OS X → All
Hardware: x86 → All
Seems easy enough to me. I can work on this.
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/
Updated patch to include fix in jsutil.h as well.
Attachment #709333 - Attachment is obsolete: true
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.
Attachment #709336 - Flags: feedback+
...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.
I didn't realize Cairo was imported code. In that case, I don't think we should alter it cosmetically.
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?
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.
Assignee: nobody → sbkraeme
Status: NEW → ASSIGNED
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?
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__)?
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.
(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.
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.
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.
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__).
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.
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.
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.
Attachment #717157 - Flags: review?(dvander) → review+
Can you confirm that you're still working on this bug?
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?
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
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.
Pushed it to try https://tbpl.mozilla.org/?tree=Try&rev=e22c44fe6665 if it is green add checkin-needed to the keywords.
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.
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.
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
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.