Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

On 23 Sep 09:55, Jeff Law wrote:
> On 09/22/14 00:40, Ilya Enkovich wrote:
> >
> >Bounds don't have to vary for different pointers.  E.g. p and p + 1
> >always have equal bounds.  In this particular case we have function
> >pointers and all of them have default bounds.
> OK.  It looked a bit odd and I wanted to make sure there wasn't
> something fundamentally wrong.
> 
> >>>I attach a dump I got from Chrome compilation with no additional
> >>>checks restrictions in split.  Original function returns value defined
> >>>by phi node in return_bb and bounds defined in BB2.  Split part
> >>>contains BB3, BB4 and BB5 and resulting function part has usage of
> >>>returned bounds but no producer for it.
> >>
> >>Right, but my question is whether or not the bounds from BB2 were really the
> >>correct bounds to be using in the first place!  I would have expected a PHI
> >>in BB6 to select the bounds based on the path leading to BB6, much like we
> >>select a different return value.
> >
> >Consider we have pointer computation and then
> >
> >return __bnd_init_ptr_bounds (res);
> >
> >In such case you would never have a PHI node for bounds.  Also do not
> >forget that we may have no PHI nodes for both return value and return
> >bounds.  In such case we could also easily fall into undefined value
> >as in dump.
> This code (visit_bb, find_return_bb, consider_split) is a bit of a
> mess, but I do see what you're trying to do now.  Thanks for being
> patient with my questions.
> 
> If I were to look at this at a high level, the core issue seems to
> me that we're really not prepared to handle functions with multiple
> return values.  This shows up in your MPX work, but IIRC there's
> cases in the atomics where we have multiple return values as well.
> I wouldn't be surprised if there's latent bugs with splitting &
> atomics lurking to bite us one day.
> 
> So if I'm reading all this code correctly, given a return block
> which returns a (pointer,bounds) pair, if the bounds are set  by a
> normal statement (ie, not a PHI), then we won't use that block for
> RETURN_BB.  So there's nothing to worry about in that case.
> Similarly if the bounds are  set by a PHI in the return block,
> consider_split will reject that split point as well.  So really the
> only case here is when the bounds are set in another dominating
> block.  Right?
> 
> I can see how you're using the relevant part of the same test we
> need for the retval.  My gut tells me we want to commonize that test
> so that they don't get out-of-sync.    Specifically, can we pull the
> code which sets split_part_set_retbnd into a little function, then
> use it for the retval here too:
> 
>   else if (TREE_CODE (retval) == SSA_NAME)
>     current->split_part_set_retval
>       = (!SSA_NAME_IS_DEFAULT_DEF (retval)
>          && (bitmap_bit_p (current->split_bbs,
>                           gimple_bb (SSA_NAME_DEF_STMT (retval))->index)
>              || gimple_bb (SSA_NAME_DEF_STMT (retval)) == return_bb));
> 
> 
> 
> Iteration through the statements in find_retbnd should start at the
> end of the block and walk backwards.  It probably doesn't matter in
> practice all that much, but might as well be sensible since the
> GIMPLE_RETURN is almost always going to be the last statement in the
> block.
> 
> Similarly for the statement walk in split_function when you want to
> replace retbnd with new one.
> 
> It seems like the code to build the bndret call to obtain bounds is
> repeated.  Can you refactor that into its own little function and
> just use that.  It's not a huge amount of code, but it does make
> things a bit easier to follow.
> 
> With those changes this will be OK.
> 
> Jeff
> 
> 

Here is a version with modifications you proposed.  Thanks for review!

Ilya
--
2014-09-25  Ilya Enkovich  <ilya.enkovich@intel.com>

	* ipa-split.c: Include tree-chkp.h.
	(find_retbnd): New.
	(split_part_set_ssa_name_p): New.
	(consider_split): Do not split retbnd and retval
	producers.
	(insert_bndret_call_after): new.
	(split_function): Propagate Pointer Bounds Checker
	instrumentation marks and handle returned bounds.


diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index 2af3a93..7a1b75e 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -110,6 +110,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-pretty-print.h"
 #include "ipa-inline.h"
 #include "cfgloop.h"
+#include "tree-chkp.h"
 
 /* Per basic block info.  */
 
@@ -151,6 +152,7 @@ struct split_point best_split_point;
 static bitmap forbidden_dominators;
 
 static tree find_retval (basic_block return_bb);
