678558 - Detect broken vrp and disable it

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: detect and reject broken gcc

The attached patch adds a check for the gcc bug mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=676607.

While that bug is not fixed in any release of gcc, it is not visible with default flags in 4.6.0 and newer because -fstrict-enums is not the default in newer releases. Unfortunately there is no -fno-strict-enums in gcc 4.5.

Comment 1

[(not currently active) Ted Mielczarek]

Comment on attachment 552701 [details] [diff] [review]
detect and reject broken gcc

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

::: configure.in
@@ +3197,4 @@
>  AC_SUBST(WRAP_SYSTEM_INCLUDES)
>  AC_SUBST(VISIBILITY_FLAGS)
>  
> +cat > conftest.cc <<EOF

Can you not use AC_TRY_COMPILE here?

Also, can you put an AC_MSG_CHECKING([For GCC bug XXX]) here?

@@ +3233,5 @@
> +}
> +int main(void) {
> +  jsop_setelem(0, 47);
> +}
> +EOF

I wish this testcase wasn't so long. :-/

@@ +3238,5 @@
> +
> +if ! ${CXX-c++} ${CFLAGS} -O2 -o conftest conftest.cc ; then
> +  AC_MSG_ERROR([broken gcc. See
> +                https://bugzilla.mozilla.org/show_bug.cgi?id=676607 or upgrade
> +                to at least gcc 4.6.0.])

Can we make this error message a little clearer? Like "Your GCC suffers from bug XXX with these compiler flags. You should update your GCC or use a different set of compiler flags. See http://... for more information."

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: updated patch

This one uses AC_TRY_RUN, MOZ_OPTIMIZE_FLAGS and has a better error message.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 553228 [details] [diff] [review]
updated patch

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

::: configure.in
@@ +3229,5 @@
> +AC_LANG_SAVE
> +AC_LANG_CPLUSPLUS
> +_SAVE_CXXFLAGS=$CXXFLAGS
> +CXXFLAGS="$CXXFLAGS $MOZ_OPTIMIZE_FLAGS"
> +AC_TRY_RUN([

Surely you want AC_TRY_COMPILE or AC_TRY_LINK and not AC_TRY_RUN here? Otherwise we'd never run this test on a cross-compile.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Surely you want AC_TRY_COMPILE or AC_TRY_LINK and not AC_TRY_RUN here?
> Otherwise we'd never run this test on a cross-compile.

No, gcc miscopiles the code, so gcc (and ld) exit with status 0. The only way I can think for testing this on a cross compile environment is to grep the output of -fdump-tree-all :-(

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Disable vrp instead of aborting

Checking for failures with MOZ_OPTIMIZE_FLAGS, MOZ_PGO_OPTIMIZE_FLAGS and MODULE_OPTIMIZE_FLAGS matches what was discussed on IRC, but I am not so sure that is the best thing to do anymore.

I think we can just test with a set of flags know to trigger the bug and if it is found, disable vrp. The logic is
* If the set of flags the user is using doesn't enable vrp, disabling it is a nop
* It is possible that vrp is enabled, the bug is present but some other user specified flag hides the bug in the reduced case but not in some other part of the code.

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I did a a try run:

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=cc36d3a7e687

It is all green and the gcc pr is correctly detected:

http://tbpl.allizom.org/php/getParsedLog.php?id=6009937

and -fno-tree-vrp is added to the build.

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Detect the bug with -O2 istead of MOZ_OPTIMIZE_FLAGS

This is a variant of the patch that I like a bit better. We just use -O2 since that is known to trigger the bug.

The test is to check if *gcc* has the bug. If it does, we add -fno-tree-vrp. If with the flags the user specified vrp would not be used, this is a nop. If it would be, we disable it as desired.

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Try runs:

With current gcc

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=8bd493e9dbce

With the test gcc

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=be97c9e34ec8

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 553960 [details] [diff] [review]
Disable vrp instead of aborting

I'm assuming the new patch supersedes this ... if that's incorrect, flag me for review again.

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

The new patch is an alternative. We should use one or the other. I like the new one better, but I am OK with both.

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

The is something broken with our "make check". I did a new try run and now it is green:

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=fb24978b81e4

Not that we use -fno-tree-vrp in the logs.

The one with the new compiler is still running:

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=184da11eedad

Comment 12

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> The one with the new compiler is still running:
> 
> http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=184da11eedad

It is done and it correctly detects that the bug is fixed in that compiler:

checking for gcc PR49911... no

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 555173 [details] [diff] [review]
Detect the bug with -O2 istead of MOZ_OPTIMIZE_FLAGS

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

Overall this is fine, just a couple of questions.

::: build/autoconf/gcc-pr49911.m4
@@ +1,1 @@
> +dnl Check if the compiler is gcc and has PR49911. If so

You don't actually check that we're using GCC anywhere. Do you want to?

@@ +10,5 @@
> +AC_LANG_CPLUSPLUS
> +
> +_SAVE_CXXFLAGS=$CXXFLAGS
> +CXXFLAGS="-O2"
> +AC_TRY_RUN([

AC_TRY_RUN is the only way to detect this? :-/ That's unfortunate. Does AC_TRY_RUN skip the test automatically on cross-compiles?

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/6da9774903dc