diff mbox

[RFC] Sanopt for use-after-scope ASAN_MARK internal functions

Message ID be360aeb-78b1-816c-d40c-c0929a6322af@suse.cz
State Superseded
Headers show

Commit Message

Martin Liška Dec. 9, 2016, 11:39 a.m. UTC
Hello.

I've spent quite some time reading sanopt pass and I eventually decided to come up
with quite simplified optimization algorithm for ASAN_MARK internal functions. As the
most interesting (common) cases are that an ASAN_MARK unpoison is dominated by entry
block (where we unpoison all variables). Second interesting situation is ASAN_MARK poison
which is not followed by a ASAN_CHECK (in post dominator tree). Both these internal functions
can be removed.

There are numbers for linux kernel (running for couple of minutes):

  20306 expand ASAN_MARK
   2892 Removing ASAN_MARK poison
   8124 Removing ASAN_MARK unpoison

This removes 1/3 of ASAN_MARK internal function call.

For tramp3d the situation is not so happy as it massively utilizes C++ temporaries:

  34163 expand ASAN_MARK
   1920 Removing ASAN_MARK poison
   2051 Removing ASAN_MARK unpoison

Thoughts?
Thanks,
Martin

Comments

Jakub Jelinek Dec. 9, 2016, 11:56 a.m. UTC | #1
On Fri, Dec 09, 2016 at 12:39:24PM +0100, Martin Liška wrote:
> +	  if (is_gimple_call (stmt) && gimple_call_internal_p (stmt))

> +	    {

> +	      enum internal_fn ifn = gimple_call_internal_fn (stmt);

> +	      switch (ifn)

> +		{

> +		case IFN_ASAN_MARK:

> +		  {

> +		    HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt, 0));

> +		    bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;

> +		    if (is_clobber)

> +		      {

> +			bitmap_set_bit (with_poison, bb->index);

> +			finish = true;

> +		      }

> +		    break;

> +		  }

> +		  default:

> +		    break;

> +		}


This looks weird.  Wouldn't it be nicer to just use
	      if (gimple_call_internal_fn (stmt) == IFN_ASAN_MARK)
		{
		  HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt, 0));
		  bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
		  if (is_clobber)
		    {
		      bitmap_set_bit (with_poison, bb->index);
		      finish = true;
		    }
		}
