Re: [PATCH, Pointer Bounds Checker 23/x] Function split
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
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
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
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
2014-09-19 23:45 GMT+04:00 Jeff Law l...@redhat.com: On 09/16/14 03:09, Ilya Enkovich wrote: I must be misunderstanding something then. I fundamentally don't see how the return bounds are any different here than the return value. If we have exposed the bounds in the IL, then aren't they going to be handled just like any other object in the IL? They are not handled like any other object in IL because return block and all statements in it are not handled as all other statements we put into split part. Here is a comment from find_return_bb: /* Return basic block containing RETURN statement. We allow basic blocks of the form: retval = tmp_var; return retval but return_bb can not be more complex than this. ... */ Phi nodes also may present in return_bb. Right. I've seen this stuff, but it's still not clear to me what the real issue is. The first thing that jumps out when I look at your dump is we don't have a PHI for __bound_tmp.322 in BB6. Now it may be that we really just wanted __bound_tmp.322_36, but that seems wrong as the return value varies depending on how we reach BB6 and it seems to me the bounds ought to vary in a similar manner. 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. All blocks going to split part are analyzed by visit_bb function. Return basic block is not analyzed in the same way but still may be copied into split part in case return value is defined in it. There is a special code in visit_bb to add args of phi statements of return_bb as uses of split part to have no undefined values in copied block. It was enough when those phi args plus return value were only uses in return_bb. But now we add returned bounds to GIMPLE_RETURN as a new use and this new use is ignored. If split part returns value then return_bb will be copied into it. It means I should check returned bounds are defined there too. If SSA_NAME_DEF_STMT of returned bounds is in split part then it is OK. If SSA_NAME_DEF_STMT of returned bounds is in return_bb then it is also OK because it means it is a result of PHI node whose args were added as additional uses for split part earlier in visit_bb. At least that is how I think this happens :) Maybe you should post the IL for a case where this all matters and walk me through the key issues. 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. Thanks, Ilya Jeff
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
On 09/16/14 03:09, Ilya Enkovich wrote: I must be misunderstanding something then. I fundamentally don't see how the return bounds are any different here than the return value. If we have exposed the bounds in the IL, then aren't they going to be handled just like any other object in the IL? They are not handled like any other object in IL because return block and all statements in it are not handled as all other statements we put into split part. Here is a comment from find_return_bb: /* Return basic block containing RETURN statement. We allow basic blocks of the form: retval = tmp_var; return retval but return_bb can not be more complex than this. ... */ Phi nodes also may present in return_bb. Right. I've seen this stuff, but it's still not clear to me what the real issue is. The first thing that jumps out when I look at your dump is we don't have a PHI for __bound_tmp.322 in BB6. Now it may be that we really just wanted __bound_tmp.322_36, but that seems wrong as the return value varies depending on how we reach BB6 and it seems to me the bounds ought to vary in a similar manner. All blocks going to split part are analyzed by visit_bb function. Return basic block is not analyzed in the same way but still may be copied into split part in case return value is defined in it. There is a special code in visit_bb to add args of phi statements of return_bb as uses of split part to have no undefined values in copied block. It was enough when those phi args plus return value were only uses in return_bb. But now we add returned bounds to GIMPLE_RETURN as a new use and this new use is ignored. If split part returns value then return_bb will be copied into it. It means I should check returned bounds are defined there too. If SSA_NAME_DEF_STMT of returned bounds is in split part then it is OK. If SSA_NAME_DEF_STMT of returned bounds is in return_bb then it is also OK because it means it is a result of PHI node whose args were added as additional uses for split part earlier in visit_bb. At least that is how I think this happens :) Maybe you should post the IL for a case where this all matters and walk me through the key issues. 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. Jeff
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
2014-09-16 1:08 GMT+04:00 Jeff Law l...@redhat.com: On 09/15/14 10:20, Ilya Enkovich wrote: A problem I'm trying to avoid is that bounds in return statement are not taken into account when checking for data dependencies between parts. It means we may have a case when return statement with bounds is put into split part but bounds producer is not. If SSA_NAME_DEFSTMT for returned bounds is in the same partition as a return then I do not think I should care about the rest of definitions chain because regular split point checks should make sure we have everything required. Is the data dependency in the gimple IL? If so there shouldn't be anything particularly special we need to do. If not, then how ugly would it be to use the bounds at the return statement to expose the missing dependency? Not asking you to make that change, just want to make sure that I understand the core issue and that if something is missing from a dependency standpoint that we consider what it would take to expose the missing dependency. Gimple IL has required data dependencies to handle returns properly. But split pass handles return basic block in a special way. Return basic block has to have a simple form and is not scanned using stmt walkers as it is done for all other BBs by visit_bb. It is assumed that all dependencies for return BB are PHI args and returned value. Thus returned bounds are just not taken into account. That's how I see the problem. I must be misunderstanding something then. I fundamentally don't see how the return bounds are any different here than the return value. If we have exposed the bounds in the IL, then aren't they going to be handled just like any other object in the IL? They are not handled like any other object in IL because return block and all statements in it are not handled as all other statements we put into split part. Here is a comment from find_return_bb: /* Return basic block containing RETURN statement. We allow basic blocks of the form: retval = tmp_var; return retval but return_bb can not be more complex than this. ... */ Phi nodes also may present in return_bb. All blocks going to split part are analyzed by visit_bb function. Return basic block is not analyzed in the same way but still may be copied into split part in case return value is defined in it. There is a special code in visit_bb to add args of phi statements of return_bb as uses of split part to have no undefined values in copied block. It was enough when those phi args plus return value were only uses in return_bb. But now we add returned bounds to GIMPLE_RETURN as a new use and this new use is ignored. If split part returns value then return_bb will be copied into it. It means I should check returned bounds are defined there too. If SSA_NAME_DEF_STMT of returned bounds is in split part then it is OK. If SSA_NAME_DEF_STMT of returned bounds is in return_bb then it is also OK because it means it is a result of PHI node whose args were added as additional uses for split part earlier in visit_bb. At least that is how I think this happens :) Maybe you should post the IL for a case where this all matters and walk me through the key issues. 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. Thanks, Ilya jeff split.dump Description: Binary data
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
2014-09-03 23:12 GMT+04:00 Jeff Law l...@redhat.com: On 08/18/14 09:55, Ilya Enkovich wrote: On 04 Jun 01:15, Jeff Law wrote: On 06/03/14 01:10, Ilya Enkovich wrote: Hi, This patch does not allow splitting in case bounds are returned until retutrned bounds are supported. It also propagates instrumentation marks for generated call and function. Bootstrapped and tested on linux-x86_64. Thanks, Ilya -- gcc/ 2014-06-03 Ilya Enkovich ilya.enkov...@intel.com * ipa-split.c: Include tree-chkp.h. (consider_split): Do not split when return bounds. (split_function): Propagate Pointer Bounds Checker instrumentation marks. It's a hack. There's no reason we can't support this. So I'll approve on the condition that you do look at removing this limitation in the future. jeff I did some work for function splitting and now patch cover more cases. Now returned bounds are supported but it is not allowed to split producers of returned pointer and its bounds. Is it OK? Thanks, Ilya -- 2014-08-15 Ilya Enkovich ilya.enkov...@intel.com * ipa-split.c: Include tree-chkp.h. (find_retbnd): New. (consider_split): Do not split retbnd and retval producers. (split_function): Propagate Pointer Bounds Checker instrumentation marks and handle returned bounds. I don't think it's sufficient to just look at the SSA_NAME_DEFSTMT and verify that it's not in the header. You could easily have the SSA_NAME_DEF_STMT be a PHI which is in the same partition as the RETURN statement. One of the PHI arguments might be fed from a statement in the header, right? Don't you have to look at the entire set of definitions which directly and indirectly feed the return statement and verify that each and every one is in the same partition as the return statement? A problem I'm trying to avoid is that bounds in return statement are not taken into account when checking for data dependencies between parts. It means we may have a case when return statement with bounds is put into split part but bounds producer is not. If SSA_NAME_DEFSTMT for returned bounds is in the same partition as a return then I do not think I should care about the rest of definitions chain because regular split point checks should make sure we have everything required. And if so, that makes me start to think the original hack wasn't so bad after all :-) It's always nice to have a backup plan! :) Ilya jeff
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
On 09/15/14 03:51, Ilya Enkovich wrote: 2014-08-15 Ilya Enkovich ilya.enkov...@intel.com * ipa-split.c: Include tree-chkp.h. (find_retbnd): New. (consider_split): Do not split retbnd and retval producers. (split_function): Propagate Pointer Bounds Checker instrumentation marks and handle returned bounds. I don't think it's sufficient to just look at the SSA_NAME_DEFSTMT and verify that it's not in the header. You could easily have the SSA_NAME_DEF_STMT be a PHI which is in the same partition as the RETURN statement. One of the PHI arguments might be fed from a statement in the header, right? Don't you have to look at the entire set of definitions which directly and indirectly feed the return statement and verify that each and every one is in the same partition as the return statement? A problem I'm trying to avoid is that bounds in return statement are not taken into account when checking for data dependencies between parts. It means we may have a case when return statement with bounds is put into split part but bounds producer is not. If SSA_NAME_DEFSTMT for returned bounds is in the same partition as a return then I do not think I should care about the rest of definitions chain because regular split point checks should make sure we have everything required. Is the data dependency in the gimple IL? If so there shouldn't be anything particularly special we need to do. If not, then how ugly would it be to use the bounds at the return statement to expose the missing dependency? Not asking you to make that change, just want to make sure that I understand the core issue and that if something is missing from a dependency standpoint that we consider what it would take to expose the missing dependency. jeff
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
2014-09-15 19:39 GMT+04:00 Jeff Law l...@redhat.com: On 09/15/14 03:51, Ilya Enkovich wrote: 2014-08-15 Ilya Enkovich ilya.enkov...@intel.com * ipa-split.c: Include tree-chkp.h. (find_retbnd): New. (consider_split): Do not split retbnd and retval producers. (split_function): Propagate Pointer Bounds Checker instrumentation marks and handle returned bounds. I don't think it's sufficient to just look at the SSA_NAME_DEFSTMT and verify that it's not in the header. You could easily have the SSA_NAME_DEF_STMT be a PHI which is in the same partition as the RETURN statement. One of the PHI arguments might be fed from a statement in the header, right? Don't you have to look at the entire set of definitions which directly and indirectly feed the return statement and verify that each and every one is in the same partition as the return statement? A problem I'm trying to avoid is that bounds in return statement are not taken into account when checking for data dependencies between parts. It means we may have a case when return statement with bounds is put into split part but bounds producer is not. If SSA_NAME_DEFSTMT for returned bounds is in the same partition as a return then I do not think I should care about the rest of definitions chain because regular split point checks should make sure we have everything required. Is the data dependency in the gimple IL? If so there shouldn't be anything particularly special we need to do. If not, then how ugly would it be to use the bounds at the return statement to expose the missing dependency? Not asking you to make that change, just want to make sure that I understand the core issue and that if something is missing from a dependency standpoint that we consider what it would take to expose the missing dependency. Gimple IL has required data dependencies to handle returns properly. But split pass handles return basic block in a special way. Return basic block has to have a simple form and is not scanned using stmt walkers as it is done for all other BBs by visit_bb. It is assumed that all dependencies for return BB are PHI args and returned value. Thus returned bounds are just not taken into account. That's how I see the problem. Ilya jeff
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
On 09/15/14 10:20, Ilya Enkovich wrote: A problem I'm trying to avoid is that bounds in return statement are not taken into account when checking for data dependencies between parts. It means we may have a case when return statement with bounds is put into split part but bounds producer is not. If SSA_NAME_DEFSTMT for returned bounds is in the same partition as a return then I do not think I should care about the rest of definitions chain because regular split point checks should make sure we have everything required. Is the data dependency in the gimple IL? If so there shouldn't be anything particularly special we need to do. If not, then how ugly would it be to use the bounds at the return statement to expose the missing dependency? Not asking you to make that change, just want to make sure that I understand the core issue and that if something is missing from a dependency standpoint that we consider what it would take to expose the missing dependency. Gimple IL has required data dependencies to handle returns properly. But split pass handles return basic block in a special way. Return basic block has to have a simple form and is not scanned using stmt walkers as it is done for all other BBs by visit_bb. It is assumed that all dependencies for return BB are PHI args and returned value. Thus returned bounds are just not taken into account. That's how I see the problem. I must be misunderstanding something then. I fundamentally don't see how the return bounds are any different here than the return value. If we have exposed the bounds in the IL, then aren't they going to be handled just like any other object in the IL? Maybe you should post the IL for a case where this all matters and walk me through the key issues. jeff
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
On 08/18/14 09:55, Ilya Enkovich wrote: On 04 Jun 01:15, Jeff Law wrote: On 06/03/14 01:10, Ilya Enkovich wrote: Hi, This patch does not allow splitting in case bounds are returned until retutrned bounds are supported. It also propagates instrumentation marks for generated call and function. Bootstrapped and tested on linux-x86_64. Thanks, Ilya -- gcc/ 2014-06-03 Ilya Enkovich ilya.enkov...@intel.com * ipa-split.c: Include tree-chkp.h. (consider_split): Do not split when return bounds. (split_function): Propagate Pointer Bounds Checker instrumentation marks. It's a hack. There's no reason we can't support this. So I'll approve on the condition that you do look at removing this limitation in the future. jeff I did some work for function splitting and now patch cover more cases. Now returned bounds are supported but it is not allowed to split producers of returned pointer and its bounds. Is it OK? Thanks, Ilya -- 2014-08-15 Ilya Enkovich ilya.enkov...@intel.com * ipa-split.c: Include tree-chkp.h. (find_retbnd): New. (consider_split): Do not split retbnd and retval producers. (split_function): Propagate Pointer Bounds Checker instrumentation marks and handle returned bounds. I don't think it's sufficient to just look at the SSA_NAME_DEFSTMT and verify that it's not in the header. You could easily have the SSA_NAME_DEF_STMT be a PHI which is in the same partition as the RETURN statement. One of the PHI arguments might be fed from a statement in the header, right? Don't you have to look at the entire set of definitions which directly and indirectly feed the return statement and verify that each and every one is in the same partition as the return statement? And if so, that makes me start to think the original hack wasn't so bad after all :-) jeff
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
On 04 Jun 01:15, Jeff Law wrote:
On 06/03/14 01:10, Ilya Enkovich wrote:
Hi,
This patch does not allow splitting in case bounds are returned until
retutrned bounds are supported. It also propagates instrumentation marks
for generated call and function.
Bootstrapped and tested on linux-x86_64.
Thanks,
Ilya
--
gcc/
2014-06-03 Ilya Enkovich ilya.enkov...@intel.com
* ipa-split.c: Include tree-chkp.h.
(consider_split): Do not split when return bounds.
(split_function): Propagate Pointer Bounds Checker
instrumentation marks.
It's a hack. There's no reason we can't support this. So I'll
approve on the condition that you do look at removing this
limitation in the future.
jeff
I did some work for function splitting and now patch cover more cases. Now
returned bounds are supported but it is not allowed to split producers of
returned pointer and its bounds. Is it OK?
Thanks,
Ilya
--
2014-08-15 Ilya Enkovich ilya.enkov...@intel.com
* ipa-split.c: Include tree-chkp.h.
(find_retbnd): New.
(consider_split): Do not split retbnd and retval
producers.
(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..f8ecaf7 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. */
@@ -387,6 +389,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))
@@ -588,6 +591,32 @@ 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
+ = (!SSA_NAME_IS_DEFAULT_DEF (retbnd)
+ (bitmap_bit_p (current-split_bbs,
+gimple_bb (SSA_NAME_DEF_STMT (retbnd))-index)
+ || gimple_bb (SSA_NAME_DEF_STMT (retbnd)) == 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 +739,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_start_bb (return_bb); !gsi_end_p (bsi); gsi_next (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. */
@@ -1095,8 +1136,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 +1287,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
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
2014-06-04 11:15 GMT+04:00 Jeff Law l...@redhat.com: On 06/03/14 01:10, Ilya Enkovich wrote: Hi, This patch does not allow splitting in case bounds are returned until retutrned bounds are supported. It also propagates instrumentation marks for generated call and function. Bootstrapped and tested on linux-x86_64. Thanks, Ilya -- gcc/ 2014-06-03 Ilya Enkovich ilya.enkov...@intel.com * ipa-split.c: Include tree-chkp.h. (consider_split): Do not split when return bounds. (split_function): Propagate Pointer Bounds Checker instrumentation marks. It's a hack. There's no reason we can't support this. So I'll approve on the condition that you do look at removing this limitation in the future. I looked into it and did not find an easy way to fix damaged return. Will definitely work more on this later. Any help from knowledgeable person (Jan Hubicka?) would be great here. Ilya jeff
Re: [PATCH, Pointer Bounds Checker 23/x] Function split
On 06/03/14 01:10, Ilya Enkovich wrote: Hi, This patch does not allow splitting in case bounds are returned until retutrned bounds are supported. It also propagates instrumentation marks for generated call and function. Bootstrapped and tested on linux-x86_64. Thanks, Ilya -- gcc/ 2014-06-03 Ilya Enkovich ilya.enkov...@intel.com * ipa-split.c: Include tree-chkp.h. (consider_split): Do not split when return bounds. (split_function): Propagate Pointer Bounds Checker instrumentation marks. It's a hack. There's no reason we can't support this. So I'll approve on the condition that you do look at removing this limitation in the future. jeff
[PATCH, Pointer Bounds Checker 23/x] Function split
Hi,
This patch does not allow splitting in case bounds are returned until retutrned
bounds are supported. It also propagates instrumentation marks for generated
call and function.
Bootstrapped and tested on linux-x86_64.
Thanks,
Ilya
--
gcc/
2014-06-03 Ilya Enkovich ilya.enkov...@intel.com
* ipa-split.c: Include tree-chkp.h.
(consider_split): Do not split when return bounds.
(split_function): Propagate Pointer Bounds Checker
instrumentation marks.
diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index 38bd883..edf322f 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. */
@@ -496,6 +497,19 @@ consider_split (struct split_point *current, bitmap
non_ssa_vars,
if (!VOID_TYPE_P (TREE_TYPE (current_function_decl)))
call_overhead += estimate_move_cost (TREE_TYPE (current_function_decl));
+ /* Currently returned value is processed but returned bounds
+ are not processed. It results in bounds in return statement
+ with no definition. Forbid split until returned bounds are
+ supported. */
+ if (chkp_function_instrumented_p (current_function_decl)
+ chkp_type_has_pointer (TREE_TYPE (TREE_TYPE (current_function_decl
+{
+ if (dump_file (dump_flags TDF_DETAILS))
+ fprintf (dump_file,
+ Refused: need to return bounds\n);
+ return;
+}
+
if (current-split_size = call_overhead)
{
if (dump_file (dump_flags TDF_DETAILS))
@@ -1096,6 +1110,7 @@ split_function (struct split_point *split_point)
edge_iterator ei;
tree retval = NULL, real_retval = 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;
@@ -1248,6 +1263,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;
@@ -1282,6 +1303,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 ();
