Message ID | 545A9A86.1070008@codesourcery.com |
---|---|
State | New |
Headers | show |
Ping. On 05/11/14 21:45, Andrew Stubbs wrote: > This patch adds the following warning message: > > undefined.c:9:20: warning: statement may be undefined in the final loop > iteration. [-Waggressive-loop-optimizations] > for (i = 0; array[i] && i < 5; i++) > ^ > > (Where the code ought to read "i < 5 && array[i]".) > > The tree-ssa loop optimizations already eliminate useless loop-exit > conditions (i.e. conditions that will never be true). Unfortunately, > they also eliminate exit conditions that can be true, but only after > undefined behaviour has occurred. Typically, that means that the > undefined behaviour becomes an infinite loop (if it doesn't happen to > crash, of course), and that's surprising. It also looks more like a > compiler bug than a crash does. > > The new warning should highlight these cases but does not actually > change anything. I've included a comment where the compiler could be > adjusted to avoid the surprising optimization. Would it be appropriate > to do so? > > OK to commit? > > Andrew
On Wed, Nov 5, 2014 at 10:45 PM, Andrew Stubbs <ams@codesourcery.com> wrote: > This patch adds the following warning message: > > undefined.c:9:20: warning: statement may be undefined in the final loop > iteration. [-Waggressive-loop-optimizations] > for (i = 0; array[i] && i < 5; i++) > ^ > > (Where the code ought to read "i < 5 && array[i]".) > > The tree-ssa loop optimizations already eliminate useless loop-exit > conditions (i.e. conditions that will never be true). Unfortunately, they > also eliminate exit conditions that can be true, but only after undefined > behaviour has occurred. Typically, that means that the undefined behaviour > becomes an infinite loop (if it doesn't happen to crash, of course), and > that's surprising. It also looks more like a compiler bug than a crash does. > > The new warning should highlight these cases but does not actually change > anything. I've included a comment where the compiler could be adjusted to > avoid the surprising optimization. Would it be appropriate to do so? > > OK to commit? Please find a better way to communicate possibly_undefined_stmt than enlarging struct loop. Like associating it with the niter bound we record (so you can also have more than one). Thanks, Richard. > Andrew
On 12/11/14 11:15, Richard Biener wrote: > Please find a better way to communicate possibly_undefined_stmt than > enlarging struct loop. Like associating it with the niter bound > we record (so you can also have more than one). Unfortunately, the bounds get regenerated frequently, but the upper bound can only get reduced once, so the subsequent generations would lose the possibly_undefined_stmt. I did originally try warning at the point where the upper bound gets reduced, but there were too many false positives. The two part solution I found gives the result I was looking for, but maybe there is another way? I'm going to keep looking, but any suggestions would be appreciated. Andrew
2014-11-05 Andrew Stubbs <ams@codesourcery.com> gcc/ * tree-ssa-loop-niter.c (maybe_lower_iteration_bound): Set loop->possibly_undefined_stmt appropriately. * tree-ssa-loop-ivcanon.c (remove_redundant_iv_tests): Warn if any tests have loop->possibly_undefined_stmt set. * cfgloop.h (struct loop): Add field "possibly_undefined_stmt". gcc/testsuite/ * gcc.dg/undefined-loop.c: New file. Index: gcc/tree-ssa-loop-niter.c =================================================================== --- gcc/tree-ssa-loop-niter.c (revision 217085) +++ gcc/tree-ssa-loop-niter.c (working copy) @@ -3320,6 +3320,7 @@ visited = BITMAP_ALLOC (NULL); bitmap_set_bit (visited, loop->header->index); found_exit = false; + gimple problem_stmt = NULL; do { @@ -3334,6 +3335,8 @@ if (not_executed_last_iteration->contains (stmt)) { stmt_found = true; + if (!problem_stmt) + problem_stmt = stmt; break; } if (gimple_has_side_effects (stmt)) @@ -3375,6 +3378,8 @@ if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, "Reducing loop iteration estimate by 1; " "undefined statement must be executed at the last iteration.\n"); + if (!loop->possibly_undefined_stmt) + loop->possibly_undefined_stmt = problem_stmt; record_niter_bound (loop, loop->nb_iterations_upper_bound - 1, false, true); } Index: gcc/testsuite/gcc.dg/undefined-loop.c =================================================================== --- gcc/testsuite/gcc.dg/undefined-loop.c (revision 0) +++ gcc/testsuite/gcc.dg/undefined-loop.c (revision 0) @@ -0,0 +1,15 @@ +/* Check that loops whose final iteration is undefined are detected. */ +/* { dg-do compile } */ +/* { dg-options "-O2 -Waggressive-loop-optimizations" } */ + +void doSomething(char); + +char array[5]; + +void +foo (void) +{ + int i; + for (i = 0; array[i] && i < 5; i++) /* { dg-warning "statement may be undefined in the final loop iteration" } */ + doSomething(array[i]); +} Index: gcc/tree-ssa-loop-ivcanon.c =================================================================== --- gcc/tree-ssa-loop-ivcanon.c (revision 217085) +++ gcc/tree-ssa-loop-ivcanon.c (working copy) @@ -86,6 +86,7 @@ #include "target.h" #include "tree-cfgcleanup.h" #include "builtins.h" +#include "diagnostic-core.h" /* Specifies types of loops that may be unrolled. */ @@ -586,6 +587,18 @@ wi::to_widest (niter.niter))) continue; + /* If another loop exit has previously been suspected of causing + undefined behavior then removing other exit statements may be + unsafe. */ + if (loop->possibly_undefined_stmt) + { + warning_at (gimple_location (loop->possibly_undefined_stmt), + OPT_Waggressive_loop_optimizations, + "statement may be undefined in the final loop iteration."); + /* We could avoid the unsafe optimzation here: + continue; */ + } + if (dump_file && (dump_flags & TDF_DETAILS)) { fprintf (dump_file, "Removed pointless exit: "); Index: gcc/cfgloop.h =================================================================== --- gcc/cfgloop.h (revision 217085) +++ gcc/cfgloop.h (working copy) @@ -179,6 +179,10 @@ already. */ bool warned_aggressive_loop_optimizations; + /* Set if the loop count was reduced do to a statement that is undefined + in the final iteration. */ + gimple possibly_undefined_stmt; + /* An integer estimation of the number of iterations. Estimate_state describes what is the state of the estimation. */ enum loop_estimation estimate_state;