Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)

[Harshit Chopra <harshit at google dot com>]

Thanks David for the review. My comments are inline.


On Sat, Nov 3, 2012 at 12:38 PM, Xinliang David Li <davidxl@google.com> wrote:
>
> Harshit, Nov 5 is the gcc48 cutoff date. If you want to have the x-ray
> instrumentation feature into this release, you will need to port your
> patch and submit for trunk review now.


I am a bit too late now, I guess. If I target for the next release,
will it create any issues for the gcc48 release?

>
>
>
> On Tue, Oct 30, 2012 at 5:15 PM, Harshit Chopra <harshit@google.com> wrote:
> > Adding function attributes: 'always_patch_for_instrumentation' and 'never_patch_for_instrumentation' to always patch a function or to never patch a function, respectively, when given the option -mpatch-functions-for-instrumentation. Additionally, the attribute always_patch_for_instrumentation disables inlining of that function.
> >
> > Tested:
> >   Tested by 'crosstool-validate.py --crosstool_ver=16 --testers=crosstool'
> >
> > ChangeLog:
> >
> > 2012-10-30  Harshit Chopra <harshit@google.com>
> >
> >         * gcc/c-family/c-common.c (handle_always_patch_for_instrumentation_attribute): Handle
> >   always_patch_for_instrumentation attribute and turn inlining off for the function.
> >         (handle_never_patch_for_instrumentation_attribute): Handle never_patch_for_instrumentation
> >   attribute of a function.
> >         * gcc/config/i386/i386.c (check_should_patch_current_function): Takes into account
> >   always_patch_for_instrumentation or never_patch_for_instrumentation attribute when
> >   deciding that a function should be patched.
> >         * gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching.c: Test case
> >   to test for never_patch_for_instrumentation attribute.
> >         * gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c: Test case to
> >   test for always_patch_for_instrumentation attribute.
> >         * gcc/tree.h (struct GTY): Add fields for the two attributes and macros to access
> >   the fields.
> > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> > index ab416ff..998645d 100644
> > --- a/gcc/c-family/c-common.c
> > +++ b/gcc/c-family/c-common.c
> > @@ -396,6 +396,13 @@ static tree ignore_attribute (tree *, tree, tree, int, bool *);
> >  static tree handle_no_split_stack_attribute (tree *, tree, tree, int, bool *);
> >  static tree handle_fnspec_attribute (tree *, tree, tree, int, bool *);
> >
> > +static tree handle_always_patch_for_instrumentation_attribute (tree *, tree,
> > +                                                               tree, int,
> > +                                                               bool *);
>
> Move bool * to the previous line.


If I do that, it goes beyond the 80 char boundary.

>
>
> > +static tree handle_never_patch_for_instrumentation_attribute (tree *, tree,
> > +                                                              tree, int,
> > +                                                              bool *);
> > +
>
> Same here.


As above.

>
>
> >  static void check_function_nonnull (tree, int, tree *);
> >  static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
> >  static bool nonnull_check_p (tree, unsigned HOST_WIDE_INT);
> > @@ -804,6 +811,13 @@ const struct attribute_spec c_common_attribute_table[] =
> >       The name contains space to prevent its usage in source code.  */
> >    { "fn spec",               1, 1, false, true, true,
> >                               handle_fnspec_attribute, false },
> > +  { "always_patch_for_instrumentation", 0, 0, true,  false, false,
> > +                              handle_always_patch_for_instrumentation_attribute,
> > +                              false },
> > +  { "never_patch_for_instrumentation", 0, 0, true,  false, false,
> > +                              handle_never_patch_for_instrumentation_attribute,
> > +                              false },
> > +
> >    { NULL,                     0, 0, false, false, false, NULL, false }
> >  };
> >
> > @@ -9158,6 +9172,48 @@ handle_no_thread_safety_analysis_attribute (tree *node, tree name,
> >    return NULL_TREE;
> >  }
> >
> > +/* Handle a "always_patch_for_instrumentation" attribute; arguments as in
> > +   struct attribute_spec.handler.  */
>
> Add new line here


