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

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

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

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.

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.

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}

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}

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}

On 10/30/13 04:34, Ilya Enkovich 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?
In general, when someone proposes a change in the size of tree, rtl or 
gimple nodes, it's a "yellow flag" that something may need further 
investigation.

In this specific instance, I could trivially predict how that additional 
field would be used and a GIMPLE_RETURN isn't terribly important from a 
size standpoint, so I didn't call it out.


> 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
Exactly what I expected.

>
> Unfortunately patch has been already installed.  Should we uninstall
> it?  If not, then here is patch for documentation.
I think we're OK for now.  If Richi wants it out, he'll say so explicitly.


>
> 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.
Can you also fixup the GIMPLE_RETURN documentation in gimple.texi.  It 
needs a minor update after these changes.

jeff

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

On 30 Oct 11:40, Jeff Law wrote:
> 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 suppose the possile problem here is when we run LTO compiler without -fcheck-pointer-bounds and give instrumented code as input. gimple_call_nobnd_arg would work wrong for instrumented code. Actually there are other places in subsequent patches wich assume that flag_check_pointer_bounds is 1 if we have instrumented code. 

Ilya

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

2013/10/30 Jeff Law <law@redhat.com>:
> On 10/30/13 04:34, Ilya Enkovich 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?
>
> In general, when someone proposes a change in the size of tree, rtl or
> gimple nodes, it's a "yellow flag" that something may need further
> investigation.
>
> In this specific instance, I could trivially predict how that additional
> field would be used and a GIMPLE_RETURN isn't terribly important from a size
> standpoint, so I didn't call it out.
>
>
>
>> 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
>
> Exactly what I expected.
>
>
>>
>> Unfortunately patch has been already installed.  Should we uninstall
>> it?  If not, then here is patch for documentation.
>
> I think we're OK for now.  If Richi wants it out, he'll say so explicitly.
>
>
>
>>
>> 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.
>
> Can you also fixup the GIMPLE_RETURN documentation in gimple.texi.  It needs
> a minor update after these changes.

I could not find anything but accessors for GIMPLE_RETURN in
gimple.texi. And new accessors are in my doc patch already.

Ilya
>
> jeff
>

On 10/30/13 12:12, Ilya Enkovich wrote:

>>> 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 suppose the possile problem here is when we run LTO compiler
> without -fcheck-pointer-bounds and give instrumented code as input.
> gimple_call_nobnd_arg would work wrong for instrumented code.
> Actually there are other places in subsequent patches wich assume
> that flag_check_pointer_bounds is 1 if we have instrumented code.
OK, I can see how that would be problematical.  I'm not entirely sure 
how you're going to avoid that problem with the argument passing scheme 
you've built.

At the least,  I think an error message would be appropriate if you 
encounter instrumented code and -fcheck-pointer-bounds isn't on.


Jeff

On Wed, Oct 30, 2013 at 5:20 PM, Jeff Law <law@redhat.com> wrote:
> On 10/30/13 04:34, Ilya Enkovich 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?
>
> In general, when someone proposes a change in the size of tree, rtl or
> gimple nodes, it's a "yellow flag" that something may need further
> investigation.
>
> In this specific instance, I could trivially predict how that additional
> field would be used and a GIMPLE_RETURN isn't terribly important from a size
> standpoint, so I didn't call it out.

Btw, both for regular return with no value and this case only required
operands could be allocated.  There may be legacy issues with
the regular return value but for new uses always allocating the
operand seems wrong (even if "cheap" in practice).

Richard.

>> 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
>
> Exactly what I expected.
>
>
>>
>> Unfortunately patch has been already installed.  Should we uninstall
>> it?  If not, then here is patch for documentation.
>
> I think we're OK for now.  If Richi wants it out, he'll say so explicitly.
>
>
>
>>
>> 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.
>
> Can you also fixup the GIMPLE_RETURN documentation in gimple.texi.  It needs
> a minor update after these changes.
>
> jeff
>

On Wed, Oct 30, 2013 at 6:40 PM, Jeff Law <law@redhat.com> wrote:
> 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".

Well, not exactly.  I can see __bound_tmp.0_4 passed to access_and_store.

Are __builtin_ia32_bndcu and access_and_store supposed to be
called directly after each other?  What if for example profile instrumentation
inserts a call inbetween 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?).
>
> 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.

