798800 - ANGLE fails to build on Mac 10.6 and 10.8 with clang rev 161022 and clang rev 163716

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I get:

In file included from ../../../mozilla/gfx/angle/src/compiler/preprocessor/new/Tokenizer.cpp:1:
./Tokenizer.cpp:811:84: error: implicit conversion of NULL constant to 'bool' [-Werror,-Wnull-conversion]
  if ( ! ( yyg->yy_buffer_stack ? yyg->yy_buffer_stack[yyg->yy_buffer_stack_top] : __null) ) {
       ~                                                                           ^~~~~~
                                                                                   false

That line of code is:

		if ( ! YY_CURRENT_BUFFER ) {

which is defined on line 280 of the same file as:


280 #define YY_CURRENT_BUFFER ( yyg->yy_buffer_stack \
281                           ? yyg->yy_buffer_stack[yyg->yy_buffer_stack_top] \
282                           : NULL)

Comment 1

[Boris Zbarsky [:bzbarsky]]

I have no idea why this started (pretty recently)...  The -Werror has been there for a while, so clearly something else changed?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looks like this started with bug 778980?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I tried updating clang to the revision our builders use (163716), but the problem remains.  This is completely blocking work for me.  :(  I have no idea how this is compiling on the builders....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, I'm on 10.8 in case that matters....

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> This is completely blocking work for me.  :(

Ouch. If you back out 778980 locally, does that fix it for you? Do that allow you to unblock your work?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, as long as I remember to qpush/qpop it every time I pull.  Which means I get full "hey, configure changed" rebuilds on each pull....

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

So, if you build without --enable-warnings-as-errors (or with --disable-warnings-as-errors) does it succeed?

FWIW I've had to disable it myself from time to time, as I use a more recent GCC than the builders have.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> So, if you build without --enable-warnings-as-errors (or with --disable-warnings-
> as-errors) does it succeed?

No, it does not.  Looks like this particular -Werror bit is not controlled by --disable-warnings-as-errors...

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

That is a very good basis to back out 778980.

Comment 10

[Chris Peterson [:cpeterson]]

Bug 778980 added gcc -Werror=conversion-null because, AFAIU, these NULL conversions were errors with clang, but only warnings with gcc. If that is not true, then backing out bug 778980 sounds like a good idea.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Meh, even if that is not true -- -Werror is only a good idea insofar as it's not too much of a PITA. A -Werror that can't be turned down by a --disable-warnings-as-errors is IMHO not OK.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Dunno.  I certainly can't get clang to even warn on a simple testcase that tries to reproduce something like the code from comment 0 in a standalone .cpp file.  And yet it definitely warns on the actual angle code, and does so non-fatally before bug 778980 afaict.

Comment 13

[Jeff Walden [:Waldo]]

If I run a .i file from bz's machine through my clang (on a Linux system, r163985), I get the warning as well, for what it's worth.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Relevant .i file

I'm kinda curious.  Does this also warn with clang on our builder setup?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I just checked, and I get the same build failure on my 10.6 machine, with XCode 3.2.6.

So I _really_ don't understand how this is possibly building on our infra.  :(

Comment 16

[Joe Drew (not getting mail)]

Different OS X SDK target?

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, I talked to Chris about this and we're going to back out bug 778980, more or less.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Or more precisely just part 5 of it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Turn off -Werro since we trigger it with ANGLE on recent Macs.

Comment 20

[Chris Peterson [:cpeterson]]

Comment on attachment 670251 [details] [diff] [review]
Turn off -Werro since we trigger it with ANGLE on recent Macs.

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

LGTM. Sorry for the build hassles!

::: configure.in
@@ +1487,5 @@
>      # -Wtype-limits - catches overflow bugs, few false positives
>      # -Wempty-body - catches bugs, e.g. "if (c); foo();", few false positives
> +    # -Werror=conversion-null - would catch conversions between NULL and
> +    #                           non-pointer types, but unfortunately ANGLE
> +    #                           triggers it on Mac.

I think we should just remove any mention of -W[error=]conversion-null. You can leave the comment if you think it will prevent someone from breaking ANGLE in the future, but configure.in is already a very large file without documenting things we're _not_ doing. :)

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed8e070a4ae

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2ed8e070a4ae