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
