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

[Ilya Enkovich <enkovich.gnu@gmail.com>]

2013/10/30 Richard Biener <richard.guenther@gmail.com>:
> On Wed, Oct 30, 2013 at 11:34 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> On 30 Oct 10:26, Richard Biener wrote:
>>>
>>> Ick - you enlarge all return statements?  But you don't set the actual value?
>>> So why allocate it with 2 ops in the first place??
>>
>> When return does not return bounds it has operand with zero value similar to case when it does not return value. What is the difference then?
>>
>>>
>>> [Seems I completely missed that MPX changes "gimple" and the design
>>> document that was posted somewhere??]
>>>
>>
>> Design is on GCC Wiki and link was posted few times: http://gcc.gnu.org/wiki/Intel%20MPX%20support%20in%20the%20GCC%20compiler. Here is quote about return statements:
>>
>> Returns instrumentation. We add new operand to return statement to hold returned bounds and instrumentation pass is responsible to fill this operand with correct bounds
>
> 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?

Verifier just ignores bound arguments while iterating through them.

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

I'll consider using another layout for bound args. But why should we
have any PARM_DECL or other pointer not having bounds?
>
> /* 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. And even having all bound args at the end would not fix it. We
have to additionally keep number of bounds if want to fix it. But I do
not see the strong reason for that. Currently there are just three
calls to this function in whole GCC.

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

Yep. Marking instrumented calls and functions would be useful in LTO case.

>
> I wish I had seen all this before.
>
>>> Bah.
>>>
>>> Where is the update to gimple.texi and tree.texi?
>>>
>>> Richard.
>>>
>>
>> Unfortunately patch has been already installed.  Should we uninstall it?  If not, then here is patch for documentation.
>
> I hope the reviewers that approved the patch will work with you to
> address the above issues.  I can't be everywhere.

Thanks for valuable input!

Ilya

>
> Richard.
>
>> Thanks,
>> Ilya
>> --
>>
>> gcc/
>>
>> 2013-10-30  Ilya Enkovich  <ilya.enkovich@intel.com>
>>
>>         * doc/gimple.texi (gimple_call_num_nobnd_args): New.
>>         (gimple_call_nobnd_arg): New.
>>         (gimple_return_retbnd): New.
>>         (gimple_return_set_retbnd): New.
>>         (gimple_call_get_nobnd_arg_index): New.
>>
>>
>> diff --git a/gcc/doc/gimple.texi b/gcc/doc/gimple.texi
>> index 7bd9fd5..be74170 100644
>> --- a/gcc/doc/gimple.texi
>> +++ b/gcc/doc/gimple.texi
>> @@ -1240,11 +1240,25 @@ Set @code{CHAIN} to be the static chain for call statement @code{G}.
>>  Return the number of arguments used by call statement @code{G}.
>>  @end deftypefn
>>
>> +@deftypefn {GIMPLE function} unsigned gimple_call_num_nobnd_args (gimple g)
>> +Return the number of arguments used by call statement @code{G}
>> +ignoring bound ones.
>> +@end deftypefn
>> +
>>  @deftypefn {GIMPLE function} tree gimple_call_arg (gimple g, unsigned index)
>>  Return the argument at position @code{INDEX} for call statement @code{G}.  The
>>  first argument is 0.
>>  @end deftypefn
>>
>> +@deftypefn {GIMPLE function} tree gimple_call_nobnd_arg (gimple g, unsigned index)
>> +Return the argument at position @code{INDEX} for call statement @code{G}
>> +ignoring bound ones.  The first argument is 0.
>> +@end deftypefn
>> +
>> +@deftypefn {GIMPLE function} unsigned gimple_call_get_nobnd_arg_index (gimple g, unsigned index)
>> +Return index of @code{INDEX}'s non bound argument of the call statement @code{G}
>> +@end deftypefn
>> +
>>  @deftypefn {GIMPLE function} {tree *} gimple_call_arg_ptr (gimple g, unsigned index)
>>  Return a pointer to the argument at position @code{INDEX} for call
>>  statement @code{G}.
>> @@ -2029,6 +2043,15 @@ Return the return value for @code{GIMPLE_RETURN} @code{G}.
>>  Set @code{RETVAL} to be the return value for @code{GIMPLE_RETURN} @code{G}.
>>  @end deftypefn
>>
>> +@deftypefn {GIMPLE function} tree gimple_return_retbnd (gimple g)
>> +Return the bounds of return value for @code{GIMPLE_RETURN} @code{G}.
>> +@end deftypefn
>> +
>> +@deftypefn {GIMPLE function} void gimple_return_set_retbnd (gimple g, tree retbnd)
>> +Set @code{RETBND} to be the bounds of return value for @code{GIMPLE_RETURN}
>> +@code{G}.
>> +@end deftypefn
>> +
>>  @node @code{GIMPLE_SWITCH}
>>  @subsection @code{GIMPLE_SWITCH}
>>  @cindex @code{GIMPLE_SWITCH}