Issue 6821051: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation
DescriptionAdding 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.
Patch Set 1 #Patch Set 2 : [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation #
MessagesTotal messages: 8
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 *); +static tree handle_never_patch_for_instrumentation_attribute (tree *, tree, + tree, int, + bool *); + 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. */ +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. */ +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; + 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" } */ + +/* Even complicated functions shouldn't get patched if they have the + never_patch_for_instrumentation attribute. */ + +/* { dg-final { scan-assembler-not ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler-not "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +__attribute__ ((never_patch_for_instrumentation)) +int foo () { + volatile unsigned x = 0; + volatile unsigned y = 1; + x += y; + x *= y; + while (++x) + foo (); + return y; +} + + +int main () +{ + int x = 0; + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c b/gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c new file mode 100644 index 0000000..86ad159 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c @@ -0,0 +1,20 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target lp64 } */ +/* { dg-options "-O3 -mpatch-functions-for-instrumentation -mno-patch-functions-main-always" } */ + +/* Functions which have the always_patch attribute should be patched no matter + what. Check that there are nop-bytes at the beginning and end of the + function. We add -O3 so that the compiler will try to inline foo (but it + will be blocked by the attribute). */ + +/* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +__attribute__ ((always_patch_for_instrumentation)) +static int foo () { + return 3; +} + +int main () { + volatile int x = foo (); +} diff --git a/gcc/tree.h b/gcc/tree.h index 1eafaa0..9d26a00 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3474,6 +3474,16 @@ struct GTY(()) #define DECL_NO_INLINE_WARNING_P(NODE) \ (FUNCTION_DECL_CHECK (NODE)->function_decl.no_inline_warning_flag) +/* In a FUNCTION_DECL, nonzero if patching is forced. */ +#define DECL_FORCE_PATCHING_FOR_INSTRUMENTATION(NODE) \ + (FUNCTION_DECL_CHECK (NODE)->function_decl.force_patching_for_instrumentation) + +/* In a FUNCTION_DECL, nonzero if not patching is forced. */ +#define DECL_FORCE_NO_PATCHING_FOR_INSTRUMENTATION(NODE)\ + (FUNCTION_DECL_CHECK (NODE)->function_decl.force_no_patching_for_instrumentation) + + + /* 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; }; /* The source language of the translation-unit. */ -- This patch is available for review at http://codereview.appspot.com/6821051
Sign in to reply to this message.
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. 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. > +static tree handle_never_patch_for_instrumentation_attribute (tree *, tree, > + tree, int, > + bool *); > + Same here. > 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 > +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. > +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. > + 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. thanks, David > }; > > /* The source language of the translation-unit. */ > > -- > This patch is available for review at http://codereview.appspot.com/6821051
Sign in to reply to this message.
2012-11-05 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 (has_attribute): Checks the presence of an attribute. (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 (int main): 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. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index ab416ff..04b973f 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 *); +static tree handle_never_patch_for_instrumentation_attribute (tree *, tree, + tree, int, + bool *); + 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. */ + +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) + { + /* Disable inlining if forced instrumentation. */ + 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. */ + +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) + { + 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..4e0d770 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -10968,6 +10968,13 @@ ix86_expand_epilogue (int style) returns. */ static bool patch_current_function_p = false; +static inline bool +has_attribute (const char* attribute_name) +{ + return lookup_attribute (attribute_name, + DECL_ATTRIBUTES (current_function_decl)) != NULL; +} + /* Return true if we patch the current function. By default a function is patched if it has loops or if the number of insns is greater than patch_functions_min_instructions (number of insns roughly translates @@ -10983,6 +10990,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 (has_attribute ("always_patch_for_instrumentation")) + return true; + else if (has_attribute ("never_patch_for_instrumentation")) + 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" } */ + +/* Even complicated functions shouldn't get patched if they have the + never_patch_for_instrumentation attribute. */ + +/* { dg-final { scan-assembler-not ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler-not "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +__attribute__ ((never_patch_for_instrumentation)) +int foo () { + volatile unsigned x = 0; + volatile unsigned y = 1; + x += y; + x *= y; + while (++x) + foo (); + return y; +} + + +int main () +{ + int x = 0; + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c b/gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c new file mode 100644 index 0000000..86ad159 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c @@ -0,0 +1,20 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target lp64 } */ +/* { dg-options "-O3 -mpatch-functions-for-instrumentation -mno-patch-functions-main-always" } */ + +/* Functions which have the always_patch attribute should be patched no matter + what. Check that there are nop-bytes at the beginning and end of the + function. We add -O3 so that the compiler will try to inline foo (but it + will be blocked by the attribute). */ + +/* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +__attribute__ ((always_patch_for_instrumentation)) +static int foo () { + return 3; +} + +int main () { + volatile int x = foo (); +} -- This patch is available for review at http://codereview.appspot.com/6821051
Sign in to reply to this message.
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
Sign in to reply to this message.
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 <harshit@google.com> wrote: > 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
Sign in to reply to this message.
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 <davidxl@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 <harshit@google.com> wrote: >> 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
Sign in to reply to this message.
ok for google branches. David On Tue, Nov 6, 2012 at 5:17 PM, Harshit Chopra <harshit@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 <davidxl@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 <harshit@google.com> wrote: >>> 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
Sign in to reply to this message.
Thanks for the review. Submitted to google-main as revision r193381. -- Harshit On Wed, Nov 7, 2012 at 8:44 AM, Xinliang David Li <davidxl@google.com>wrote: > ok for google branches. > > David > > On Tue, Nov 6, 2012 at 5:17 PM, Harshit Chopra <harshit@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 <davidxl@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 <harshit@google.com> > wrote: > >>> 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 >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
