Bug 164207 – -Wpragmas warning spam from angleutils.h

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[Michael Catanzaro]

Seems angleutils.h has GCC pragmas to suppress GCC warnings that do not exist and have never existed:

[5/4010] Building CXX object Source/We...dParty/ANGLE/src/common/mathutil.cpp.o
In file included from ../../Source/ThirdParty/ANGLE/src/common/debug.h:16:0,
                 from ../../Source/ThirdParty/ANGLE/src/common/third_party/numerics/base/logging.h:11,
                 from ../../Source/ThirdParty/ANGLE/src/common/third_party/numerics/base/numerics/safe_math.h:13,
                 from ../../Source/ThirdParty/ANGLE/src/common/mathutil.h:19,
                 from ../../Source/ThirdParty/ANGLE/src/common/mathutil.cpp:9:
../../Source/ThirdParty/ANGLE/src/common/angleutils.h: In function ‘const char* MakeStaticString(const string&)’:
../../Source/ThirdParty/ANGLE/src/common/angleutils.h:120:32: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
 #pragma GCC diagnostic ignored "-Wexit-time-destructors"
                                ^~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/ThirdParty/ANGLE/src/common/mathutil.cpp: At global scope:
../../Source/ThirdParty/ANGLE/src/common/mathutil.cpp:35:32: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
 #pragma GCC diagnostic ignored "-Wglobal-constructors"
                                ^~~~~~~~~~~~~~~~~~~~~~~

It prints about 100 times in a row since it's included in so many places.

[Michael Catanzaro]

Created attachment 293350 [details]
Patch

[WebKit Commit Bot]

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

[Michael Catanzaro]

I bet MSVCC is warning too, and if so the right fix would be to add additional #if COMPILER(CLANG) guards around all of the pragma statements (which makes it significantly messier unfortunately). I didn't do that because that requires figuring out how to regenerate the .diff file, whereas the fix for GCC just required a simple find/replace.

[Darin Adler]

Comment on attachment 293350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293350&action=review

If we are going to the trouble of touching the library to make the warnings go away, maybe we should add in a NeverDestroyed equivalent and fix the issue instead.

> Source/ThirdParty/ANGLE/src/common/mathutil.cpp:39
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wglobal-constructors"
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wglobal-constructors"
>  static const float g_sharedexp_max = ((pow(2.0f, g_sharedexp_mantissabits) - 1) /
>                                         pow(2.0f, g_sharedexp_mantissabits)) *
>                                       pow(2.0f, g_sharedexp_maxexponent - g_sharedexp_bias);
> -#pragma GCC diagnostic pop
> +#pragma clang diagnostic pop

Note that this mixes the pow function, which takes and returns a double, with the float type. I think std::pow would be better: it’s overloaded for float.

Maybe this one can be done with constexpr instead of const.

[Michael Catanzaro]

(In reply to comment #4)
> Comment on attachment 293350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293350&action=review
> 
> If we are going to the trouble of touching the library to make the warnings
> go away, maybe we should add in a NeverDestroyed equivalent and fix the
> issue instead.

That's a good option too, though none of these cases look particularly dangerous... or we could just add compiler flags to silence the warnings in CMakeLists.txt.

> Note that this mixes the pow function, which takes and returns a double,
> with the float type. I think std::pow would be better: it’s overloaded for
> float.
> 
> Maybe this one can be done with constexpr instead of const.

I'm not planning to make further changes to ANGLE myself, I'll just say it's really unfortunate that we have to deal with this bundled code at all, just because Google doesn't know how to maintain a library. :(

[WebKit Commit Bot]

Comment on attachment 293350 [details]
Patch

Clearing flags on attachment: 293350

Committed r208136: <http://trac.webkit.org/changeset/208136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Note that there are important steps to take when updating ANGLE. See
> http://trac.webkit.org/wiki/UpdatingANGLE

Please file a bug on <https://bugs.chromium.org/p/angleproject/issues/list> to track this change.  We don't want to have to maintain this change locally forever.

Thanks!

[Michael Catanzaro]

(I'm not planning to report it upstream myself since I just modified the diff we already have, but didn't we have a bug report with ANGLE about the exit-time destructors already? I wasn't able to find it with a quick search.)