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
