* [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
@ 2012-10-31 0:39 Harshit Chopra
2012-11-03 19:38 ` Xinliang David Li
0 siblings, 1 reply; 7+ messages in thread
From: Harshit Chopra @ 2012-10-31 0:39 UTC (permalink / raw)
To: gcc-patches, davidxl, reply
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
2012-10-31 0:39 [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051) Harshit Chopra
@ 2012-11-03 19:38 ` Xinliang David Li
2012-11-05 20:21 ` Harshit Chopra
0 siblings, 1 reply; 7+ messages in thread
From: Xinliang David Li @ 2012-11-03 19:38 UTC (permalink / raw)
To: Harshit Chopra; +Cc: GCC Patches, reply
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
2012-11-03 19:38 ` Xinliang David Li
@ 2012-11-05 20:21 ` Harshit Chopra
2012-11-05 20:29 ` Xinliang David Li
0 siblings, 1 reply; 7+ messages in thread
From: Harshit Chopra @ 2012-11-05 20:21 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches, reply
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
2012-11-05 20:21 ` Harshit Chopra
@ 2012-11-05 20:29 ` Xinliang David Li
2012-11-07 1:18 ` Harshit Chopra
0 siblings, 1 reply; 7+ messages in thread
From: Xinliang David Li @ 2012-11-05 20:29 UTC (permalink / raw)
To: Harshit Chopra; +Cc: GCC Patches, reply
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
2012-11-05 20:29 ` Xinliang David Li
@ 2012-11-07 1:18 ` Harshit Chopra
2012-11-07 16:44 ` Xinliang David Li
0 siblings, 1 reply; 7+ messages in thread
From: Harshit Chopra @ 2012-11-07 1:18 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches, reply
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
2012-11-07 1:18 ` Harshit Chopra
@ 2012-11-07 16:44 ` Xinliang David Li
0 siblings, 0 replies; 7+ messages in thread
From: Xinliang David Li @ 2012-11-07 16:44 UTC (permalink / raw)
To: Harshit Chopra; +Cc: GCC Patches, reply
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
@ 2012-11-05 20:20 Harshit Chopra
0 siblings, 0 replies; 7+ messages in thread
From: Harshit Chopra @ 2012-11-05 20:20 UTC (permalink / raw)
To: gcc-patches, reply
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-11-07 16:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-31 0:39 [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051) Harshit Chopra
2012-11-03 19:38 ` Xinliang David Li
2012-11-05 20:21 ` Harshit Chopra
2012-11-05 20:29 ` Xinliang David Li
2012-11-07 1:18 ` Harshit Chopra
2012-11-07 16:44 ` Xinliang David Li
2012-11-05 20:20 Harshit Chopra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).