Re: [PATCH, MPX, 2/X] Pointers Checker [5/25] Tree and gimple ifaces

[Jeff Law <law at redhat dot com>]

On 10/30/13 04:48, Richard Biener wrote:
> foo (int * p, unsigned int size)
> {
>    <unnamed type> __bound_tmp.0;
>    long unsigned int D.2239;
>    long unsigned int _2;
>    sizetype _6;
>    int * _7;
>
>    <bb 3>:
>    __bound_tmp.0_4 = __builtin_ia32_arg_bnd (p_3(D));
>
>    <bb 2>:
>    _2 = (long unsigned int) size_1(D);
>    __builtin_ia32_bndcl (__bound_tmp.0_4, p_3(D));
>    _6 = _2 + 18446744073709551615;
>    _7 = p_3(D) + _6;
>    __builtin_ia32_bndcu (__bound_tmp.0_4, _7);
>    access_and_store (p_3(D), __bound_tmp.0_4, size_1(D));
>
> so it seems there is now a mismatch between DECL_ARGUMENTS
> and the GIMPLE call stmt arguments.  How (if) did you amend
> the GIMPLE stmt verifier for this?
Effectively the bounds are passed "on the side".

> How does regular code deal with this which may expect matching
> to DECL_ARGUMENTS?  In fact interleaving the additional
> arguments sounds very error-prone for existing code - I'd have
> appended all bound args at the end.  Also you unconditionally
> claim all pointer arguments have a bound - that looks like bad
> design as well.  Why didn't you add a flag to the relevant
> PARM_DECL (and then, what do you do for indirect calls?).
You can't actually interleave them -- that results in MPX and normal 
code not being able to interact.   Passing the bound at the end doesn't 
really work either -- varargs and the desire to pass some of the bounds 
around in bound registers.


> /* Return the number of arguments used by call statement GS
>     ignoring bound ones.  */
>
> static inline unsigned
> gimple_call_num_nobnd_args (const_gimple gs)
> {
>    unsigned num_args = gimple_call_num_args (gs);
>    unsigned res = num_args;
>    for (unsigned n = 0; n < num_args; n++)
>      if (POINTER_BOUNDS_P (gimple_call_arg (gs, n)))
>        res--;
>    return res;
> }
>
> the choice means that gimple_call_num_nobnd_args is not O(1).
Yes, but I don't see that's terribly problematical.


> /* Return INDEX's call argument ignoring bound ones.  */
> static inline tree
> gimple_call_nobnd_arg (const_gimple gs, unsigned index)
> {
>    /* No bound args may exist if pointers checker is off.  */
>    if (!flag_check_pointer_bounds)
>      return gimple_call_arg (gs, index);
>    return gimple_call_arg (gs, gimple_call_get_nobnd_arg_index (gs, index));
> }
>
> GIMPLE layout depending on flag_check_pointer_bounds sounds
> like a recipie for desaster if you consider TUs compiled with and
> TUs compiled without and LTO.  Or if you consider using
> optimized attribute with that flag.
Sorry, I don't follow.  Can you elaborate please.

> I hope the reviewers that approved the patch will work with you to
> address the above issues.  I can't be everywhere.
Obviously I will.

jeff