+static tree find_retbnd (basic_block return_bb);
 
 /* Callback for walk_stmt_load_store_addr_ops.  If T is non-SSA automatic
    variable, check it if it is present in bitmap passed via DATA.  */
@@ -370,6 +372,21 @@ dominated_by_forbidden (basic_block bb)
   return false;
 }
 
+/* For give split point CURRENT and return block RETURN_BB return 1
+   if ssa name VAL is set by split part and 0 otherwise.  */
+static bool
+split_part_set_ssa_name_p (tree val, struct split_point *current,
+			   basic_block return_bb)
+{
+  if (TREE_CODE (val) != SSA_NAME)
+    return false;
+
+  return (!SSA_NAME_IS_DEFAULT_DEF (val)
+	  && (bitmap_bit_p (current->split_bbs,
+			    gimple_bb (SSA_NAME_DEF_STMT (val))->index)
+	      || gimple_bb (SSA_NAME_DEF_STMT (val)) == return_bb));
+}
+
 /* We found an split_point CURRENT.  NON_SSA_VARS is bitmap of all non ssa
    variables used and RETURN_BB is return basic block.
    See if we can split function here.  */
@@ -387,6 +404,7 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
   unsigned int i;
   int incoming_freq = 0;
   tree retval;
+  tree retbnd;
   bool back_edge = false;
 
   if (dump_file && (dump_flags & TDF_DETAILS))
@@ -575,10 +593,7 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
        = bitmap_bit_p (non_ssa_vars, DECL_UID (SSA_NAME_VAR (retval)));
   else if (TREE_CODE (retval) == SSA_NAME)
     current->split_part_set_retval
-      = (!SSA_NAME_IS_DEFAULT_DEF (retval)
-	 && (bitmap_bit_p (current->split_bbs,
-			  gimple_bb (SSA_NAME_DEF_STMT (retval))->index)
-	     || gimple_bb (SSA_NAME_DEF_STMT (retval)) == return_bb));
+      = split_part_set_ssa_name_p (retval, current, return_bb);
   else if (TREE_CODE (retval) == PARM_DECL)
     current->split_part_set_retval = false;
   else if (TREE_CODE (retval) == VAR_DECL
@@ -588,6 +603,29 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
   else
     current->split_part_set_retval = true;
 
+  /* See if retbnd used by return bb is computed by header or split part.  */
+  retbnd = find_retbnd (return_bb);
+  if (retbnd)
+    {
+      bool split_part_set_retbnd
+	= split_part_set_ssa_name_p (retbnd, current, return_bb);
+
+      /* If we have both return value and bounds then keep their definitions
+	 in a single function.  We use SSA names to link returned bounds and
+	 value and therefore do not handle cases when result is passed by
+	 reference (which should not be our case anyway since bounds are
+	 returned for pointers only).  */
+      if ((DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))
+	   && current->split_part_set_retval)
+	  || split_part_set_retbnd != current->split_part_set_retval)
+	{
+	  if (dump_file && (dump_flags & TDF_DETAILS))
+	    fprintf (dump_file,
+		     "  Refused: split point splits return value and bounds\n");
+	  return;
+	}
+    }
+
   /* split_function fixes up at most one PHI non-virtual PHI node in return_bb,
      for the return value.  If there are other PHIs, give up.  */
   if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
@@ -710,6 +748,18 @@ find_retval (basic_block return_bb)
   return NULL;
 }
 
+/* Given return basic block RETURN_BB, see where return bounds are really
+   stored.  */
+static tree
+find_retbnd (basic_block return_bb)
+{
+  gimple_stmt_iterator bsi;
+  for (bsi = gsi_last_bb (return_bb); !gsi_end_p (bsi); gsi_prev (&bsi))
+    if (gimple_code (gsi_stmt (bsi)) == GIMPLE_RETURN)
+      return gimple_return_retbnd (gsi_stmt (bsi));
+  return NULL;
+}
+
 /* Callback for walk_stmt_load_store_addr_ops.  If T is non-SSA automatic
    variable, mark it as used in bitmap passed via DATA.
    Return true when access to T prevents splitting the function.  */
@@ -1079,6 +1129,19 @@ find_split_points (int overall_time, int overall_size)
   BITMAP_FREE (current.ssa_names_to_pass);
 }
 
