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

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2014-09-24 0:58 GMT+04:00 Jeff Law <law@redhat.com>:
> 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.

Special slot is not a register.  When bounds are passed in a register
then everything work as if we pass any other argument in a register.
Special slot is used when we are out of bounds registers and pass
bounds for pointer passed in a register.  It doesn't refer to any
hardware resource.  In MPX ABI we state that special Bounds Table
entries (related to stack pointer value (and lower) right before a
call) are used.  In software implementation it also may be some other
places like vars in TLS.

>
> 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.

I'm trying to not introduce any hardware dependencies into middle end.
Several months ago I created a simple prototype of generic target
support in Pointer Bounds Checker which used library calls instead of
MPX instructions, TLS for bounds passing etc.  I did it to check our
design is not bound to MPX and allows such implementation.  It was
very useful and showed some MPX details soaked into GIMPLE part. E.g.
chkp_initialize_bounds and chkp_make_bounds_constant hooks appeared
during that work.  Special slots mechanism worked well for it though.

>
>
>
>
>> @@ -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.

There are also some other patches which require update.  I'll do it
and repost modified patches.

>
> 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.

It should be quite safe to expand another call until we start moving
arguments to their actual places.

>
>
>
>> @@ -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 didn't update it. Will do.

Thanks,
Ilya

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