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