Done.

>
>
> > +static tree
> > +handle_always_patch_for_instrumentation_attribute (tree *node, tree name,
> > +                                                   tree ARG_UNUSED (args),
> > +                                                   int ARG_UNUSED (flags),
> > +                                                   bool *no_add_attrs)
> > +{
> > +  if (TREE_CODE (*node) == FUNCTION_DECL)
> > +    {
> > +      DECL_FORCE_PATCHING_FOR_INSTRUMENTATION (*node) = 1;
> > +      DECL_UNINLINABLE (*node) = 1;
> > +    }
> > +  else
> > +    {
> > +      warning (OPT_Wattributes, "%qE attribute ignored", name);
> > +      *no_add_attrs = true;
> > +    }
> > +  return NULL_TREE;
> > +}
> > +
> > +
> > +/* Handle a "never_patch_for_instrumentation" attribute; arguments as in
> > +   struct attribute_spec.handler.  */
>
> A new line here.


Done

>
>
> > +static tree
> > +handle_never_patch_for_instrumentation_attribute (tree *node, tree name,
> > +                                                  tree ARG_UNUSED (args),
> > +                                                  int ARG_UNUSED (flags),
> > +                                                  bool *no_add_attrs)
> > +{
> > +  if (TREE_CODE (*node) == FUNCTION_DECL)
> > +    DECL_FORCE_NO_PATCHING_FOR_INSTRUMENTATION (*node) = 1;
>
> Probably no need for this. The attribute will be attached to the decl
> node -- can be queried using lookup_attribute method.


Done.

>
>
> > +  else
> > +    {
> > +      warning (OPT_Wattributes, "%qE attribute ignored", name);
> > +      *no_add_attrs = true;
> > +    }
> > +  return NULL_TREE;
> > +}
> > +
> > +
> > +
> >  /* Check for valid arguments being passed to a function with FNTYPE.
> >     There are NARGS arguments in the array ARGARRAY.  */
> >  void
> > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> > index 6972ae6..b1475bf 100644
> > --- a/gcc/config/i386/i386.c
> > +++ b/gcc/config/i386/i386.c
> > @@ -10983,6 +10983,13 @@ check_should_patch_current_function (void)
> >    int num_loops = 0;
> >    int min_functions_instructions;
> >
> > +  /* If a function has an attribute forcing patching on or off, do as it
> > +     indicates.  */
> > +  if (DECL_FORCE_PATCHING_FOR_INSTRUMENTATION (current_function_decl))
> > +    return true;
> > +  else if (DECL_FORCE_NO_PATCHING_FOR_INSTRUMENTATION (current_function_decl))
> > +    return false;
> > +
> >    /* Patch the function if it has at least a loop.  */
> >    if (!patch_functions_ignore_loops)
> >      {
> > diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching.c b/gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching.c
> > new file mode 100644
> > index 0000000..cad6f2d
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching.c
> > @@ -0,0 +1,27 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target lp64 } */
> > +/* { dg-options "-mpatch-functions-for-instrumentation -mno-patch-functions-main-always" } */
>
> >  /* Nonzero if a FUNCTION_CODE is a TM load/store.  */
> >  #define BUILTIN_TM_LOAD_STORE_P(FN) \
> >    ((FN) >= BUILT_IN_TM_STORE_1 && (FN) <= BUILT_IN_TM_LOAD_RFW_LDOUBLE)
> > @@ -3586,7 +3596,8 @@ struct GTY(()) tree_function_decl {
> >    unsigned has_debug_args_flag : 1;
> >    unsigned tm_clone_flag : 1;
> >
> > -  /* 1 bit left */
> > +  unsigned force_patching_for_instrumentation : 1;
> > +  unsigned force_no_patching_for_instrumentation : 1;
>
>
> I don't think you should use precious bits here -- directly query the
> attributes.


Done.

>
>
> thanks,
>
> David
>
> >  };
> >
> >  /* The source language of the translation-unit.  */
> >
> > --
> > This patch is available for review at http://codereview.appspot.com/6821051