Add GCC warning flags from WebKit to the ANGLE Xcode targets

Add GCC warning flags from WebKit to the ANGLE Xcode targets

ANGLE currently passes this set of warning flags, taken from the WebKit builds. I would like to treat these warnings as errors in the ANGLE Xcode targets. If we don't violate these warnings, updating ANGLE in WebKit can be much smoother in the future.

Additionally, some of these warnings could find bugs like: http://code.google.com/p/angleproject/issues/detail?id=349 (which -Wtype-limits, included in -Wextra, would have caught).

[mvujovic, 2012-07-19 17:51:24]

Hi guys,

Does this sound reasonable?

Thanks,
Max

[apatrick1, 2012-07-19 18:18:48]

I'm fine with this in principle. Do these flags not also work on linux? Does
clang understand them all, or at least ignore them without error?

[mvujovic, 2012-07-19 20:46:43]

On 2012/07/19 18:18:48, apatrick1 wrote:
> I'm fine with this in principle. Do these flags not also work on linux? 

I think they should work. I'll get a Linux VM and try it out.

> Does clang understand them all, or at least ignore them without error?

clang understands them all.

[mvujovic, 2012-07-20 16:32:27]

I posted patch set #2. I enabled the warnings for the Linux build in the gyp
file. I did have to remove a couple of the flags, though.

Unlike Mac GCC 4.2, Mac clang and Linux GCC 4.6 generate warnings on the ANGLE
source for the following flags:

-Wsign-compare
-Wmissing-format-attribute
-Wmissing-noreturn

Additionally, Linux GCC 4.6 doesn't understand:

-Wextra-tokens

I've removed these flags for now. It'll be easier if we add each one separately
with its corresponding fix in the ANGLE source (if appropriate).

[mvujovic, 2012-07-24 16:04:59]

Hi Alastair,

Does this look good?

Thanks,
Max

[Alok Priyadarshi, 2012-07-24 16:55:45]

https://codereview.appspot.com/6427049/diff/4001/src/build_angle.gyp
File src/build_angle.gyp (right):

https://codereview.appspot.com/6427049/diff/4001/src/build_angle.gyp#newcode6
src/build_angle.gyp:6: 'variables': {
compiler-specific variables go into common.gypi so that various clients may
choose to use or ignore those.

[mvujovic, 2012-07-24 17:59:55]

https://codereview.appspot.com/6427049/diff/4001/src/build_angle.gyp
File src/build_angle.gyp (right):

https://codereview.appspot.com/6427049/diff/4001/src/build_angle.gyp#newcode6
src/build_angle.gyp:6: 'variables': {
On 2012/07/24 16:55:45, Alok Priyadarshi wrote:
> compiler-specific variables go into common.gypi so that various clients may
> choose to use or ignore those.

In Patch Set 3, I relocated this variable to the topmost variables section of
common.gypi. Thanks, Alok.

[apatrick1, 2012-07-24 18:05:53]

LGTM. Do you need this landed?

[mvujovic, 2012-07-24 18:09:32]

On 2012/07/24 18:05:53, apatrick1 wrote:
> LGTM. Do you need this landed?

Thanks! Nope, I can commit it.

[mvujovic, 2012-07-24 18:15:33]

On 2012/07/24 18:09:32, mvujovic wrote:
> On 2012/07/24 18:05:53, apatrick1 wrote:
> > LGTM. Do you need this landed?
> 
> Thanks! Nope, I can commit it.

Committed in r1230.

[Alok Priyadarshi, 2012-07-24 18:28:11]

https://codereview.appspot.com/6427049/diff/8001/src/build_angle.gyp
File src/build_angle.gyp (right):

https://codereview.appspot.com/6427049/diff/8001/src/build_angle.gyp#newcode45
src/build_angle.gyp:45: ],
This is not right. You cannot reference them here. If you do, then you are
forcing everyone using build_angle.gyp to use these compiler settings. Please
see msvs_settings in common.gypi as an example.

[mvujovic, 2012-07-24 18:54:18]

https://codereview.appspot.com/6427049/diff/8001/src/build_angle.gyp
File src/build_angle.gyp (right):

https://codereview.appspot.com/6427049/diff/8001/src/build_angle.gyp#newcode45
src/build_angle.gyp:45: ],
On 2012/07/24 18:28:11, Alok Priyadarshi wrote:
> This is not right. You cannot reference them here. If you do, then you are
> forcing everyone using build_angle.gyp to use these compiler settings. Please
> see msvs_settings in common.gypi as an example.

Sorry, I think I misunderstood. Is this the right thinking?: We want these
settings as defaults in common.gypi. Then, clients can overrwrite them in
build_angle.gyp.

I would like these flags to only apply to the preprocessor, translator_common,
and translator_glsl targets. gtest and samples shouldn't have them applied.

[Alok Priyadarshi, 2012-07-24 19:09:48]

