[asan] Patch - fix an ICE in asan.c

[Jakub Jelinek]

On Sat, Nov 10, 2012 at 07:54:34PM +0100, Tobias Burnus wrote:
> 2012-11-10  Tobias Burnus  <burnus@net-b.de>
> 	    Jakub Jelinek  <jakub@redhat.com>
> 
>         * asan.c (maybe_instrument_builtin_call): Set *iter
>         to gsi for the call at the end.
> 	(transform_statements): Leave loop when gsi_end_p.

But IMHO it is still wrong.  On e.g.
typedef __SIZE_TYPE__ size_t;

size_t
foo (size_t x, char *y)
{
  if (x)
    x = __builtin_strlen (y);
  return x;
}

size_t
bar (const char *y, const char *z)
{
  size_t a, b;
  a = __builtin_strlen (y);
  b = __builtin_strlen (z);
  return a + b;
}
you don't want to crash in foo (where there are no statements after
strlen in the same bb (with my previous patch it would still crash),
but in bar you also want to instrument both calls, not just the first one.
So, for strlen we need to make sure gsi_next isn't performed in
transform_statements.

2012-11-12  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (instrument_strlen_call): Return bool whether the call has
	been instrumented.
	(maybe_instrument_builtin_call): Change return value to mean whether
	caller should avoid gsi_next before processing next statement.  Pass
	thru return value from instrument_strlen_call.  Set *iter to gsi for
	the call at the end.
	(maybe_instrument_call): Return bool whether caller should avoid
	gsi_next.
	(transform_statements): Don't do gsi_next if maybe_instrument_call
	returned true.

--- gcc/asan.c.jj	2012-11-02 00:09:22.000000000 +0100
+++ gcc/asan.c	2012-11-12 09:34:03.226777133 +0100
@@ -824,7 +824,7 @@ instrument_mem_region_access (tree base,
    access to the last byte of the argument; it uses the result of the
    call to deduce the offset of that last byte.  */
 
-static void
+static bool
 instrument_strlen_call (gimple_stmt_iterator *iter)
 {
   gimple call = gsi_stmt (*iter);
@@ -839,7 +839,7 @@ instrument_strlen_call (gimple_stmt_iter
   if (len == NULL)
     /* Some passes might clear the return value of the strlen call;
        bail out in that case.  */
-    return;
+    return false;
   gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (len)));
 
   location_t loc = gimple_location (call);
@@ -881,13 +881,20 @@ instrument_strlen_call (gimple_stmt_iter
 		    /*before_p=*/false, /*is_store=*/false, 1);
 
   /* Ensure that iter points to the statement logically following the
-     one it was initially pointing to.  */
+     one it was initially pointing to.  Return true to inform
+     transform_statements that it shouldn't do gsi_next (&i) in that
+     case, that statement is the first statement in a block that would
+     be otherwise skipped (too high block number), by doing gsi_next (&i)
+     either that statement wouldn't be instrumented or, if there are no
+     statement, transform_statements could crash in gsi_next (&i).  */
   *iter = gsi;
+  return true;
 }
 
-/* if the statement pointed to by the iterator iter is a call to a
-   builtin memory access function, instrument it and return true.
-   otherwise, return false.  */
+/* If the statement pointed to by the iterator iter is a call to a
+   builtin memory access function, instrument it.  Return true
+   if *ITER should be processed next, false if gsi_next should
+   be done on it first.  */
 
 static bool
 maybe_instrument_builtin_call (gimple_stmt_iterator *iter)
@@ -951,8 +958,7 @@ maybe_instrument_builtin_call (gimple_st
       break;
 
     case BUILT_IN_STRLEN:
-      instrument_strlen_call (iter);
-      return true;
+      return instrument_strlen_call (iter);
 
     /* And now the __atomic* and __sync builtins.
        These are handled differently from the classical memory memory
@@ -1170,7 +1176,7 @@ maybe_instrument_builtin_call (gimple_st
 	  gcc_unreachable ();
 
 	instrument_derefs (iter, dest, loc, is_store);
-	return true;
+	return false;
       }
 
     default:
@@ -1191,7 +1197,8 @@ maybe_instrument_builtin_call (gimple_st
       else if (dest != NULL_TREE)
 	instrument_mem_region_access (dest, len, iter,
 				      loc, /*is_store=*/true);
-      return true;
+      *iter = gsi_for_stmt (call);
+      return false;
     }
   return false;
 }
@@ -1217,10 +1224,10 @@ instrument_assignment (gimple_stmt_itera
    calls that are instrumented are some built-in functions that access
    memory.  Look at maybe_instrument_builtin_call to learn more.  */
 
-static void
+static bool
 maybe_instrument_call (gimple_stmt_iterator *iter)
 {
-  maybe_instrument_builtin_call (iter);
+  return maybe_instrument_builtin_call (iter);
 }
 
 /* asan: this looks too complex. Can this be done simpler? */
@@ -1239,14 +1246,19 @@ transform_statements (void)
   FOR_EACH_BB (bb)
     {
       if (bb->index >= saved_last_basic_block) continue;
-      for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
+      for (i = gsi_start_bb (bb); !gsi_end_p (i); )
         {
 	  gimple s = gsi_stmt (i);
 
 	  if (gimple_assign_single_p (s))
 	    instrument_assignment (&i);
 	  else if (is_gimple_call (s))
-	    maybe_instrument_call (&i);
+	    {
+	      if (maybe_instrument_call (&i))
+		/* Avoid gsi_next (&i).  */
+		continue;
+	    }
+          gsi_next (&i);
         }
     }
 }


	Jakub