Re: [PATCH, Pointer Bounds Checker 6/x] New static constructor types

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

On Wed, Jun 4, 2014 at 3:13 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2014-06-04 13:58 GMT+04:00 Richard Biener <richard.guenther@gmail.com>:
>> On Wed, Apr 16, 2014 at 2:33 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>> Hi,
>>>
>>> This patch add new static constructor types used by Pointer Bounds Checker.  It was approved earlier for 4.9 and I'll assume patch is OK for trunk if no objections arise.
>>>
>>> Patch was bootstrapped and tested for linux-x86_64.
>>>
>>> Thanks,
>>> Ilya
>>> --
>>> gcc/
>>>
>>> 2014-04-16  Ilya Enkovich  <ilya.enkovich@intel.com>
>>>
>>>         * ipa.c (cgraph_build_static_cdtor_1): Support contructors
>>>         with "chkp ctor" and "bnd_legacy" attributes.
>>>         * gimplify.c (gimplify_init_constructor): Avoid infinite
>>>         loop during gimplification of bounds initializer.
>>>
>>>
>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>>> index 7441784..67ab515 100644
>>> --- a/gcc/gimplify.c
>>> +++ b/gcc/gimplify.c
>>> @@ -3803,10 +3803,19 @@ gimplify_init_constructor (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>>>            individual element initialization.  Also don't do this for small
>>>            all-zero initializers (which aren't big enough to merit
>>>            clearing), and don't try to make bitwise copies of
>>> -          TREE_ADDRESSABLE types.  */
>>> +          TREE_ADDRESSABLE types.
>>> +
>>> +          We cannot apply such transformation when compiling chkp static
>>> +          initializer because creation of initializer image in the memory
>>> +          will require static initialization of bounds for it.  It should
>>> +          result in another gimplification of similar initializer and we
>>> +          may fall into infinite loop.  */
>>>         if (valid_const_initializer
>>>             && !(cleared || num_nonzero_elements == 0)
>>> -           && !TREE_ADDRESSABLE (type))
>>> +           && !TREE_ADDRESSABLE (type)
>>> +           && (!current_function_decl
>>> +               || !lookup_attribute ("chkp ctor",
>>> +                                     DECL_ATTRIBUTES (current_function_decl))))
>>
>> Simply make the type TREE_ADDRESSABLE?
>
> Wouldn't it be a hack to mark it addressable just to not hit this
> condition? It would also require to have an unshared copy of the type
> to not affect other statements with that type.
>
>>
>>>           {
>>>             HOST_WIDE_INT size = int_size_in_bytes (type);
>>>             unsigned int align;
>>> diff --git a/gcc/ipa.c b/gcc/ipa.c
>>> index 26e9b03..5ab3aed 100644
>>> --- a/gcc/ipa.c
>>> +++ b/gcc/ipa.c
>>> @@ -1345,9 +1345,11 @@ make_pass_ipa_whole_program_visibility (gcc::context *ctxt)
>>>  }
>>>
>>>  /* Generate and emit a static constructor or destructor.  WHICH must
>>> -   be one of 'I' (for a constructor) or 'D' (for a destructor).  BODY
>>> -   is a STATEMENT_LIST containing GENERIC statements.  PRIORITY is the
>>> -   initialization priority for this constructor or destructor.
>>> +   be one of 'I' (for a constructor), 'D' (for a destructor), 'P'
>>> +   (for chp static vars constructor) or 'B' (for chkp static bounds
>>> +   constructor).  BODY is a STATEMENT_LIST containing GENERIC
>>> +   statements.  PRIORITY is the initialization priority for this
>>> +   constructor or destructor.
>>>
>>>     FINAL specify whether the externally visible name for collect2 should
>>>     be produced. */
>>> @@ -1406,6 +1408,20 @@ cgraph_build_static_cdtor_1 (char which, tree body, int priority, bool final)
>>>        DECL_STATIC_CONSTRUCTOR (decl) = 1;
>>>        decl_init_priority_insert (decl, priority);
>>>        break;
>>> +    case 'P':
>>> +      DECL_STATIC_CONSTRUCTOR (decl) = 1;
>>> +      DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("chkp ctor"),
>>> +                                         NULL,
>>> +                                         NULL_TREE);
>>
>> Ick.  Please try to avoid using attributes for this.  Rather adjust the
>> caller of this function to set a flag in the cgraph node.
>
> It is too late because all early local passes are executed by that
> time and I need this attribute to be set before them.

Ok, so where do you call this from?  It should be possible to create
the function in GIMPLE form directly, avoiding the need to go through
gimplification.

Richard.

> Ilya
>
>>
>> So I don't like this patch at all.
>>
>> Richard.
>>
>>> +      decl_init_priority_insert (decl, priority);
>>> +      break;
>>> +    case 'B':
>>> +      DECL_STATIC_CONSTRUCTOR (decl) = 1;
>>> +      DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("bnd_legacy"),
>>> +                                         NULL,
>>> +                                         NULL_TREE);
>>> +      decl_init_priority_insert (decl, priority);
>>> +      break;
>>>      case 'D':
>>>        DECL_STATIC_DESTRUCTOR (decl) = 1;
>>>        decl_fini_priority_insert (decl, priority);
>>> @@ -1423,9 +1439,11 @@ cgraph_build_static_cdtor_1 (char which, tree body, int priority, bool final)
>>>  }
>>>
>>>  /* Generate and emit a static constructor or destructor.  WHICH must
>>> -   be one of 'I' (for a constructor) or 'D' (for a destructor).  BODY
>>> -   is a STATEMENT_LIST containing GENERIC statements.  PRIORITY is the
>>> -   initialization priority for this constructor or destructor.  */
>>> +   be one of 'I' (for a constructor), 'D' (for a destructor), 'P'
>>> +   (for chkp static vars constructor) or 'B' (for chkp static bounds
>>> +   constructor).  BODY is a STATEMENT_LIST containing GENERIC
>>> +   statements.  PRIORITY is the initialization priority for this
>>> +   constructor or destructor.  */
>>>
>>>  void
>>>  cgraph_build_static_cdtor (char which, tree body, int priority)