On 2012/07/24 18:54:18, mvujovic wrote:
> https://codereview.appspot.com/6427049/diff/8001/src/build_angle.gyp
> File src/build_angle.gyp (right):
> 
> https://codereview.appspot.com/6427049/diff/8001/src/build_angle.gyp#newcode45
> src/build_angle.gyp:45: ],
> On 2012/07/24 18:28:11, Alok Priyadarshi wrote:
> > This is not right. You cannot reference them here. If you do, then you are
> > forcing everyone using build_angle.gyp to use these compiler settings.
Please
> > see msvs_settings in common.gypi as an example.
> 
> Sorry, I think I misunderstood. Is this the right thinking?: We want these
> settings as defaults in common.gypi. Then, clients can overrwrite them in
> build_angle.gyp.
> 
> I would like these flags to only apply to the preprocessor, translator_common,
> and translator_glsl targets. gtest and samples shouldn't have them applied.

1. samples do not build on non-windows platform.
2. I have not looked at the meaning of each flag, but is there a problem
compiling gtest/gmock with these flags? If they do, we might want to
differentiate based on whether the code is angle-code or third-party code.

build_angle.gyp should mostly be about listing targets and files that need to be
compiled.

[apatrick1, 2012-07-24 19:28:00]

I can't roll ANGLE into Chrome with this change:


Traceback (most recent call last):
  File "src/build/gyp_chromium", line 173, in <module>
    sys.exit(gyp.main(args))
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/__init__.py",
line 471, in main
    options.circular_check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/__init__.py",
line 111, in Load
    depth, generator_input_info, check, circular_check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 2378, in Load
    depth, check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 430, in LoadTargetBuildFile
    includes, depth, check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 430, in LoadTargetBuildFile
    includes, depth, check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 430, in LoadTargetBuildFile
    includes, depth, check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 430, in LoadTargetBuildFile
    includes, depth, check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 430, in LoadTargetBuildFile
    includes, depth, check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 430, in LoadTargetBuildFile
    includes, depth, check)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 384, in LoadTargetBuildFile
    build_file_data, PHASE_EARLY, variables, build_file_path)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 1053, in ProcessVariablesAndConditionsInDict
    build_file)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 1068, in ProcessVariablesAndConditionsInList
    ProcessVariablesAndConditionsInDict(item, phase, variables, build_file)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 1027, in ProcessVariablesAndConditionsInDict
    ProcessConditionsInDict(the_dict, phase, variables, build_file)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 904, in ProcessConditionsInDict
    variables, build_file)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 1053, in ProcessVariablesAndConditionsInDict
    build_file)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 1072, in ProcessVariablesAndConditionsInList
    expanded = ExpandVariables(item, phase, variables, build_file)
  File
"/mnt/data/b/build/slave/linux_clang/build/src/tools/gyp/pylib/gyp/input.py",
line 739, in ExpandVariables
    ' in ' + build_file
KeyError: 'Undefined variable gcc_or_clang_warnings in
/mnt/data/b/build/slave/linux_clang/build/src/third_party/angle/src/build_angle.gyp
while loading dependencies of
/mnt/data/b/build/slave/linux_clang/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/WebCore.gyp
while loading dependencies of
/mnt/data/b/build/slave/linux_clang/build/src/third_party/WebKit/Source/WebKit/chromium/WebKit.gyp
while loading dependencies of
/mnt/data/b/build/slave/linux_clang/build/src/content/browser/debugger/devtools_resources.gyp
while loading dependencies of
/mnt/data/b/build/slave/linux_clang/build/src/chrome/chrome_resources.gyp while
loading dependencies of
/mnt/data/b/build/slave/linux_clang/build/src/chrome/chrome.gyp while loading
dependencies of /mnt/data/b/build/slave/linux_clang/build/src/build/all.gyp
while trying to load
/mnt/data/b/build/slave/linux_clang/build/src/build/all.gyp'
Error: Command /usr/bin/python src/build/gyp_chromium returned non-zero exit
status 1 in /mnt/data/b/build/slave/linux_clang/build

[Alok Priyadarshi, 2012-07-24 19:30:08]

This is because chrome build system ignores common.gypi in ANGLE.

[dgkoch, 2012-07-24 19:30:14]

On 2012/07/24 19:28:00, apatrick1 wrote:
> I can't roll ANGLE into Chrome with this change:

Revert until fixed?

[Alok Priyadarshi, 2012-07-24 19:30:55]

On 2012/07/24 19:30:14, dgkoch wrote:
> On 2012/07/24 19:28:00, apatrick1 wrote:
> > I can't roll ANGLE into Chrome with this change:
> 
> Revert until fixed?

Yes. It should be reverted.

[apatrick1, 2012-07-24 19:37:47]

I'll revert it.

[mvujovic, 2012-07-24 19:42:57]

