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

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Oct 30, 2013 at 11:48 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> 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?
>
> 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?).
>
> /* 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).
>
> /* 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.

Btw, we have this kind of mess pre-existing with stuff like flag_wrapv
and flag_trapv, adding a new case should be a 100% no-go.  If you
really need to have this conditional per call then add a flag on
CALL_EXPR and GIMPLE_CALL saying this call has bound arguments
and return value.  And append the bound arguments at the end.

Richard.

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