(or don't use finish and just break; there)?
> +	      enum internal_fn ifn = gimple_call_internal_fn (stmt);

> +	      switch (ifn)

> +		{

> +		case IFN_ASAN_MARK:

> +		  {


Likewise.

No function comment:

> +static bool

> +maybe_contains_asan_check (gimple *stmt)

> +{

> +  if (is_gimple_call (stmt))

> +    {

> +      if (gimple_call_internal_p (stmt))

> +	{

> +	  enum internal_fn ifn = gimple_call_internal_fn (stmt);

> +	  switch (ifn)

> +	    {

> +	    case IFN_ASAN_CHECK:

> +	      return true;

> +	    default:

> +	      return false;


Are all internal functions really known to be ASAN_CHECK free?

> +	    }

> +	}

> +      else

> +        return true;


What about builtins?  Many will not be fine, but many should be known
to be ASAN_CHECK free.  Consider e.g. most math builtins (except sincos).

> @@ -698,6 +928,9 @@ pass_sanopt::execute (function *fun)

>    bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX

>      && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;

>  

> +  sanitize_asan_mark_unpoison ();

> +  sanitize_asan_mark_poison ();

> +


What I don't really like here is that you force several IL walks for all
sanitizers, just in case.  Look e.g. how asan_num_accesses is computed,
couldn't you compute similarly has_asan_marks and guard those two functions
on that?

	Jakub
diff mbox

Patch

From c91ad7c9fa2406145467c473dc7239479635c9a2 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 25 Nov 2016 15:05:33 +0100
Subject: [PATCH] Add sanopt for ASAN_MARK poison and unpoison.

---
 gcc/sanopt.c | 235 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 234 insertions(+), 1 deletion(-)

diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 77307d9..a110db5 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -162,7 +162,6 @@  struct sanopt_ctx
   int asan_num_accesses;
 };
 
-
 /* Return true if there might be any call to free/munmap operation
    on any path in between DOM (which should be imm(BB)) and BB.  */
 
@@ -671,6 +670,237 @@  public:
 
 }; // class pass_sanopt
 
+static void
+sanitize_asan_mark_unpoison (void)
+{
+  /* 1) Find all BBs that contain an ASAN_MARK poison call.  */
+  auto_sbitmap with_poison (last_basic_block_for_fn (cfun) + 1);
+  bitmap_clear (with_poison);
+  basic_block bb;
+
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      if (bitmap_bit_p (with_poison, bb->index))
+	continue;
+
+      gimple_stmt_iterator gsi;
+      bool finish = false;
+      for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
+	{
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (is_gimple_call (stmt) && gimple_call_internal_p (stmt))
+	    {
+	      enum internal_fn ifn = gimple_call_internal_fn (stmt);
+	      switch (ifn)
+		{
+		case IFN_ASAN_MARK:
+		  {
+		    HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt, 0));
+		    bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
+		    if (is_clobber)
+		      {
+			bitmap_set_bit (with_poison, bb->index);
+			finish = true;
+		      }
+		    break;
+		  }
+		  default:
+		    break;
+		}
+	    }
+
+	  if (finish)
+	    break;
+	}
+    }
+
+  auto_sbitmap poisoned (last_basic_block_for_fn (cfun) + 1);
+  bitmap_clear (poisoned);
+  auto_sbitmap worklist (last_basic_block_for_fn (cfun) + 1);
+  bitmap_copy (worklist, with_poison);
+
+  /* 2) Propagate the information to all reachable blocks.  */
+  while (!bitmap_empty_p (worklist))
+    {
+      unsigned i = bitmap_first_set_bit (worklist);
+      bitmap_clear_bit (worklist, i);
+      basic_block bb = BASIC_BLOCK_FOR_FN (cfun, i);
+      gcc_assert (bb);
+
+      edge e;
+      edge_iterator ei;
+      FOR_EACH_EDGE (e, ei, bb->succs)
+	if (!bitmap_bit_p (poisoned, e->dest->index))
+	  {
+	    bitmap_set_bit (poisoned, e->dest->index);
+	    bitmap_set_bit (worklist, e->dest->index);
+	  }
+    }
+
+  /* 3) Iterate all BBs not included in POISONED BBs and remove unpoison
+	ASAN_MARK preceding an ASAN_MARK poison (which can still happen).  */
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      if (bitmap_bit_p (poisoned, bb->index))
+	continue;
+
+      gimple_stmt_iterator gsi;
+      bool finish = false;
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi);)
+	{
+	  bool next = true;
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (is_gimple_call (stmt) && gimple_call_internal_p (stmt))
+	    {
+	      enum internal_fn ifn = gimple_call_internal_fn (stmt);
+	      switch (ifn)
+		{
+		case IFN_ASAN_MARK:
+		  {
+		    HOST_WIDE_INT flags
+		      = tree_to_shwi (gimple_call_arg (stmt, 0));
+		    bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
+		    if (is_clobber)
+		      finish = true;
+		    else
+		      {
+			if (dump_file)
+			  fprintf (dump_file, "Removing ASAN_MARK unpoison\n");
+			unlink_stmt_vdef (stmt);
+			gsi_remove (&gsi, true);
+			next = false;
+		      }
+		    break;
+		  }
+		  default:
+		    break;
+		}
+	    }
+
+	  if (finish)
+	    break;
+
+	  if (next)
+	    gsi_next (&gsi);
+	}
+    }
+}
+
+static bool
+maybe_contains_asan_check (gimple *stmt)
+{
+  if (is_gimple_call (stmt))
+    {
+      if (gimple_call_internal_p (stmt))
+	{
+	  enum internal_fn ifn = gimple_call_internal_fn (stmt);
+	  switch (ifn)
+	    {
+	    case IFN_ASAN_CHECK:
+	      return true;
+	    default:
+	      return false;
+	    }
+	}
+      else
+        return true;
+    }
+  else if (is_a<gasm *> (stmt))
+    return true;
+
+  return false;
+}
+
+static void
+sanitize_asan_mark_poison (void)
+{
+  /* 1) Find all BBs that possibly contain an ASAN_CHECK.  */
+  auto_sbitmap with_check (last_basic_block_for_fn (cfun) + 1);
+  bitmap_clear (with_check);
+  basic_block bb;
+
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      if (bitmap_bit_p (with_check, bb->index))
+	continue;
+
+      gimple_stmt_iterator gsi;
+      bool finish = false;
+      for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
+	{
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (maybe_contains_asan_check (stmt))
+	    {
+	      bitmap_set_bit (with_check, bb->index);
+	      finish = true;
+	    }
+
+	  if (finish)
+	    break;
+	}
+    }
+
+  auto_sbitmap can_reach_check (last_basic_block_for_fn (cfun) + 1);
+  bitmap_clear (can_reach_check);
+  auto_sbitmap worklist (last_basic_block_for_fn (cfun) + 1);
+  bitmap_copy (worklist, with_check);
+
+  /* 2) Propagate the information to all definitions blocks.  */
+  while (!bitmap_empty_p (worklist))
+    {
+      unsigned i = bitmap_first_set_bit (worklist);
+      bitmap_clear_bit (worklist, i);
+      basic_block bb = BASIC_BLOCK_FOR_FN (cfun, i);
+      gcc_assert (bb);
+
+      edge e;
+      edge_iterator ei;
+      FOR_EACH_EDGE (e, ei, bb->preds)
+	if (!bitmap_bit_p (can_reach_check, e->src->index))
+	  {
+	    bitmap_set_bit (can_reach_check, e->src->index);
+	    bitmap_set_bit (worklist, e->src->index);
+	  }
+    }
+
+  /* 3) Iterate all BBs not included in CAN_REACH_CHECK BBs and remove poison
+	ASAN_MARK not followed by a call to function having an ASAN_CHECK.  */
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      if (bitmap_bit_p (can_reach_check, bb->index))
+	continue;
+
+      gimple_stmt_iterator gsi;
+      bool finish = false;
+      for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi);)
+	{
+	  bool prev = true;
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (maybe_contains_asan_check (stmt))
+	    finish = true;
+	  else if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+	    {
+	      HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt, 0));
+	      bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
+	      if (is_clobber)
+		{
+		  if (dump_file)
+		    fprintf (dump_file, "Removing ASAN_MARK poison\n");
+		  unlink_stmt_vdef (stmt);
+		  gsi_remove (&gsi, true);
+		  prev = false;
+		}
+	    }
+
+	  if (finish)
+	    break;
+
+	  if (prev)
+	    gsi_prev (&gsi);
+	}
+    }
+}
+
 unsigned int
 pass_sanopt::execute (function *fun)
 {
@@ -698,6 +928,9 @@  pass_sanopt::execute (function *fun)
   bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX
     && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
 
+  sanitize_asan_mark_unpoison ();
+  sanitize_asan_mark_poison ();
+
   bool need_commit_edge_insert = false;
   FOR_EACH_BB_FN (bb, fun)
     {
-- 
2.10.2