I'm only looking at how bound arguments are passed at the GIMPLE
level - which I think is arbitrary given they are passed on-the-side
during code-generation.  So I'm looking for a more "sensible" option
for the GIMPLE representation which looks somewhat fragile to me
(it looks the argument is only there to have a data dependence
between the bndcu intrinsic and the actual call?)

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

Well, consider compile.exp limits-fnargs.c written with int * parameters
and bound support ;)  [there is a limit on the number of bound registers
available, but the GIMPLE parts doesn't put any limit on instrumented
args?]

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

Compile TU1 with -fno-chk, TU2 with -fchk, LTO both object files
which gives you - well, either -fno-chk or -fchk.  Then you read in
the GIMPLE and when calling gimple_call_nobnd_arg you get
a mismatch between where you generated the gimple and where
you use the gimple.

A flag on whether a call is instrumented is better (consider inlining
from TU1 into TU2 where a flag on struct function for example wouldn't
be enough either).

Richard.

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

On 11/04/13 05:40, Richard Biener wrote:
>>
>> Effectively the bounds are passed "on the side".
>
> Well, not exactly.  I can see __bound_tmp.0_4 passed to access_and_store.
I'm referring to how they're dealt with in FUNCTION_ARG and friends, ie, 
the low level aspects.  Maybe that's why we're crossing wires here.


>
> Are __builtin_ia32_bndcu and access_and_store supposed to be
> called directly after each other?  What if for example profile instrumentation
> inserts a call inbetween them?
That's a question for Ilya.

>>
>> 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.
>
> I'm only looking at how bound arguments are passed at the GIMPLE
> level - which I think is arbitrary given they are passed on-the-side
> during code-generation.  So I'm looking for a more "sensible" option
> for the GIMPLE representation which looks somewhat fragile to me
> (it looks the argument is only there to have a data dependence
> between the bndcu intrinsic and the actual call?)
OK, we're clearly looking at two different aspects here.  While I think 
we can express them arbitrarily in GIMPLE, we have to pass them on the 
side once we get into the low level code for compatibility purposes.

I think argument passing for MPX is going to ultimately be ugly/fragile 
regardless of what we do given the constraints.  Given we're passing on 
the side at the low level, I'd prefer them passed on the side in GIMPLE, 
but I'm willing to consider other alternatives.

