2014-09-24 0:58 GMT+04:00 Jeff Law <l...@redhat.com>:
> On 06/05/14 08:46, Ilya Enkovich wrote:
>>
>> 2014-06-05 Ilya Enkovich <ilya.enkov...@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
