Re: [PATCH, MPX, 2/X] Pointers Checker [7/25] Suppress BUILT_IN_CHKP_ARG_BND optimizations.

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Nov 5, 2013 at 1:02 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2013/11/4 Richard Biener <richard.guenther@gmail.com>:
>> Richard Biener <richard.guenther@gmail.com> wrote:
>>>On Thu, Oct 31, 2013 at 10:02 AM, Ilya Enkovich
>>><enkovich.gnu@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> Here is a patch which hadles the problem with optimization of
>>>BUILT_IN_CHKP_ARG_BND calls.  Pointer Bounds Checker expects that
>>>argument of this call is a default SSA_NAME of the PARM_DECL whose
>>>bounds we want to get.  The problem is in optimizations which may
>>>replace arg with it's copy or a known value.  This patch suppress such
>>>modifications.
>>>
>>>This doesn't seem like a good fix.  I suppose you require the same on
>>>RTL, that is, have the incoming arg reg coalesced with the use?
>>>In that case better set SSA_NAME_OCCURS_IN_ABNORMAL_PHI.
>>
>> Btw, I would have chosen
>>
>> P_2 = __builtin_xyz (p_1, bound)
>> Call (p_2)
>>
>> Thus make the builtins a transparent pass-through which effectively binds parameter to their bound, removing the need for the artificial arguments and making propagations a non.issue
>>
>> Also, how does the feature interact with other extensions such as nested functions or optimizations like inlining?
>
> In RTL all incoming bounds are materialized into slot where bounds are
> passed and arg_bnd call is expanded into this slot.  Thus in RTL bound
> arg looks more like a regular arg.

I don't care so much for RTL, but this description hints at that the
suggestion above would work (and it would eliminate all my concerns about
the representation on the GIMPLE level - you'd not even need this
strange POINTER_BOUNDS_TYPE as far as I can see.

> If I just set abnormal phi flag for SSA_NAME, SSA verifier should fail
> because it does not used in abnormal phi, shouldn't it?  Also it would
> prevent all optimizations for these SSA_NAMEs right?  Instrumentation
> is performed on the earlier stage, right after we build SSA. I think
> using abnormal phi flag and binding pointers with bounds via calls
> would prevent some useful optimizations.

Well, what are the constraints that you need to avoid propagation in
the first place?

> Many interprocedural optimizations require some support when work with
> instrumented calls.  Inlining support includes:
>   - replacement of arg_bnd calls with actual bounds passed to the inlined call
>   - replacement of retbnd call with bounds, returned by inlined function
>
> Not all IPA passes are fully enabled right now.  E.g. I restrict
> bounded value propagation in ipa-prop and bounded args in functions
> generated by ipa-split.  Such features will be enabled later.
>
> For nested functions I do not see much difference from checker point
> of view.  It just has an additional static chain param. Probably I
> miss here something.  I did just few tests with nested functions.

I was thinking of

int foo (char *s)
{
   int bar (void)
   {
      ... use bound of 's' of the containing function ...
      foo (q, and pass it along here for q)
   }
}

that is references to the containing functions parameters and their bounds.

Richard.

> Ilya
>
>>
>> Richard.
>>
>>
>>>Richard.
>>>
>>>> Thanks,
>>>> Ilya
>>>> --
>>>>
>>>> gcc/
>>>>
>>>> 2013-10-28  Ilya Enkovich  <ilya.enkovich@intel.com>
>>>>
>>>>         * tree-into-ssa.c: Include "target.h"
>>>>         (rewrite_update_stmt): Skip BUILT_IN_CHKP_ARG_BND calls.
>>>>         * tree-ssa-dom.c: Include "target.h"
>>>>         (cprop_into_stmt): Skip BUILT_IN_CHKP_ARG_BND calls.
>>>>
>>>>
>>>> diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c
>>>> index 981e9f4..8d48f6d 100644
>>>> --- a/gcc/tree-into-ssa.c
>>>> +++ b/gcc/tree-into-ssa.c
>>>> @@ -46,6 +46,7 @@ along with GCC; see the file COPYING3.  If not see
>>>>  #include "params.h"
>>>>  #include "diagnostic-core.h"
>>>>  #include "tree-into-ssa.h"
>>>> +#include "target.h"
>>>>
>>>>
>>>>  /* This file builds the SSA form for a function as described in:
>>>> @@ -1921,8 +1922,14 @@ rewrite_update_stmt (gimple stmt,
>>>gimple_stmt_iterator gsi)
>>>>      }
>>>>
>>>>    /* Rewrite USES included in OLD_SSA_NAMES and USES whose
>>>underlying
>>>> -     symbol is marked for renaming.  */
>>>> -  if (rewrite_uses_p (stmt))
>>>> +     symbol is marked for renaming.
>>>> +     Skip calls to BUILT_IN_CHKP_ARG_BND whose arg should never be
>>>> +     renamed.  */
>>>> +  if (rewrite_uses_p (stmt)
>>>> +      && !(flag_check_pointer_bounds
>>>> +          && (gimple_code (stmt) == GIMPLE_CALL)
>>>> +          && gimple_call_fndecl (stmt)
>>>> +          == targetm.builtin_chkp_function (BUILT_IN_CHKP_ARG_BND)))
>>>>      {
>>>>        if (is_gimple_debug (stmt))
>>>>         {
>>>> diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
>>>> index 211bfcf..445278a 100644
>>>> --- a/gcc/tree-ssa-dom.c
>>>> +++ b/gcc/tree-ssa-dom.c
>>>> @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.  If not see
>>>>  #include "params.h"
>>>>  #include "tree-ssa-threadedge.h"
>>>>  #include "tree-ssa-dom.h"
>>>> +#include "target.h"
>>>>
>>>>  /* This file implements optimizations on the dominator tree.  */
>>>>
>>>> @@ -2266,6 +2267,16 @@ cprop_into_stmt (gimple stmt)
>>>>    use_operand_p op_p;
>>>>    ssa_op_iter iter;
>>>>
>>>> +  /* Call used to obtain bounds of input arg by Pointer Bounds
>>>Checker
>>>> +     should not be optimized.  Argument of the call is a default
>>>> +     SSA_NAME of PARM_DECL.  It should never be replaced by value.
>>>*/
>>>> +  if (flag_check_pointer_bounds && gimple_code (stmt) ==
>>>GIMPLE_CALL)
>>>> +    {
>>>> +      tree fndecl = gimple_call_fndecl (stmt);
>>>> +      if (fndecl == targetm.builtin_chkp_function
>>>(BUILT_IN_CHKP_ARG_BND))
>>>> +       return;
>>>> +    }
>>>> +
>>>>    FOR_EACH_SSA_USE_OPERAND (op_p, stmt, iter, SSA_OP_USE)
>>>>      cprop_operand (stmt, op_p);
>>>>  }
>>
>>