[PATCH, Pointer Bounds Checker 19/x] Support bounds in expand

Jeff Law law@redhat.com
Tue Sep 23 20:58:00 GMT 2014


On 06/05/14 08:46, Ilya Enkovich wrote:
> 2014-06-05  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	* calls.c: Include tree-chkp.h, rtl-chkp.h, bitmap.h.
> 	(arg_data): Add fields special_slot, pointer_arg and
> 	pointer_offset.
> 	(store_bounds): New.
> 	(emit_call_1): Propagate instrumentation flag for CALL.
> 	(initialize_argument_information): Compute pointer_arg,
> 	pointer_offset and special_slot for pointer bounds arguments.
> 	(finalize_must_preallocate): Preallocate when storing bounds
> 	in bounds table.
> 	(compute_argument_addresses): Skip pointer bounds.
> 	(expand_call): Store bounds into tables separately.  Return
> 	result joined with resulting bounds.
> 	* cfgexpand.c: Include tree-chkp.h, rtl-chkp.h.
> 	(expand_call_stmt): Propagate bounds flag for CALL_EXPR.
> 	(expand_return): Add returned bounds arg.  Handle returned bounds.
> 	(expand_gimple_stmt_1): Adjust to new expand_return signature.
> 	(gimple_expand_cfg): Reset rtx bounds map.
> 	* expr.c: Include tree-chkp.h, rtl-chkp.h.
> 	(expand_assignment): Handle returned bounds.
> 	(store_expr_with_bounds): New.  Replaces store_expr with new bounds
> 	target argument.  Handle bounds returned by calls.
> 	(store_expr): Now wraps store_expr_with_bounds.
> 	* expr.h (store_expr_with_bounds): New.
> 	* function.c: Include tree-chkp.h, rtl-chkp.h.
> 	(bounds_parm_data): New.
> 	(use_register_for_decl): Do not registerize decls used for bounds
> 	stores and loads.
> 	(assign_parms_augmented_arg_list): Add bounds of the result
> 	structure pointer as the second argument.
> 	(assign_parm_find_entry_rtl): Mark bounds are never passed on
> 	the stack.
> 	(assign_parm_is_stack_parm): Likewise.
> 	(assign_parm_load_bounds): New.
> 	(assign_bounds): New.
> 	(assign_parms): Load bounds and determine a location for
> 	returned bounds.
> 	(diddle_return_value_1): New.
> 	(diddle_return_value): Handle returned bounds.
> 	* function.h (rtl_data): Add field for returned bounds.
>
>
> diff --git a/gcc/calls.c b/gcc/calls.c
> index e1dc8eb..5fbbe9f 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -44,11 +44,14 @@ along with GCC; see the file COPYING3.  If not see
>   #include "tm_p.h"
>   #include "timevar.h"
>   #include "sbitmap.h"
> +#include "bitmap.h"
>   #include "langhooks.h"
>   #include "target.h"
>   #include "cgraph.h"
>   #include "except.h"
>   #include "dbgcnt.h"
> +#include "tree-chkp.h"
> +#include "rtl-chkp.h"
>
>   /* Like PREFERRED_STACK_BOUNDARY but in units of bytes, not bits.  */
>   #define STACK_BYTES (PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT)
> @@ -76,6 +79,15 @@ struct arg_data
>     /* If REG is a PARALLEL, this is a copy of VALUE pulled into the correct
>        form for emit_group_move.  */
>     rtx parallel_value;
> +  /* If value is passed in neither reg nor stack, this field holds a number
> +     of a special slot to be used.  */
> +  rtx special_slot;
I really dislike "special_slot" and the comment here.  The comment that 
it's neither a reg nor stack is just bogus.  What hardware resource does 
"special_slot" refer to?  It's a register, but one that we do not 
typically expose.  Let's at least clarify the comment and then we'll see 
if something other than "special_slot" as a name makes sense.  Yes, I 
realize this is a bit of bikeshedding, but when the comments/terminology 
is confusing, the code becomes even harder to understand.

I'm a bit concerned that this is exposing more details of the MPX 
implementation than is advisable to the front/middle end.  On the other 
hand, I'd expect any other implementation that seeks to work in a 
transparent manner is going to have many of the same implementation 
properties as we see with MPX, so perhaps it's not a major problem.



> @@ -1141,18 +1158,84 @@ initialize_argument_information (int num_actuals ATTRIBUTE_UNUSED,
>     /* First fill in the actual arguments in the ARGS array, splitting
>        complex arguments if necessary.  */
>     {
> -    int j = i;
> +    int j = i, ptr_arg = -1;
>       call_expr_arg_iterator iter;
>       tree arg;
> +    bitmap slots = NULL;
>
>       if (struct_value_addr_value)
>         {
>   	args[j].tree_value = struct_value_addr_value;
> +
>   	j += inc;
> +
> +	/* If we pass structure address then we need to
> +	   create bounds for it.  Since created bounds is
> +	   a call statement, we expand it right here to avoid
> +	   fixing all other places where it may be expanded.  */
> +	if (CALL_WITH_BOUNDS_P (exp))
> +	  {
> +	    args[j].value = gen_reg_rtx (targetm.chkp_bound_mode ());
> +	    args[j].tree_value
> +	      = chkp_make_bounds_for_struct_addr (struct_value_addr_value);
> +	    expand_expr_real (args[j].tree_value, args[j].value, VOIDmode,
> +			      EXPAND_NORMAL, 0, false);
> +	    args[j].pointer_arg = j - inc;
> +
> +	    j += inc;
> +	  }
Just an FYI, I'm pretty sure this hunk isn't going to apply cleanly as 
the context has changed on the trunk.  I'd recommend getting this code 
updated for the trunk.  I suspect you're getting close to having all the 
basic functionality bits in, you're obviously going to need to do a new 
bootstrap & regression test prior to checkin.  I think git sqashing the 
series and testing/committing them as an atomic unit is probably wise.

It's been a while since I looked at this code, but is it safe to create 
a new call tree at this point?  I recall some major complications if you 
try to insert a call once you've started filling in arguments.  Hmm, 
gIven you're at the start of initialize_argument_information, you're 
probably OK since we haven't stored any arguments into their arg 
regs/memory yet.



> @@ -1302,6 +1388,12 @@ initialize_argument_information (int num_actuals ATTRIBUTE_UNUSED,
>         args[i].reg = targetm.calls.function_arg (args_so_far, mode, type,
>   						argpos < n_named_args);
>
> +      if (args[i].reg && CONST_INT_P (args[i].reg))
> +	{
> +	  args[i].special_slot = args[i].reg;
> +	  args[i].reg = NULL;
> +	}
I can't recall from the earlier patches, but have you updated the 
documentation to indicate that function_arg can return a CONST_INT?


I think this is mostly OK.  If you could update and resend for another 
once-over, it'd be appreciated.

Jeff



More information about the Gcc-patches mailing list