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.enkov...@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);
        }

Reply via email to