ok for google branches. David
On Tue, Nov 6, 2012 at 5:17 PM, Harshit Chopra <hars...@google.com> wrote: > Yes, will do, but probably not so soon. Once I have some spare time to > prepare my case for this being useful to public. > > Meanwhile, this patch is just for google-main and then I will port it > to google_4-7 and adds to the already existing functionality of > -mpatch-function-for-instrumentation. > > Thanks, > Harshit > > > On Mon, Nov 5, 2012 at 12:29 PM, Xinliang David Li <davi...@google.com> wrote: >> It does not hurt to submit the patch for review -- you need to provide >> more background and motivation for this work >> 1) comparison with -finstrument-functions (runtime overhead etc) >> 2) use model difference (production binary ..) >> 3) Interesting examples of use cases (with graphs). >> >> thanks, >> >> David >> >> On Mon, Nov 5, 2012 at 12:20 PM, Harshit Chopra <hars...@google.com> wrote: >>> Thanks David for the review. My comments are inline. >>> >>> >>> On Sat, Nov 3, 2012 at 12:38 PM, Xinliang David Li <davi...@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 <hars...@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 <hars...@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