diff mbox

Small asan improvement

Message ID 20130213190845.GZ4385@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 13, 2013, 7:08 p.m. UTC
Hi!

This is just a small improvement for Dodji's work.  We can flush the hash
table with memory references known to be instrumented only at extended basic
block boundaries, no need to flush it at every bb (of course, for 4.9 we
want to do something better).  And, now that we have nonfreeing_call_p
predicate, we can avoid also flushing at builtin calls that are known not to
free memory or use callbacks etc.

The first 5 hunks are there because I've noticed that we were creating a
fallthru edge from basic blocks with __asan_report_* call (which is
noreturn) - we shouldn't have any edge there.

Where these change make a difference is e.g.:
void baz (long *, int);

long
foo (long *p, int q)
{
  long a = *p;
  if (q)
    {
      *p = 6;
      baz (p, 7);
    }
  else
    {
      *p = 25;
      baz (p + 1, 17);
    }
  return a;
}

long
bar (long *p, double q, double r)
{
  long a = *p;
  a += __builtin_pow (q, r);
  *p = 6;
  return a;
}

Without the patch, in asan1 pass there are two load instrumentations
(one in each fn) and 3 store insturmentations, with the patch 2 load
and one store instrumentation (only one of then and else bbs are
considered extended bb).  For foo later optimizations actually optimize
away all of store instrumentations, but it doesn't have to happen always.

2013-02-13  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (create_cond_insert_point): Add create_then_fallthru_edge
	argument.  If it is false, don't create edge from then_bb to
	fallthru_bb.
	(insert_if_then_before_iter): Pass true to it.
	(build_check_stmt): Pass false to it.
	(transform_statements): Flush hash table only on extended basic
	block boundaries, rather than at the beginning of every bb.
	Don't flush hash table on nonfreeing_call_p calls.
	* tree-flow.h (nonfreeing_call_p): New prototype.
	* tree-ssa-phiopt.c (nonfreeing_call_p): No longer static.


	Jakub

Comments

Dodji Seketeli Feb. 13, 2013, 8:40 p.m. UTC | #1
Jakub Jelinek <jakub@redhat.com> writes:

> This is just a small improvement for Dodji's work.  We can flush the hash
> table with memory references known to be instrumented only at extended basic
> block boundaries, no need to flush it at every bb (of course, for 4.9 we
> want to do something better).

This makes a lot of sense, thank you for doing this.

>  And, now that we have nonfreeing_call_p predicate,

I didn't even know this existed ...

> we can avoid also flushing at builtin calls that are known not to
> free memory or use callbacks etc.

Right.

> The first 5 hunks are there because I've noticed that we were creating a
> fallthru edge from basic blocks with __asan_report_* call (which is
> noreturn) - we shouldn't have any edge there.

Good catch.

> Where these change make a difference is e.g.:
> void baz (long *, int);
>
> long
> foo (long *p, int q)
> {
>   long a = *p;
>   if (q)
>     {
>       *p = 6;
>       baz (p, 7);
>     }
>   else
>     {
>       *p = 25;
>       baz (p + 1, 17);
>     }
>   return a;
> }
>
> long
> bar (long *p, double q, double r)
> {
>   long a = *p;
>   a += __builtin_pow (q, r);
>   *p = 6;
>   return a;
> }
>
> Without the patch, in asan1 pass there are two load instrumentations
> (one in each fn) and 3 store insturmentations, with the patch 2 load
> and one store instrumentation (only one of then and else bbs are
> considered extended bb).  For foo later optimizations actually optimize
> away all of store instrumentations, but it doesn't have to happen
> always.

Maybe we could have this added to he GCC test suite?

> 2013-02-13  Jakub Jelinek  <jakub@redhat.com>
>
> 	* asan.c (create_cond_insert_point): Add create_then_fallthru_edge
> 	argument.  If it is false, don't create edge from then_bb to
> 	fallthru_bb.
> 	(insert_if_then_before_iter): Pass true to it.
> 	(build_check_stmt): Pass false to it.
> 	(transform_statements): Flush hash table only on extended basic
> 	block boundaries, rather than at the beginning of every bb.
> 	Don't flush hash table on nonfreeing_call_p calls.
> 	* tree-flow.h (nonfreeing_call_p): New prototype.
> 	* tree-ssa-phiopt.c (nonfreeing_call_p): No longer static.

This looks OK to me.  Thanks.
diff mbox

Patch

