[PATCH, Pointer Bounds Checker, Builtins instrumentation 1/5] Builtin codes and decls

[Ilya Enkovich]

2014-11-14 9:43 GMT+03:00 Jeff Law <law@redhat.com>:
> On 11/06/14 04:48, Ilya Enkovich wrote:
>>
>> --
>> 2014-11-06  Ilya Enkovich  <ilya.enkovich@intel.com>
>>
>>         * tree-core.h (built_in_class): Add builtin codes to be used
>>         by Pointer Bounds Checker for instrumented builtin functions.
>>         * tree-streamer-in.c: Include ipa-chkp.h.
>>         (streamer_get_builtin_tree): Create instrumented decl if
>>         required.
>>         * ipa-chkp.h (chkp_maybe_clone_builtin_fndecl): New.
>>         * ipa-chkp.c (chkp_build_instrumented_fndecl): Support builtin
>>         function decls.
>>         (chkp_maybe_clone_builtin_fndecl): New.
>>         (chkp_maybe_create_clone): Support builtin function decls.
>
> Looks much better than prior versions.
>>
>>
>>
>> @@ -355,6 +365,30 @@ chkp_add_bounds_params_to_function (tree fndecl)
>>       chkp_copy_function_type_adding_bounds (TREE_TYPE (fndecl));
>>   }
>>
>> +tree
>> +chkp_maybe_clone_builtin_fndecl (tree fndecl)
>
> Need a function comment here.
>
>
>>   /* Return clone created for instrumentation of NODE or NULL.  */
>>
>>   cgraph_node *
>> @@ -365,6 +399,52 @@ chkp_maybe_create_clone (tree fndecl)
>>
>>     gcc_assert (!node->instrumentation_clone);
>>
>> +  if (DECL_BUILT_IN (fndecl)
>> +      && (DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL
>> +         || DECL_FUNCTION_CODE (fndecl) >= BEGIN_CHKP_BUILTINS))
>> +    return NULL;
>
> Just so I'm sure, the only way to get into chkp_maybe_clone_builtin_decl is
> if this test is false.  Right?
>
> Can we ultimately end up with checking clones for any normal builtin? What
> filters out builtins that don't need a checking variant?

As you already noticed the filter is in the second patch.

>
>
>> +
>> +  clone = node->instrumented_version;
>> +
>> +  /* For builtin functions we may loose and recreate
>> +     cgraph node.  We should check if we already have
>> +     instrumented version.  */
>
> Can you describe to me under what circumstances this happens?  It seems like
> we may be papering over an issue that would be better fixed elsewhere.

I don't think I'm hiding some problem here.  Builtin function calls
may be removed during various optimizations.  Therefore we may remove
all calls to some instrumented builtins and corresponding cgraph_node
is removed as unreachable (but fndecl still exists).  Later calls to
removed function may be created again.  IIRC in my test case it
happened in strlen pass which may replace builtin calls with another
ones.  In this case cgraph_node is recreated but fndecl recreation
should be avoided, existing one should be used instead.

Thanks,
Ilya

>
>
>> @@ -409,6 +489,15 @@ chkp_maybe_create_clone (tree fndecl)
>>          actually copies args list from the original decl.  */
>>         chkp_add_bounds_params_to_function (new_decl);
>>
>> +      /* Remember builtin fndecl.  */
>> +      if (DECL_BUILT_IN_CLASS (clone->decl) == BUILT_IN_NORMAL
>> +         && fndecl == builtin_decl_explicit (DECL_FUNCTION_CODE
>> (fndecl)))
>> +       {
>> +         gcc_assert (!builtin_decl_explicit (DECL_FUNCTION_CODE
>> (clone->decl)));
>> +         set_builtin_decl (DECL_FUNCTION_CODE (clone->decl),
>> +                           clone->decl, false);
>> +       }
>
> I'm not a big fan of slamming in a new DECL like this, but it may be OK.
> I'm not going to object to that now, but I worry about downstream impacts.
>
>
> Tentatively OK after adding the missing function comment.  Please wait for
> entire kit to be approved before committing anything.  I may come back to
> something as I dig deeper into the other patches in the series.
>
> jeff