Closed
Bug 678558
Opened 12 years ago
Closed 12 years ago
Detect broken vrp and disable it
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla9
People
(Reporter: espindola, Assigned: espindola)
References
Details
Attachments
(2 files, 2 obsolete files)
5.04 KB,
patch
|
Details | Diff | Splinter Review | |
4.64 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #552701 -
Flags: review?(ted.mielczarek)
Comment 1•12 years ago
|
||
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."
Attachment #552701 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 2•12 years ago
|
||
This one uses AC_TRY_RUN, MOZ_OPTIMIZE_FLAGS and has a better error message.
Attachment #552701 -
Attachment is obsolete: true
Attachment #553228 -
Flags: review?(ted.mielczarek)
Comment 3•12 years ago
|
||
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.
Attachment #553228 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 4•12 years ago
|
||
> 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 :-(
Assignee | ||
Comment 5•12 years ago
|
||
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.
Assignee: nobody → respindola
Attachment #553228 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #553960 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•12 years ago
|
Summary: Detect and reject broken gcc → Detect broken vrp and disable it
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Attachment #553960 -
Flags: review?(ted.mielczarek) → review?(khuey)
Assignee | ||
Comment 7•12 years ago
|
||
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.
Attachment #555173 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 8•12 years ago
|
||
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 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.
Attachment #553960 -
Flags: review?(khuey)
Assignee | ||
Comment 10•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
> 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•12 years ago
|
||
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?
Attachment #555173 -
Flags: review?(ted.mielczarek) → review+
Comment 14•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/6da9774903dc
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•