On Wed, Oct 30, 2013 at 11:48 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Wed, Oct 30, 2013 at 11:34 AM, Ilya Enkovich <enkovich....@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.enkov...@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}