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