diff mbox

Warn on undefined loop exit

Message ID 545A9A86.1070008@codesourcery.com
State New
Headers show

Commit Message

Andrew Stubbs Nov. 5, 2014, 9:45 p.m. UTC 
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

Comments

Andrew Stubbs Nov. 12, 2014, 10:35 a.m. UTC | #1 
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
Richard Biener Nov. 12, 2014, 11:15 a.m. UTC | #2 
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
Andrew Stubbs Nov. 13, 2014, 9:35 p.m. UTC | #3 
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
diff mbox

Patch

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;