GCC Bugzilla – Bug 40172
[4.5 Regression] Revision 147596 breaks bootstrap
Last modified: 2009-05-21 11:05:24 UTC
Revision 147596: http://gcc.gnu.org/ml/gcc-cvs/2009-05/msg00572.html breaks bootstrap on ia64. A testcase: [hjl@gnu-12 prev-gcc]$ cat /tmp/x.i struct rtx_def; typedef struct rtx_def *rtx; extern int foo; extern int bar; extern int test (void); extern int xxx; int test (void) { if (((rtx) 0 != (rtx) 0) && xxx ? foo : bar) return 1; else return 0; } [hjl@gnu-12 prev-gcc]$ ./xgcc -B./ -Wall -W -O2 -Werror -S /tmp/x.i cc1: warnings being treated as errors /tmp/x.i: In function ‘test’: /tmp/x.i:13: error: logical ‘and’ of mutually exclusive tests is always false [hjl@gnu-12 prev-gcc]$
*** Bug 40169 has been marked as a duplicate of this bug. ***
This has also been reported at http://gcc.gnu.org/ml/gcc-patches/2009-05/msg00974.html
Here is a testcase for more problems: [hjl@gnu-12 prev-gcc]$ cat /tmp/x.i struct rtx_def; typedef struct rtx_def *rtx; extern int foo; extern int bar; extern int xxx; int test (void) { if (((rtx) 0 != (rtx) 0) && xxx ? foo : bar) return 1; else if ((foo & 0) && xxx) return 2; else if (foo & 0) return 3; else if (0 && xxx) return 4; else if (0) return 5; else return 0; } [hjl@gnu-12 prev-gcc]$ ./xgcc -B./ -Wall -W -O2 -Werror -S /tmp/x.i cc1: warnings being treated as errors /tmp/x.i: In function ‘test’: /tmp/x.i:11: error: logical ‘and’ of mutually exclusive tests is always false /tmp/x.i:13: error: logical ‘and’ of mutually exclusive tests is always false [hjl@gnu-12 prev-gcc]$
Also breaks bootstrap on hppa.
This patch seems to fix the problem and still warn for the interesting cases. Could you all test it in your targets? I can only test x86-64-unknown-linux-gnu. Index: gcc/c-common.c =================================================================== --- gcc/c-common.c (revision 147636) +++ gcc/c-common.c (working copy) @@ -1782,14 +1782,12 @@ warn_logical_operator (location_t locati again at the end. */ if (or_op) in0_p = !in0_p, in1_p = !in1_p; /* If both expressions are the same, if we can merge the ranges, and we - can build the range test, return it or it inverted. If one of the - ranges is always true or always false, consider it to be the same - expression as the other. */ - if ((lhs == 0 || rhs == 0 || operand_equal_p (lhs, rhs, 0)) + can build the range test, return it or it inverted. */ + if (lhs && rhs && operand_equal_p (lhs, rhs, 0) && merge_ranges (&in_p, &low, &high, in0_p, low0, high0, in1_p, low1, high1) && 0 != (tem = build_range_check (type, lhs != 0 ? lhs : rhs != 0 ? rhs : integer_zero_node,
(In reply to comment #5) > This patch seems to fix the problem and still warn for the interesting cases. > Could you all test it in your targets? I can only test > x86-64-unknown-linux-gnu. > The testcase in comment #3 isn't target specific. You should include it in your patch.
Subject: Bug 40172 Author: hjl Date: Sun May 17 18:36:44 2009 New Revision: 147639 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=147639 Log: gcc/ 2009-05-17 Manuel López-Ibáñez <manu@gcc.gnu.org> PR c/40172 * c-common.c (warn_logical_operator): Don't warn if one of expression isn't always true or false. gcc/testscase/ 2009-05-17 H.J. Lu <hongjiu.lu@intel.com> PR c/40172 * gcc.dg/pr40172.c: New. Added: trunk/gcc/testsuite/gcc.dg/pr40172.c Modified: trunk/gcc/ChangeLog trunk/gcc/c-common.c trunk/gcc/testsuite/ChangeLog
Fixed.
Still does not fix the PPC-darwin bootstrap: cc1: warnings being treated as errors /Users/regress/tbox/svn-gcc/gcc/toplev.c: In function 'process_options': /Users/regress/tbox/svn-gcc/gcc/toplev.c:2043: error: logical 'and' of mutually exclusive tests is always false if (!FRAME_GROWS_DOWNWARD && flag_stack_protect)
(In reply to comment #9) > Still does not fix the PPC-darwin bootstrap: > cc1: warnings being treated as errors > /Users/regress/tbox/svn-gcc/gcc/toplev.c: In function 'process_options': > /Users/regress/tbox/svn-gcc/gcc/toplev.c:2043: error: logical 'and' of mutually > exclusive tests is always false > > if (!FRAME_GROWS_DOWNWARD && flag_stack_protect) > This warning may be correct: rs6000.h:#define FRAME_GROWS_DOWNWARD (flag_stack_protect != 0)
Maybe the warning is correct but with macros like this, it is hard to avoid the warning really.
I don't think this warning is particularly useful if it breaks even GCC.
If GCC does not want to be warned about if (!x && x) then the warning is not useful for GCC. Then take it out of -Wextra. But it is definitely useful for others, and it found a bug in IRA.
For the record: This affects mips64-linux as well.
And yet another place: ../../trunk/gcc/config/mips/sb1.md:159: error: logical ‘or’ of collectively exhaustive tests is always true ../../trunk/gcc/config/mips/sb1.md:159: error: logical ‘or’ of collectively exhaustive tests is always true ../../trunk/gcc/config/mips/sb1.md:159: error: logical ‘or’ of collectively exhaustive tests is always true ../../trunk/gcc/config/mips/sb1.md:159: error: logical ‘or’ of collectively exhaustive tests is always true Can we either get the patch reverted, or move it out of -Wextra?
Just to chime in, the warning is a useful warning, but the way rs6000 and mips define FRAME_GROWS_DOWNWARD, the test in toplev.c will never succeed. I can see a couple of ways to fix this: 1) Revert the patch that moves this warning to -Wextra. I think this is a bad idea, since the warning does seem to be useful. 2) Disable the check in toplev.c. Again, I think this is useful in general, but as an immediate palative, it can be useful. 3) Add a new macro to say not to do the test in #2, and define it in mips and rs6000. This is doable, but in general it is not a good idea to add new global macros like this. 4) Change mips and rs6000 to have a global variable that is what FRAME_GROWS_DOWNWARD should be. This is certainly doable. The test will be tested at runtime, but never invoke the error message. 5) Move FRAME_GROWS_DOWNWARD (and STACK_GROWS_DOWNWARD, etc.) into the target structure, and set the field in the target structure from the macro. I tend to like this (and eventually move backends to set the field directly and get rid of the macros). I tend to like this idea best.
(In reply to comment #16) > Just to chime in, the warning is a useful warning, but the way rs6000 and mips > define FRAME_GROWS_DOWNWARD, the test in toplev.c will never succeed. > > 5) Move FRAME_GROWS_DOWNWARD (and STACK_GROWS_DOWNWARD, etc.) into the target > structure, and set the field in the target structure from the macro. I tend to Can't we change it to if (!FRAME_GROWS_DOWNWARD) { if (flag_stack_protect) { warning (0, "-fstack-protector not supported for this target"); flag_stack_protect = 0; } } with a comment?
The following patch moves the warning out of Wextra. I haven't tested it, though. Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 147679) +++ gcc/doc/invoke.texi (working copy) @@ -2806,7 +2806,6 @@ @gccoptlist{-Wclobbered @gol -Wempty-body @gol -Wignored-qualifiers @gol --Wlogical-op @gol -Wmissing-field-initializers @gol -Wmissing-parameter-type @r{(C only)} @gol -Wold-style-declaration @r{(C only)} @gol @@ -3793,8 +3792,7 @@ @opindex Wno-logical-op Warn about suspicious uses of logical operators in expressions. This includes using logical operators in contexts where a -bit-wise operator is likely to be expected. This warning is enabled by -@option{-Wextra}. +bit-wise operator is likely to be expected. @item -Waggregate-return @opindex Waggregate-return Index: gcc/c-opts.c =================================================================== --- gcc/c-opts.c (revision 147679) +++ gcc/c-opts.c (working copy) @@ -1073,8 +1073,6 @@ warn_override_init = extra_warnings; if (warn_ignored_qualifiers == -1) warn_ignored_qualifiers = extra_warnings; - if (warn_logical_op == -1) - warn_logical_op = extra_warnings; /* -Wpointer-sign is disabled by default, but it is enabled if any of -Wall or -pedantic are given. */
Created attachment 17890 [details] Proposed fix. I am testing this patch.
David, as I said in my message, this patch is just papering over the problem. While we might want to install this patch temporarily, to get the mips and rs6000 building again, I think a better solution is to change the circular dependency between FRAME_GROWS_DOWNWARD and flag_stack_protect, so at the point of the test in toplev.c the compiler won't give this warning. H. J. the problem with your patch is it that the compiler is likely to still issue the warning, since it will be discovered by the dataflow or SSA parts of the compiler.
(In reply to comment #16) > Just to chime in, the warning is a useful warning, but the way rs6000 and mips > define FRAME_GROWS_DOWNWARD, the test in toplev.c will never succeed. Yes and that is correct because we don't want that code to be involved at all. The whole if statement becomes false (which is a good thing we can detect it as being always false). > > I can see a couple of ways to fix this: > 1) Revert the patch that moves this warning to -Wextra. I think this is a bad > idea, since the warning does seem to be useful. > > 2) Disable the check in toplev.c. Again, I think this is useful in general, > but as an immediate palative, it can be useful. > > 3) Add a new macro to say not to do the test in #2, and define it in mips and > rs6000. This is doable, but in general it is not a good idea to add new global > macros like this. This is not a good option at all. a seperate option which I mentioned in an email rather than this bug report. So the current issue here is that we have: if (!DEFINED && x != 0) Where DEFINED is a macro which says (x != 0). This gets expanded as: if (!(x!=0) && x!=0) Obviously to the trained eye this would be a good warning but guess what DEFINED just happens to be based on x, it does not have to be. It would be nice to say only warn for x1 && !x2 if (obviously x1 == x2) : if either x1 or !x2 is from a macro but not both Thanks, Andrew Pinski
Another patch is posted at http://gcc.gnu.org/ml/gcc-patches/2009-05/msg01187.html
Subject: Re: [4.5 Regression] Revision 147596 breaks bootstrap On Tue, May 19, 2009 at 12:38:08AM -0000, hjl dot tools at gmail dot com wrote: > > > ------- Comment #22 from hjl dot tools at gmail dot com 2009-05-19 00:38 ------- > Another patch is posted at > > http://gcc.gnu.org/ml/gcc-patches/2009-05/msg01187.html > > > -- > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40172 > > ------- You are receiving this mail because: ------- > You are on the CC list for the bug, or are watching someone who is. I still feel that if this patch is put in, we eventually will have the same discussion, as the compiler gets smarter about flow control and propigation, since semantically: if (test1) { if (test2) { /* ... */ } } is the same as: if (test1 && test2) { /* ... */ }
(In reply to comment #23) > Subject: Re: [4.5 Regression] Revision 147596 breaks bootstrap > > I still feel that if this patch is put in, we eventually will have the same > discussion, as the compiler gets smarter about flow control and propigation, This particular warning comes from front-end. I don't think front-end will do such transformation. > since semantically: > > if (test1) > { > if (test2) > { > /* ... */ > } > } > > is the same as: > > if (test1 && test2) > { > /* ... */ > } > Gcc can eliminate many dead codes today. I don't think we will ever warn any of them, if at all.
Subject: Bug 40172 Author: manu Date: Tue May 19 19:29:27 2009 New Revision: 147717 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=147717 Log: 2009-05-19 Manuel López-Ibáñez <manu@gcc.gnu.org> PR c/40172 gcc/ * c.opt (Wlogical-op): Disabled by default. * c-opt (c_common_post_options): Do not enable Wlogical-op with Wextra. * doc/invoke.texi (Wlogical-op): Likewise. testsuite/ * gcc.dg/pr40172.c: Add -Wlogical-op to dg-options. Modified: trunk/gcc/ChangeLog trunk/gcc/c-opts.c trunk/gcc/c.opt trunk/gcc/doc/invoke.texi trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/pr40172.c
The case in toplev.c cannot be fixed without tracking macro expansions somehow, but I wonder why it warns (multiple times!) for this case: > ../../trunk/gcc/config/mips/sb1.md:159: error: logical �or� of collectively > exhaustive tests is always true > ../../trunk/gcc/config/mips/sb1.md:159: error: logical �or� of collectively > exhaustive tests is always true > ../../trunk/gcc/config/mips/sb1.md:159: error: logical �or� of collectively > exhaustive tests is always true > ../../trunk/gcc/config/mips/sb1.md:159: error: logical �or� of collectively > exhaustive tests is always true David, could you produce a testcase? Anyway, moved out of Wextra, so bootstrap should be fixed now.
Subject: Re: [4.5 Regression] Revision 147596 breaks bootstrap manu at gcc dot gnu dot org wrote: > ------- Comment #26 from manu at gcc dot gnu dot org 2009-05-19 20:29 ------- > The case in toplev.c cannot be fixed without tracking macro expansions somehow, > but I wonder why it warns (multiple times!) for this case: > >> ../../trunk/gcc/config/mips/sb1.md:159: error: logical �or� of collectively >> exhaustive tests is always true >> ../../trunk/gcc/config/mips/sb1.md:159: error: logical �or� of collectively >> exhaustive tests is always true >> ../../trunk/gcc/config/mips/sb1.md:159: error: logical �or� of collectively >> exhaustive tests is always true >> ../../trunk/gcc/config/mips/sb1.md:159: error: logical �or� of collectively >> exhaustive tests is always true > > David, could you produce a testcase? > It is in insn-attrtab.c, which is machine generated. I since the fatal warning is now disabled, it should be fine. The problem with insn-attrtab.c is that it is generated from the .md files and then includes all the target macros. So for this file you should probably never use -Wlogical-ops as filters that try to eliminate things in macros will fail. The whole file is conceptually one big macro.
Subject: Bug 40172 Author: hjl Date: Tue May 19 21:17:00 2009 New Revision: 147719 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=147719 Log: 2009-05-19 H.J. Lu <hongjiu.lu@intel.com> PR c/40172 * gcc.dg/pr40172.c: Renamed to ... * gcc.dg/pr40172-1.c: This. * gcc.dg/pr40172-2.c: New. * gcc.dg/pr40172-3.c: Likewise. Added: trunk/gcc/testsuite/gcc.dg/pr40172-1.c - copied unchanged from r147718, trunk/gcc/testsuite/gcc.dg/pr40172.c trunk/gcc/testsuite/gcc.dg/pr40172-2.c trunk/gcc/testsuite/gcc.dg/pr40172-3.c Removed: trunk/gcc/testsuite/gcc.dg/pr40172.c Modified: trunk/gcc/testsuite/ChangeLog
Subject: Bug 40172 Author: hjl Date: Tue May 19 21:24:23 2009 New Revision: 147720 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=147720 Log: 2009-05-19 H.J. Lu <hongjiu.lu@intel.com> Backport from mainline: 2009-05-19 H.J. Lu <hongjiu.lu@intel.com> PR c/40172 * gcc.dg/pr40172-1.c: New. * gcc.dg/pr40172-2.c: Likewise. * gcc.dg/pr40172-3.c: Likewise. Added: branches/gcc-4_4-branch/gcc/testsuite/gcc.dg/pr40172-1.c - copied unchanged from r147719, trunk/gcc/testsuite/gcc.dg/pr40172-1.c branches/gcc-4_4-branch/gcc/testsuite/gcc.dg/pr40172-2.c - copied unchanged from r147719, trunk/gcc/testsuite/gcc.dg/pr40172-2.c branches/gcc-4_4-branch/gcc/testsuite/gcc.dg/pr40172-3.c - copied, changed from r147719, trunk/gcc/testsuite/gcc.dg/pr40172-3.c Modified: branches/gcc-4_4-branch/gcc/testsuite/ChangeLog