--- gcc/asan.c.jj	2013-02-13 11:53:42.000000000 +0100
+++ gcc/asan.c	2013-02-13 19:46:20.033555655 +0100
@@ -1185,6 +1185,9 @@  report_error_func (bool is_store, int si
    'then block' of the condition statement to be inserted by the
    caller.
 
+   If CREATE_THEN_FALLTHRU_EDGE is false, no edge will be created from
+   *THEN_BLOCK to *FALLTHROUGH_BLOCK.
+
    Similarly, the function will set *FALLTRHOUGH_BLOCK to the 'else
    block' of the condition statement to be inserted by the caller.
 
@@ -1201,6 +1204,7 @@  static gimple_stmt_iterator
 create_cond_insert_point (gimple_stmt_iterator *iter,
 			  bool before_p,
 			  bool then_more_likely_p,
+			  bool create_then_fallthru_edge,
 			  basic_block *then_block,
 			  basic_block *fallthrough_block)
 {
@@ -1226,7 +1230,8 @@  create_cond_insert_point (gimple_stmt_it
     ? PROB_VERY_UNLIKELY
     : PROB_ALWAYS - PROB_VERY_UNLIKELY;
   e->probability = PROB_ALWAYS - fallthrough_probability;
-  make_single_succ_edge (then_bb, fallthru_bb, EDGE_FALLTHRU);
+  if (create_then_fallthru_edge)
+    make_single_succ_edge (then_bb, fallthru_bb, EDGE_FALLTHRU);
 
   /* Set up the fallthrough basic block.  */
   e = find_edge (cond_bb, fallthru_bb);
@@ -1277,6 +1282,7 @@  insert_if_then_before_iter (gimple cond,
     create_cond_insert_point (iter,
 			      /*before_p=*/true,
 			      then_more_likely_p,
+			      /*create_then_fallthru_edge=*/true,
 			      then_bb,
 			      fallthrough_bb);
   gsi_insert_after (&cond_insert_point, cond, GSI_NEW_STMT);
@@ -1314,6 +1320,7 @@  build_check_stmt (location_t location, t
      statement for the instrumentation.  */
   gsi = create_cond_insert_point (iter, before_p,
 				  /*then_more_likely_p=*/false,
+				  /*create_then_fallthru_edge=*/false,
 				  &then_bb,
 				  &else_bb);
 
@@ -1883,15 +1890,31 @@  maybe_instrument_call (gimple_stmt_itera
 static void
 transform_statements (void)
 {
-  basic_block bb;
+  basic_block bb, last_bb = NULL;
   gimple_stmt_iterator i;
   int saved_last_basic_block = last_basic_block;
 
   FOR_EACH_BB (bb)
     {
-      empty_mem_ref_hash_table ();
+      basic_block prev_bb = bb;
 
       if (bb->index >= saved_last_basic_block) continue;
+
+      /* Flush the mem ref hash table, if current bb doesn't have
+	 exactly one predecessor, or if that predecessor (skipping
+	 over asan created basic blocks) isn't the last processed
+	 basic block.  Thus we effectively flush on extended basic
+	 block boundaries.  */
+      while (single_pred_p (prev_bb))
+	{
+	  prev_bb = single_pred (prev_bb);
+	  if (prev_bb->index < saved_last_basic_block)
+	    break;
+	}
+      if (prev_bb != last_bb)
+	empty_mem_ref_hash_table ();
+      last_bb = bb;
+
       for (i = gsi_start_bb (bb); !gsi_end_p (i);)
 	{
 	  gimple s = gsi_stmt (i);
@@ -1909,11 +1932,11 @@  transform_statements (void)
 	    {
 	      /* No instrumentation happened.
 
-		 If the current instruction is a function call, let's
-		 forget about the memory references that got
-		 instrumented.  Otherwise we might miss some
-		 instrumentation opportunities.  */
-	      if (is_gimple_call (s))
+		 If the current instruction is a function call that
+		 might free something, let's forget about the memory
+		 references that got instrumented.  Otherwise we might
+		 miss some instrumentation opportunities.  */
+	      if (is_gimple_call (s) && !nonfreeing_call_p (s))
 		empty_mem_ref_hash_table ();
 
 	      gsi_next (&i);
--- gcc/tree-flow.h.jj	2013-01-16 17:04:20.000000000 +0100
+++ gcc/tree-flow.h	2013-02-13 17:29:33.512538537 +0100
@@ -609,6 +609,7 @@  struct tree_niter_desc
 /* In tree-ssa-phiopt.c */
 bool empty_block_p (basic_block);
 basic_block *blocks_in_phiopt_order (void);
+bool nonfreeing_call_p (gimple);
 
 /* In tree-ssa-loop*.c  */
 
--- gcc/tree-ssa-phiopt.c.jj	2013-02-08 16:05:14.000000000 +0100
+++ gcc/tree-ssa-phiopt.c	2013-02-13 17:28:47.442803866 +0100
@@ -1339,7 +1339,7 @@  add_or_mark_expr (basic_block bb, tree e
 
 /* Return true when CALL is a call stmt that definitely doesn't
    free any memory or makes it unavailable otherwise.  */
-static bool
+bool
 nonfreeing_call_p (gimple call)
 {
   if (gimple_call_builtin_p (call, BUILT_IN_NORMAL)