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);
}