+/* Build and insert initialization of returned bounds RETBND
+   for returned value RETVAL.  Statements are inserted after
+   a statement pointed by GSI and GSI is modified to point to
+   the last inserted statement.  */
+
+static void
+insert_bndret_call_after (tree retbnd, tree retval, gimple_stmt_iterator *gsi)
+{
+  tree fndecl = targetm.builtin_chkp_function (BUILT_IN_CHKP_BNDRET);
+  gimple bndret = gimple_build_call (fndecl, 1, retval);
+  gimple_call_set_lhs (bndret, retbnd);
+  gsi_insert_after (gsi, bndret, GSI_CONTINUE_LINKING);
+}
 /* Split function at SPLIT_POINT.  */
 
 static void
@@ -1095,8 +1158,9 @@ split_function (struct split_point *split_point)
   gimple call;
   edge e;
   edge_iterator ei;
-  tree retval = NULL, real_retval = NULL;
+  tree retval = NULL, real_retval = NULL, retbnd = NULL;
   bool split_part_return_p = false;
+  bool with_bounds = chkp_function_instrumented_p (current_function_decl);
   gimple last_stmt = NULL;
   unsigned int i;
   tree arg, ddef;
@@ -1245,6 +1309,12 @@ split_function (struct split_point *split_point)
       DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN;
       DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
     }
+
+  /* If the original function is instrumented then it's
+     part is also instrumented.  */
+  if (with_bounds)
+    chkp_function_mark_instrumented (node->decl);
+
   /* If the original function is declared inline, there is no point in issuing
      a warning for the non-inlinable part.  */
   DECL_NO_INLINE_WARNING_P (node->decl) = 1;
@@ -1279,6 +1349,7 @@ split_function (struct split_point *split_point)
 	args_to_pass[i] = arg;
       }
   call = gimple_build_call_vec (node->decl, args_to_pass);
+  gimple_call_set_with_bounds (call, with_bounds);
   gimple_set_block (call, DECL_INITIAL (current_function_decl));
   args_to_pass.release ();
 
@@ -1385,6 +1456,7 @@ split_function (struct split_point *split_point)
       if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
 	{
 	  real_retval = retval = find_retval (return_bb);
+	  retbnd = find_retbnd (return_bb);
 
 	  if (real_retval && split_point->split_part_set_retval)
 	    {
@@ -1429,6 +1501,21 @@ split_function (struct split_point *split_point)
 			  }
 		      update_stmt (gsi_stmt (bsi));
 		    }
+
+		  /* Replace retbnd with new one.  */
+		  if (retbnd)
+		    {
+		      gimple_stmt_iterator bsi;
+		      for (bsi = gsi_last_bb (return_bb); !gsi_end_p (bsi);
+			   gsi_prev (&bsi))
+			if (gimple_code (gsi_stmt (bsi)) == GIMPLE_RETURN)
+			  {
+			    retbnd = copy_ssa_name (retbnd, call);
+			    gimple_return_set_retbnd (gsi_stmt (bsi), retbnd);
+			    update_stmt (gsi_stmt (bsi));
+			    break;
+			  }
+		    }
 		}
 	      if (DECL_BY_REFERENCE (DECL_RESULT (current_function_decl)))
 		{
@@ -1450,6 +1537,9 @@ split_function (struct split_point *split_point)
 		      gsi_insert_after (&gsi, cpy, GSI_NEW_STMT);
 		      retval = tem;
 		    }
+		  /* Build bndret call to obtain returned bounds.  */
+		  if (retbnd)
+		    insert_bndret_call_after (retbnd, retval, &gsi);
 		  gimple_call_set_lhs (call, retval);
 		  update_stmt (call);
 		}
@@ -1468,6 +1558,10 @@ split_function (struct split_point *split_point)
 	    {
 	      retval = DECL_RESULT (current_function_decl);
 
+	      if (chkp_function_instrumented_p (current_function_decl)
+		  && BOUNDED_P (retval))
+		retbnd = create_tmp_reg (pointer_bounds_type_node, NULL);
+
 	      /* We use temporary register to hold value when aggregate_value_p
 		 is false.  Similarly for DECL_BY_REFERENCE we must avoid extra
 		 copy.  */
@@ -1491,6 +1585,9 @@ split_function (struct split_point *split_point)
 	        gimple_call_set_lhs (call, retval);
 	    }
           gsi_insert_after (&gsi, call, GSI_NEW_STMT);
+	  /* Build bndret call to obtain returned bounds.  */
+	  if (retbnd)
+	    insert_bndret_call_after (retbnd, retval, &gsi);
 	  ret = gimple_build_return (retval);
 	  gsi_insert_after (&gsi, ret, GSI_NEW_STMT);
 	}