On 2012/07/24 19:09:48, Alok Priyadarshi wrote:
> On 2012/07/24 18:54:18, mvujovic wrote:
> > https://codereview.appspot.com/6427049/diff/8001/src/build_angle.gyp
> > File src/build_angle.gyp (right):
> > 
> >
https://codereview.appspot.com/6427049/diff/8001/src/build_angle.gyp#newcode45
> > src/build_angle.gyp:45: ],
> > On 2012/07/24 18:28:11, Alok Priyadarshi wrote:
> > > This is not right. You cannot reference them here. If you do, then you are
> > > forcing everyone using build_angle.gyp to use these compiler settings.
> Please
> > > see msvs_settings in common.gypi as an example.
> > 
> > Sorry, I think I misunderstood. Is this the right thinking?: We want these
> > settings as defaults in common.gypi. Then, clients can overrwrite them in
> > build_angle.gyp.
> > 
> > I would like these flags to only apply to the preprocessor,
translator_common,
> > and translator_glsl targets. gtest and samples shouldn't have them applied.
> 
> 1. samples do not build on non-windows platform.

They build for me on mac. build_samples.gyp looks like it produces the
essl_to_glsl target unconditionally.

> 2. I have not looked at the meaning of each flag, but is there a problem
> compiling gtest/gmock with these flags? If they do, we might want to
> differentiate based on whether the code is angle-code or third-party code.

gtest doesn't compile with  -Wundef. It gives errors like "GTEST_OS_LINUX is not
defined".

Here's the -Wundef description:
-Wundef
Warn if an undefined identifier is evaluated in an `#if' directive. 

preprocessor_tests doesn't compile with -Wextra. It gives an error for
gtest-param-util-generated.h:2906 "Base class 'class
testing::internal::ParamInteratorInterface<...> should be explicitly initialized
in the copy constructor."

samples just complains about an unused variable and an empty loop body.

I could remove those two flags and fix up samples in order to move the settings
into common.gypi.

It does seem like we should differentiate between ANGLE sources and third party
sources, though.

> build_angle.gyp should mostly be about listing targets and files that need to
be
> compiled.

Ok, makes sense.

[mvujovic, 2012-07-24 20:06:25]

On 2012/07/24 19:37:47, apatrick1 wrote:
> I'll revert it.

Thanks. Sorry about that.

[mvujovic, 2012-07-25 19:03:06]

Let's try another approach.

I posted patch set 4.

In build_angle.gyp, we define a new variable "angle_code"=1.
In common.gypi, if angle_code==1 the compiler warnings are added to the
target_defaults.

Thus, the warnings will only apply to the targets in build_angle.gyp that are
non-Windows (preprocessor, translator_common, and translator_glsl).

This is similar to Chromium's "chromium_code" variable.

I tested this against the Chromium Mac build and ANGLE stand-alone on Linux.

What do you guys think?

Thanks,
Max

[apatrick1, 2012-07-25 19:25:18]

Let me test it in a Chromium Windows build...

[Alok Priyadarshi, 2012-07-25 19:27:11]

This is much better. lgtm.

https://codereview.appspot.com/6427049/diff/15001/src/build_angle.gyp
File src/build_angle.gyp (right):

https://codereview.appspot.com/6427049/diff/15001/src/build_angle.gyp#newcode7
src/build_angle.gyp:7: # Third party code should not set angle_code.
Remove this comment from here. Add a more informative comment in common.gypi,
where angle_code variable is introduced.

[apatrick1, 2012-07-25 19:37:33]

Works on Windows. LGTM.

[mvujovic, 2012-07-25 20:43:50]

Thank you guys for the review and testing this on Windows.

One question for Alok below...

https://codereview.appspot.com/6427049/diff/15001/src/build_angle.gyp
File src/build_angle.gyp (right):

https://codereview.appspot.com/6427049/diff/15001/src/build_angle.gyp#newcode7
src/build_angle.gyp:7: # Third party code should not set angle_code.
On 2012/07/25 19:27:12, Alok Priyadarshi wrote:
> Remove this comment from here. Add a more informative comment in common.gypi,
> where angle_code variable is introduced.

Sure, what do you think about this comment?:
    # angle_code is set to 1 for the core ANGLE targets defined in
src/build_angle.gyp.
    # angle_code is set to 0 for test code, sample code, and third party code.
    # When angle_code is 1, we build with additional warning flags on Mac and
Linux.

[Alok Priyadarshi, 2012-07-25 20:46:32]

lgtm

[Alok Priyadarshi, 2012-07-25 20:46:52]

On 2012/07/25 20:46:32, Alok Priyadarshi wrote:
> lgtm

remember to move the comment to common.gypi

[mvujovic, 2012-07-25 21:03:12]

On 2012/07/25 20:46:52, Alok Priyadarshi wrote:
> On 2012/07/25 20:46:32, Alok Priyadarshi wrote:
> > lgtm
> 
> remember to move the comment to common.gypi

Yup. Thanks, Alok.

Committed in r1244.