Bug 164207 - -Wpragmas warning spam from angleutils.h
Summary: -Wpragmas warning spam from angleutils.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-30 09:16 PDT by Michael Catanzaro
Modified: 2016-11-01 16:42 PDT (History)
7 users (show)

See Also:


Attachments
Patch (19.06 KB, patch)
2016-10-30 09:27 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-10-30 09:16:20 PDT
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.
Comment 1 Michael Catanzaro 2016-10-30 09:27:54 PDT
Created attachment 293350 [details]
Patch
Comment 2 WebKit Commit Bot 2016-10-30 09:29:04 PDT
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Michael Catanzaro 2016-10-30 09:29:47 PDT
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.
Comment 4 Darin Adler 2016-10-30 15:32:18 PDT
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.
Comment 5 Michael Catanzaro 2016-10-30 16:04:34 PDT
(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. :(
Comment 6 WebKit Commit Bot 2016-10-30 16:30:38 PDT
Comment on attachment 293350 [details]
Patch

Clearing flags on attachment: 293350

Committed r208136: <http://trac.webkit.org/changeset/208136>
Comment 7 WebKit Commit Bot 2016-10-30 16:30:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 David Kilzer (:ddkilzer) 2016-11-01 08:27:09 PDT
(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!
Comment 9 Michael Catanzaro 2016-11-01 16:42:32 PDT
(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.)