On Wed, Oct 30, 2013 at 6:40 PM, Jeff Law <l...@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
>