>
>>>
>>> /* 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.
>
> Well, consider compile.exp limits-fnargs.c written with int * parameters
> and bound support ;)  [there is a limit on the number of bound registers
> available, but the GIMPLE parts doesn't put any limit on instrumented
> args?]
Right.  Go back to Ilya's earlier messages :-)

There's these two magic hooks TARGET_{LOAD,STORE}_BOUNDS_FOR_ARG.  At 
some point when you run out of regs, you start shoving these things into 
memory.  I think we're ultimately going to need further clarification here.

>>>
>>> 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.
>
> Compile TU1 with -fno-chk, TU2 with -fchk, LTO both object files
> which gives you - well, either -fno-chk or -fchk.  Then you read in
> the GIMPLE and when calling gimple_call_nobnd_arg you get
> a mismatch between where you generated the gimple and where
> you use the gimple.
>
> A flag on whether a call is instrumented is better (consider inlining
> from TU1 into TU2 where a flag on struct function for example wouldn't
> be enough either).
OK.  I see what you're saying.  Whether or not we have these extra args 
is a property of the call site, not the CFUN, target DECL or the TU. 
That makes sense.


Ilya, I think there's enough confusion & issues around the call ABI/API 
that we need to sort it out before moving forward in any significant way.

Let's go back to argument passing and go through some concrete examples 
of the various cases.  Make sure to tie them into the calls to 
TARGET_{LOAD,STORE}_BOUNDS_FOR_ARG and the new gimple_call_* routines.

Jeff

2013/11/7 Jeff Law <law@redhat.com>:
> On 11/04/13 05:40, Richard Biener wrote:
>>>
>>>
>>> Effectively the bounds are passed "on the side".
>>
>>
>> Well, not exactly.  I can see __bound_tmp.0_4 passed to access_and_store.
>
> I'm referring to how they're dealt with in FUNCTION_ARG and friends, ie, the
> low level aspects.  Maybe that's why we're crossing wires here.
>
>
>
>>
>> Are __builtin_ia32_bndcu and access_and_store supposed to be
>> called directly after each other?  What if for example profile
>> instrumentation
>> inserts a call inbetween them?
>
> That's a question for Ilya.

I think there is some misunderstanding. __builtin_ia32_bndcu and
access_and_store calls are completely independent in this example.
This is just a code from example on how user-level builtins may be
used with legacy code.

>
>
>>>
>>> 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.
>>
>>
>> I'm only looking at how bound arguments are passed at the GIMPLE
>> level - which I think is arbitrary given they are passed on-the-side
>> during code-generation.  So I'm looking for a more "sensible" option
>> for the GIMPLE representation which looks somewhat fragile to me
>> (it looks the argument is only there to have a data dependence
>> between the bndcu intrinsic and the actual call?)
>
> OK, we're clearly looking at two different aspects here.  While I think we
> can express them arbitrarily in GIMPLE, we have to pass them on the side
> once we get into the low level code for compatibility purposes.
>
> I think argument passing for MPX is going to ultimately be ugly/fragile
> regardless of what we do given the constraints.  Given we're passing on the
> side at the low level, I'd prefer them passed on the side in GIMPLE, but I'm
> willing to consider other alternatives.
>
>
>>
>>>>
>>>> /* 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.
>>
>>
>> Well, consider compile.exp limits-fnargs.c written with int * parameters
>> and bound support ;)  [there is a limit on the number of bound registers
>> available, but the GIMPLE parts doesn't put any limit on instrumented
>> args?]
>
> Right.  Go back to Ilya's earlier messages :-)
>
> There's these two magic hooks TARGET_{LOAD,STORE}_BOUNDS_FOR_ARG.  At some
> point when you run out of regs, you start shoving these things into memory.
> I think we're ultimately going to need further clarification here.
>
>
>>>>
>>>> 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.
>>
>>
>> Compile TU1 with -fno-chk, TU2 with -fchk, LTO both object files
>> which gives you - well, either -fno-chk or -fchk.  Then you read in
>> the GIMPLE and when calling gimple_call_nobnd_arg you get
>> a mismatch between where you generated the gimple and where
>> you use the gimple.
>>
>> A flag on whether a call is instrumented is better (consider inlining
>> from TU1 into TU2 where a flag on struct function for example wouldn't
>> be enough either).
>
> OK.  I see what you're saying.  Whether or not we have these extra args is a
> property of the call site, not the CFUN, target DECL or the TU. That makes
> sense.
>
>
> Ilya, I think there's enough confusion & issues around the call ABI/API that
> we need to sort it out before moving forward in any significant way.
>
> Let's go back to argument passing and go through some concrete examples of
> the various cases.  Make sure to tie them into the calls to
> TARGET_{LOAD,STORE}_BOUNDS_FOR_ARG and the new gimple_call_* routines.

OK. Lets see at how expand pass handles instrumented calls.  As usual,
for each passed argument expand calls target hook to determine where
argument is passed.  Now target may return not only slot for arg, but
also a slot for bounds in case arg may have them (PARALLEL expr is
used to hold all slots).  If bounds slot is register, then expand
simply puts bounds to it.  If it is not a register, target hook is
called to store bounds (TARGET_STORE_BOUNDS_FOR_ARG).  Incoming bounds
are handled in a similar way but with usage of
TARGET_LOAD_BOUNDS_FOR_ARG hook.

The only remained problem for expand here is to identify which bounds
should be passed for arg.  To tie bounds to argument I put all bounds
passed for arg (in case arg is a structure, we may have multiple
bounds) as additional arguments in the call.  Each regular argument is
immediately followed by all bounds passed for it. For example:

test (&"Hello world!"[0], 0);

is translated into

test (&"Hello world!"[0], __bound_tmp.0_1, 0);

where __bounds_tmp.0_1 - bounds of passed string.

Of course, such call modification has some implications.  Some
optimizations may rely on number of arguments in the call (e,g, strlen
pass) and indexes of arguments.  Also now index of arg in fndecl does
not match the index of actual arg in the call.  For such cases new
functions are introduced to get args by it's original index, like
there is no instrumentation.  BTW there are not many usages of these
new functions and almost all of them are in strlen pass.

Also some changes, like the patch I sent for calls modifications and
copy, are required.  I suppose such changes are quite simple. and not
very wide.

Regarding proposal to put all bounds to the end of call's arg list - I
do not think it makes life much easier.  It still requires
modifications in verifier (because it checks arg count), calls copy
etc.  It becomes more complex to identify bounds of arg.  Also GIMPLE
dump for calls becomes less informative.  Surely it allows to get rid
of gimple_call_nobnd_arg and gimple_call_get_nobnd_arg_index but will
require additional interface functions for bound args.

Ilya

>
> Jeff

On Thu, Nov 7, 2013 at 10:07 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2013/11/7 Jeff Law <law@redhat.com>:
>> On 11/04/13 05:40, Richard Biener wrote:
>>>>
>>>>
>>>> Effectively the bounds are passed "on the side".
>>>
>>>
>>> Well, not exactly.  I can see __bound_tmp.0_4 passed to access_and_store.
>>
>> I'm referring to how they're dealt with in FUNCTION_ARG and friends, ie, the
>> low level aspects.  Maybe that's why we're crossing wires here.
>>
>>
>>
>>>
>>> Are __builtin_ia32_bndcu and access_and_store supposed to be
>>> called directly after each other?  What if for example profile
>>> instrumentation
>>> inserts a call inbetween them?
>>
>> That's a question for Ilya.
>
> I think there is some misunderstanding. __builtin_ia32_bndcu and
> access_and_store calls are completely independent in this example.
> This is just a code from example on how user-level builtins may be
> used with legacy code.
>
>>
>>
>>>>
>>>> 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.
>>>
>>>
>>> I'm only looking at how bound arguments are passed at the GIMPLE
>>> level - which I think is arbitrary given they are passed on-the-side
>>> during code-generation.  So I'm looking for a more "sensible" option
>>> for the GIMPLE representation which looks somewhat fragile to me
>>> (it looks the argument is only there to have a data dependence
>>> between the bndcu intrinsic and the actual call?)
>>
>> OK, we're clearly looking at two different aspects here.  While I think we
>> can express them arbitrarily in GIMPLE, we have to pass them on the side
>> once we get into the low level code for compatibility purposes.
>>
>> I think argument passing for MPX is going to ultimately be ugly/fragile
>> regardless of what we do given the constraints.  Given we're passing on the
>> side at the low level, I'd prefer them passed on the side in GIMPLE, but I'm
>> willing to consider other alternatives.
>>
>>
>>>
>>>>>
>>>>> /* 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.
>>>
>>>
>>> Well, consider compile.exp limits-fnargs.c written with int * parameters
>>> and bound support ;)  [there is a limit on the number of bound registers
>>> available, but the GIMPLE parts doesn't put any limit on instrumented
>>> args?]
>>
>> Right.  Go back to Ilya's earlier messages :-)
>>
>> There's these two magic hooks TARGET_{LOAD,STORE}_BOUNDS_FOR_ARG.  At some
>> point when you run out of regs, you start shoving these things into memory.
>> I think we're ultimately going to need further clarification here.
>>
>>
>>>>>
>>>>> 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.
>>>
>>>
>>> Compile TU1 with -fno-chk, TU2 with -fchk, LTO both object files
>>> which gives you - well, either -fno-chk or -fchk.  Then you read in
>>> the GIMPLE and when calling gimple_call_nobnd_arg you get
>>> a mismatch between where you generated the gimple and where
>>> you use the gimple.
>>>
>>> A flag on whether a call is instrumented is better (consider inlining
>>> from TU1 into TU2 where a flag on struct function for example wouldn't
>>> be enough either).
>>
>> OK.  I see what you're saying.  Whether or not we have these extra args is a
>> property of the call site, not the CFUN, target DECL or the TU. That makes
>> sense.
>>
>>
>> Ilya, I think there's enough confusion & issues around the call ABI/API that
>> we need to sort it out before moving forward in any significant way.
>>
>> Let's go back to argument passing and go through some concrete examples of
>> the various cases.  Make sure to tie them into the calls to
>> TARGET_{LOAD,STORE}_BOUNDS_FOR_ARG and the new gimple_call_* routines.
>
> OK. Lets see at how expand pass handles instrumented calls.  As usual,
> for each passed argument expand calls target hook to determine where
> argument is passed.  Now target may return not only slot for arg, but
> also a slot for bounds in case arg may have them (PARALLEL expr is
> used to hold all slots).  If bounds slot is register, then expand
> simply puts bounds to it.  If it is not a register, target hook is
> called to store bounds (TARGET_STORE_BOUNDS_FOR_ARG).  Incoming bounds
> are handled in a similar way but with usage of
> TARGET_LOAD_BOUNDS_FOR_ARG hook.
>
> The only remained problem for expand here is to identify which bounds
> should be passed for arg.  To tie bounds to argument I put all bounds
> passed for arg (in case arg is a structure, we may have multiple
> bounds) as additional arguments in the call.  Each regular argument is
> immediately followed by all bounds passed for it. For example:
>
> test (&"Hello world!"[0], 0);
>
> is translated into
>
> test (&"Hello world!"[0], __bound_tmp.0_1, 0);
>
> where __bounds_tmp.0_1 - bounds of passed string.
>
> Of course, such call modification has some implications.  Some
> optimizations may rely on number of arguments in the call (e,g, strlen
> pass) and indexes of arguments.  Also now index of arg in fndecl does
> not match the index of actual arg in the call.  For such cases new
> functions are introduced to get args by it's original index, like
> there is no instrumentation.  BTW there are not many usages of these
> new functions and almost all of them are in strlen pass.
>
> Also some changes, like the patch I sent for calls modifications and
> copy, are required.  I suppose such changes are quite simple. and not
> very wide.
>
> Regarding proposal to put all bounds to the end of call's arg list - I
> do not think it makes life much easier.  It still requires
> modifications in verifier (because it checks arg count), calls copy
> etc.  It becomes more complex to identify bounds of arg.  Also GIMPLE
> dump for calls becomes less informative.  Surely it allows to get rid
> of gimple_call_nobnd_arg and gimple_call_get_nobnd_arg_index but will
> require additional interface functions for bound args.

What about passing bounds together with the value in the same slot?
Like via

  arg = __builtin_tie_arg_and_bound (arg, bound);
  foo (arg);

you say you can have bounds for aggregates, do you mean

  struct X { int *x; int *y; };
  foo (struct X);

the ABI says for a call to foo () you pass two bound arguments?
Or do you merely mean the degenerate case struct X { int *x; }?

Richard.

> Ilya
>
>>
>> Jeff

2013/11/11 Richard Biener <richard.guenther@gmail.com>:
> On Thu, Nov 7, 2013 at 10:07 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2013/11/7 Jeff Law <law@redhat.com>:
>>> On 11/04/13 05:40, Richard Biener wrote:
>>>>>
>>>>>
>>>>> Effectively the bounds are passed "on the side".
>>>>
>>>>
>>>> Well, not exactly.  I can see __bound_tmp.0_4 passed to access_and_store.
>>>
>>> I'm referring to how they're dealt with in FUNCTION_ARG and friends, ie, the
>>> low level aspects.  Maybe that's why we're crossing wires here.
>>>
>>>
>>>
>>>>
>>>> Are __builtin_ia32_bndcu and access_and_store supposed to be
>>>> called directly after each other?  What if for example profile
>>>> instrumentation
>>>> inserts a call inbetween them?
>>>
>>> That's a question for Ilya.
>>
>> I think there is some misunderstanding. __builtin_ia32_bndcu and
>> access_and_store calls are completely independent in this example.
>> This is just a code from example on how user-level builtins may be
>> used with legacy code.
>>
>>>
>>>
>>>>>
>>>>> 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.
>>>>
>>>>
>>>> I'm only looking at how bound arguments are passed at the GIMPLE
>>>> level - which I think is arbitrary given they are passed on-the-side
>>>> during code-generation.  So I'm looking for a more "sensible" option
>>>> for the GIMPLE representation which looks somewhat fragile to me
>>>> (it looks the argument is only there to have a data dependence
>>>> between the bndcu intrinsic and the actual call?)
>>>
>>> OK, we're clearly looking at two different aspects here.  While I think we
>>> can express them arbitrarily in GIMPLE, we have to pass them on the side
>>> once we get into the low level code for compatibility purposes.
>>>
>>> I think argument passing for MPX is going to ultimately be ugly/fragile
>>> regardless of what we do given the constraints.  Given we're passing on the
>>> side at the low level, I'd prefer them passed on the side in GIMPLE, but I'm
>>> willing to consider other alternatives.
>>>
>>>
>>>>
>>>>>>
>>>>>> /* 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.
>>>>
>>>>
>>>> Well, consider compile.exp limits-fnargs.c written with int * parameters
>>>> and bound support ;)  [there is a limit on the number of bound registers
>>>> available, but the GIMPLE parts doesn't put any limit on instrumented
>>>> args?]
>>>
>>> Right.  Go back to Ilya's earlier messages :-)
>>>
>>> There's these two magic hooks TARGET_{LOAD,STORE}_BOUNDS_FOR_ARG.  At some
>>> point when you run out of regs, you start shoving these things into memory.
>>> I think we're ultimately going to need further clarification here.
>>>
>>>
>>>>>>
>>>>>> 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.
>>>>
>>>>
>>>> Compile TU1 with -fno-chk, TU2 with -fchk, LTO both object files
>>>> which gives you - well, either -fno-chk or -fchk.  Then you read in
>>>> the GIMPLE and when calling gimple_call_nobnd_arg you get
>>>> a mismatch between where you generated the gimple and where
>>>> you use the gimple.
>>>>
>>>> A flag on whether a call is instrumented is better (consider inlining
>>>> from TU1 into TU2 where a flag on struct function for example wouldn't
>>>> be enough either).
>>>
>>> OK.  I see what you're saying.  Whether or not we have these extra args is a
>>> property of the call site, not the CFUN, target DECL or the TU. That makes
>>> sense.
>>>
>>>
>>> Ilya, I think there's enough confusion & issues around the call ABI/API that
>>> we need to sort it out before moving forward in any significant way.
>>>
>>> Let's go back to argument passing and go through some concrete examples of
>>> the various cases.  Make sure to tie them into the calls to
>>> TARGET_{LOAD,STORE}_BOUNDS_FOR_ARG and the new gimple_call_* routines.
>>
>> OK. Lets see at how expand pass handles instrumented calls.  As usual,
>> for each passed argument expand calls target hook to determine where
>> argument is passed.  Now target may return not only slot for arg, but
>> also a slot for bounds in case arg may have them (PARALLEL expr is
>> used to hold all slots).  If bounds slot is register, then expand
>> simply puts bounds to it.  If it is not a register, target hook is
>> called to store bounds (TARGET_STORE_BOUNDS_FOR_ARG).  Incoming bounds
>> are handled in a similar way but with usage of
>> TARGET_LOAD_BOUNDS_FOR_ARG hook.
>>
>> The only remained problem for expand here is to identify which bounds
>> should be passed for arg.  To tie bounds to argument I put all bounds
>> passed for arg (in case arg is a structure, we may have multiple
>> bounds) as additional arguments in the call.  Each regular argument is
>> immediately followed by all bounds passed for it. For example:
>>
>> test (&"Hello world!"[0], 0);
>>
>> is translated into
>>
>> test (&"Hello world!"[0], __bound_tmp.0_1, 0);
>>
>> where __bounds_tmp.0_1 - bounds of passed string.
>>
>> Of course, such call modification has some implications.  Some
>> optimizations may rely on number of arguments in the call (e,g, strlen
>> pass) and indexes of arguments.  Also now index of arg in fndecl does
>> not match the index of actual arg in the call.  For such cases new
>> functions are introduced to get args by it's original index, like
>> there is no instrumentation.  BTW there are not many usages of these
>> new functions and almost all of them are in strlen pass.
>>
>> Also some changes, like the patch I sent for calls modifications and
>> copy, are required.  I suppose such changes are quite simple. and not
>> very wide.
>>
>> Regarding proposal to put all bounds to the end of call's arg list - I
>> do not think it makes life much easier.  It still requires
>> modifications in verifier (because it checks arg count), calls copy
>> etc.  It becomes more complex to identify bounds of arg.  Also GIMPLE
>> dump for calls becomes less informative.  Surely it allows to get rid
>> of gimple_call_nobnd_arg and gimple_call_get_nobnd_arg_index but will
>> require additional interface functions for bound args.
>
> What about passing bounds together with the value in the same slot?
> Like via
>
>   arg = __builtin_tie_arg_and_bound (arg, bound);
>   foo (arg);

There are other ways to get bounds. E.g. load bounds from table. If I
use the same builtin for both binding and bounds load, then It is not
clear what to do in case same bounds are used for multiple aegs. For
Example:

int *p
int foo()
{
  bar(p, p+1, p+2);
}

Here all 3 args have same bounds, which are result of __builtin_bndldx
call. If I use the same builtin for binding then I have 3 loads. So,
probably, a separate builtin is better.

>
> you say you can have bounds for aggregates, do you mean
>
>   struct X { int *x; int *y; };
>   foo (struct X);
>
> the ABI says for a call to foo () you pass two bound arguments?
> Or do you merely mean the degenerate case struct X { int *x; }?

In case of structure we pass bounds for all pointers in it.

Ilya

>
> Richard.
>
>> Ilya
>>
>>>
>>> Jeff