Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2013/11/20 Richard Biener <richard.guenther@gmail.com>:
> On Tue, Nov 19, 2013 at 8:02 PM, Jeff Law <law@redhat.com> wrote:
>> On 11/18/13 14:03, Ilya Enkovich wrote:
>>>
>>> 2013/11/19 Jeff Law <law@redhat.com>:
>>>>
>>>> On 11/18/13 12:16, Ilya Enkovich wrote:
>>>>>
>>>>>
>>>>> With current recursion elimination we will have:
>>>>>
>>>>> test (int *param1)
>>>>> {
>>>>> <bb1>:
>>>>>
>>>>> <bb2>:
>>>>>     _7 = PHI<param1(D) (bb1), _6 (bb2)>
>>>>>     bounds2 = __builtin_arg_bounds (_7) -- WRONG
>>>>
>>>>
>>>> I wouldn't say it's wrong.  It's conservatively correct if properly
>>>> implemented.   Why precisely do you consider this wrong?  If your code
>>>> can't
>>>> handle it, it really should be fixed.
>>>
>>>
>>> It is wrong because __builtin_arg_bounds is used to get bounds of
>>> input parameter and PHI node here is not an input parameter.
>>
>> OK, now we're getting somewhere.  So we've got this odd little function
>> which only works on parameters.   I can't help but feel this is a bit of
>> mis-design coming back to haunt us and I wonder if we're going to see other
>> instances of this kind of problem.
>
> Right.
>
>> There's something just wrong with the semantics of __builtin_arg_bounds, but
>> I'm having trouble putting my finger on it.
>
> Well, the issue is that it accepts any pointer argument as input.
> I originally thought it may just get an integer constant argument - the
> argument position.  It really seems to me that we have
> __builtin_arg_boundsN (), but to avoid having N builtins we specify N
> via an argument to the function.  But as it may not change we should
> use something that cannot change - like a constant.  A constant
> that identifies a parameter is one that gives its position for example.

I have tried to pass constant for arg_bounds.  It still requires
additional modification of optimization passes (including tail
recursion, param propagation, inline etc.)  But having constant as an
argument you really cannot detect errors.  E.g. if you inline and do
not fix inlined arg_bounds calls correctly, you may not get ICE but
get wrong program which uses wrong bounds and produces false bounds
violations.

>
> Note that any restrictions you impose on what is "valid" for GIMPLE
> should be verified in tree-cfg.c:verify_gimple_* - in the
> __builtin_arg_bounds call case in verify_gimple_call.

Will add it.

Thanks,
Ilya

>
> Richard.
>
>>
>>  Correctl
>>>
>>> handling of __builtin_arg_bounds in this 'wrong' example requires
>>> adding it a PHI node semantics based on it's arg.  For me it seems
>>> more complex then generating a regular PHI node for bounds and makes
>>> GIMPLE less readable.
>>
>> But presumably you have code to merge bound information already, right?  You
>> need that for PHI nodes.  Can't you use that to walk up the use-def chains
>> and build the bounds information?
>>
>> Or if you want to fix the tailcall stuff, you can start down that direction.
>> I don't think that'll fix the nagging concerns about the overall semantics
>> of builtin_arg_bounds, but it might be enough for you to go forward.
>>
>> jeff