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

[Jeff Law <law at redhat dot com>]

On 09/15/14 01:14, Ilya Enkovich wrote:
> Ping
>
> 2014-06-05 15:03 GMT+04:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
> > 2014-06-04 17:35 GMT+04:00 Richard Biener <richard.guenther@gmail.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.
> >
> > I create constructors at the end of unit compilation in
> > chkp_finish_file. Constructor body in this case is MODIFY_EXPRs for
> > all statically initialized pointers. "chkp ctor" attribute is used to
> > instrument this constructor properly - all pointer modifications
> > should be replaced with corresponding bounds initialization. Thus
> > gimplification is not the only place where this attribute is used.
This patch is fine.  I know Richi didn't like it, but I don't see a 
really good way around this.

Jeff