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

Jeff Law law@redhat.com
Tue Sep 23 15:55:00 GMT 2014


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




More information about the Gcc-patches mailing list