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 <hars...@google.com> wrote:
> Adding function attributes: 'always_patch_for_instrumentation' and
> 'never_patch_for_instrumentation' to always patch a function or to never
> patch a function, respectively, when given the option
> -mpatch-functions-for-instrumentation. Additionally, the attribute
> always_patch_for_instrumentation disables inlining of that function.
>
> Tested:
> Tested by 'crosstool-validate.py --crosstool_ver=16 --testers=crosstool'
>
> ChangeLog:
>
> 2012-10-30 Harshit Chopra <hars...@google.com>
>
> * gcc/c-family/c-common.c
> (handle_always_patch_for_instrumentation_attribute): Handle
> always_patch_for_instrumentation attribute and turn inlining off for the
> function.
> (handle_never_patch_for_instrumentation_attribute): Handle
> never_patch_for_instrumentation
> attribute of a function.
> * gcc/config/i386/i386.c (check_should_patch_current_function): Takes
> into account
> always_patch_for_instrumentation or never_patch_for_instrumentation
> attribute when
> deciding that a function should be patched.
> * gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching.c:
> Test case
> to test for never_patch_for_instrumentation attribute.
> * gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c:
> Test case to
> test for always_patch_for_instrumentation attribute.
> * gcc/tree.h (struct GTY): Add fields for the two attributes and
> macros to access
> the fields.
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index ab416ff..998645d 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -396,6 +396,13 @@ static tree ignore_attribute (tree *, tree, tree, int,
> bool *);
> static tree handle_no_split_stack_attribute (tree *, tree, tree, int, bool
> *);
> static tree handle_fnspec_attribute (tree *, tree, tree, int, bool *);
>
> +static tree handle_always_patch_for_instrumentation_attribute (tree *, tree,
> + tree, int,
> + bool *);
Move bool * to the previous line.
> +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
