[google] Add function name to function_patch_* sections (issue9025045)
Adding function name to the function_patch_* sections when -ffunction-sections is provided. Helps in garbage collecting dead functions with the help of linker. Tested: Tested using 'make -k check-gcc RUNTESTFLAGS=i386.exp=patch* --target_board=unix\{-m32,,-m64\}'. 2013-04-30 Harshit Chopra hars...@google.com * gcc/config/i386/i386.c (ix86_output_function_nops_prologue_epilogue): (ix86_elf_asm_named_section): diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index aa6ec82..7cb832b 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -11285,6 +11285,8 @@ ix86_output_function_nops_prologue_epilogue (FILE *file, unsigned int section_flags = SECTION_RELRO; char *section_name_comdat = NULL; const char *decl_section_name = NULL; + const char *func_name = NULL; + char *section_name_function_sections = NULL; size_t len; gcc_assert (num_remaining_nops = 0); @@ -11336,12 +11338,24 @@ ix86_output_function_nops_prologue_epilogue (FILE *file, { decl_section_name = TREE_STRING_POINTER (DECL_SECTION_NAME (current_function_decl)); - len = strlen (decl_section_name) + strlen (section_name) + 1; + len = strlen (decl_section_name) + strlen (section_name) + 2; section_name_comdat = (char *) alloca (len); sprintf (section_name_comdat, %s.%s, section_name, decl_section_name); section_name = section_name_comdat; section_flags |= SECTION_LINKONCE; } + else if (flag_function_sections) +{ + func_name = XSTR (XEXP (DECL_RTL (current_function_decl), 0), 0); + if (func_name) +{ + len = strlen (func_name) + strlen (section_name) + 2; + section_name_function_sections = (char *) alloca (len); + sprintf (section_name_function_sections, %s.%s, section_name, + func_name); + section_name = section_name_function_sections; +} +} section = get_section (section_name, section_flags, current_function_decl); switch_to_section (section); /* Align the section to 8-byte boundary. */ @@ -11369,7 +11383,7 @@ ix86_elf_asm_named_section (const char *name, unsigned int flags, tree decl) { const char *section_name = name; - if (HAVE_COMDAT_GROUP flags SECTION_LINKONCE) + if (!flag_function_sections HAVE_COMDAT_GROUP flags SECTION_LINKONCE) { const int prologue_section_name_length = sizeof(FUNCTION_PATCH_PROLOGUE_SECTION) - 1; -- This patch is available for review at http://codereview.appspot.com/9025045
[google] Port revisions for -mpatch-functions-for-instrumentation option back to google-main. (issue7301068)
2013-02-08 Harshit Chopra hars...@google.com Porting revisions r183548, r183888, r186118, r192231, r193381. 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 000..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
Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
Yes, will do, but probably not so soon. Once I have some spare time to prepare my case for this being useful to public. Meanwhile, this patch is just for google-main and then I will port it to google_4-7 and adds to the already existing functionality of -mpatch-function-for-instrumentation. Thanks, Harshit On Mon, Nov 5, 2012 at 12:29 PM, Xinliang David Li davi...@google.com wrote: It does not hurt to submit the patch for review -- you need to provide more background and motivation for this work 1) comparison with -finstrument-functions (runtime overhead etc) 2) use model difference (production binary ..) 3) Interesting examples of use cases (with graphs). thanks, David On Mon, Nov 5, 2012 at 12:20 PM, Harshit Chopra hars...@google.com wrote: Thanks David for the review. My comments are inline. On Sat, Nov 3, 2012 at 12:38 PM, Xinliang David Li davi...@google.com wrote: Harshit, Nov 5 is the gcc48 cutoff date. If you want to have the x-ray instrumentation feature into this release, you will need to port your patch and submit for trunk review now. I am a bit too late now, I guess. If I target for the next release, will it create any issues for the gcc48 release? On Tue, Oct 30, 2012 at 5:15 PM, Harshit Chopra hars...@google.com wrote: Adding function attributes: 'always_patch_for_instrumentation' and 'never_patch_for_instrumentation' to always patch a function or to never patch a function, respectively, when given the option -mpatch-functions-for-instrumentation. Additionally, the attribute always_patch_for_instrumentation disables inlining of that function. Tested: Tested by 'crosstool-validate.py --crosstool_ver=16 --testers=crosstool' ChangeLog: 2012-10-30 Harshit Chopra hars...@google.com * gcc/c-family/c-common.c (handle_always_patch_for_instrumentation_attribute): Handle always_patch_for_instrumentation attribute and turn inlining off for the function. (handle_never_patch_for_instrumentation_attribute): Handle never_patch_for_instrumentation attribute of a function. * gcc/config/i386/i386.c (check_should_patch_current_function): Takes into account always_patch_for_instrumentation or never_patch_for_instrumentation attribute when deciding that a function should be patched. * gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching.c: Test case to test for never_patch_for_instrumentation attribute. * gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c: Test case to test for always_patch_for_instrumentation attribute. * gcc/tree.h (struct GTY): Add fields for the two attributes and macros to access the fields. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index ab416ff..998645d 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -396,6 +396,13 @@ static tree ignore_attribute (tree *, tree, tree, int, bool *); static tree handle_no_split_stack_attribute (tree *, tree, tree, int, bool *); static tree handle_fnspec_attribute (tree *, tree, tree, int, bool *); +static tree handle_always_patch_for_instrumentation_attribute (tree *, tree, + tree, int, + bool *); Move bool * to the previous line. If I do that, it goes beyond the 80 char boundary. +static tree handle_never_patch_for_instrumentation_attribute (tree *, tree, + tree, int, + bool *); + Same here. As above. static void check_function_nonnull (tree, int, tree *); static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT); static bool nonnull_check_p (tree, unsigned HOST_WIDE_INT); @@ -804,6 +811,13 @@ const struct attribute_spec c_common_attribute_table[] = The name contains space to prevent its usage in source code. */ { fn spec, 1, 1, false, true, true, handle_fnspec_attribute, false }, + { always_patch_for_instrumentation, 0, 0, true, false, false, + handle_always_patch_for_instrumentation_attribute, + false }, + { never_patch_for_instrumentation, 0, 0, true, false, false, + handle_never_patch_for_instrumentation_attribute, + false }, + { NULL, 0, 0, false, false, false, NULL, false } }; @@ -9158,6 +9172,48 @@ handle_no_thread_safety_analysis_attribute (tree *node, tree name, return NULL_TREE; } +/* Handle a always_patch_for_instrumentation attribute; arguments as in + struct attribute_spec.handler. */ Add new line here Done. +static
[google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
2012-11-05 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 (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
Re: [google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
Thanks David for the review. My comments are inline. On Sat, Nov 3, 2012 at 12:38 PM, Xinliang David Li davi...@google.com wrote: Harshit, Nov 5 is the gcc48 cutoff date. If you want to have the x-ray instrumentation feature into this release, you will need to port your patch and submit for trunk review now. I am a bit too late now, I guess. If I target for the next release, will it create any issues for the gcc48 release? On Tue, Oct 30, 2012 at 5:15 PM, Harshit Chopra hars...@google.com wrote: Adding function attributes: 'always_patch_for_instrumentation' and 'never_patch_for_instrumentation' to always patch a function or to never patch a function, respectively, when given the option -mpatch-functions-for-instrumentation. Additionally, the attribute always_patch_for_instrumentation disables inlining of that function. Tested: Tested by 'crosstool-validate.py --crosstool_ver=16 --testers=crosstool' ChangeLog: 2012-10-30 Harshit Chopra hars...@google.com * gcc/c-family/c-common.c (handle_always_patch_for_instrumentation_attribute): Handle always_patch_for_instrumentation attribute and turn inlining off for the function. (handle_never_patch_for_instrumentation_attribute): Handle never_patch_for_instrumentation attribute of a function. * gcc/config/i386/i386.c (check_should_patch_current_function): Takes into account always_patch_for_instrumentation or never_patch_for_instrumentation attribute when deciding that a function should be patched. * gcc/testsuite/gcc.target/i386/patch-functions-force-no-patching.c: Test case to test for never_patch_for_instrumentation attribute. * gcc/testsuite/gcc.target/i386/patch-functions-force-patching.c: Test case to test for always_patch_for_instrumentation attribute. * gcc/tree.h (struct GTY): Add fields for the two attributes and macros to access the fields. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index ab416ff..998645d 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -396,6 +396,13 @@ static tree ignore_attribute (tree *, tree, tree, int, bool *); static tree handle_no_split_stack_attribute (tree *, tree, tree, int, bool *); static tree handle_fnspec_attribute (tree *, tree, tree, int, bool *); +static tree handle_always_patch_for_instrumentation_attribute (tree *, tree, + tree, int, + bool *); Move bool * to the previous line. If I do that, it goes beyond the 80 char boundary. +static tree handle_never_patch_for_instrumentation_attribute (tree *, tree, + tree, int, + bool *); + Same here. As above. static void check_function_nonnull (tree, int, tree *); static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT); static bool nonnull_check_p (tree, unsigned HOST_WIDE_INT); @@ -804,6 +811,13 @@ const struct attribute_spec c_common_attribute_table[] = The name contains space to prevent its usage in source code. */ { fn spec, 1, 1, false, true, true, handle_fnspec_attribute, false }, + { always_patch_for_instrumentation, 0, 0, true, false, false, + handle_always_patch_for_instrumentation_attribute, + false }, + { never_patch_for_instrumentation, 0, 0, true, false, false, + handle_never_patch_for_instrumentation_attribute, + false }, + { NULL, 0, 0, false, false, false, NULL, false } }; @@ -9158,6 +9172,48 @@ handle_no_thread_safety_analysis_attribute (tree *node, tree name, return NULL_TREE; } +/* Handle a always_patch_for_instrumentation attribute; arguments as in + struct attribute_spec.handler. */ Add new line here Done. +static tree +handle_always_patch_for_instrumentation_attribute (tree *node, tree name, + tree ARG_UNUSED (args), + int ARG_UNUSED (flags), + bool *no_add_attrs) +{ + if (TREE_CODE (*node) == FUNCTION_DECL) +{ + DECL_FORCE_PATCHING_FOR_INSTRUMENTATION (*node) = 1; + DECL_UNINLINABLE (*node) = 1; +} + else +{ + warning (OPT_Wattributes, %qE attribute ignored, name); + *no_add_attrs = true; +} + return NULL_TREE; +} + + +/* Handle a never_patch_for_instrumentation attribute; arguments as in + struct attribute_spec.handler. */ A new line here. Done
[google] Add attributes: always_patch_for_instrumentation and never_patch_for_instrumentation (issue6821051)
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 *); +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
[google] Emit relative addresses to function patch sections instead of absolute addresses. (issue6572065)
commit fc3a55ccec9bc770c79f8a221f5abd397befc8f6 Author: Harshit Chopra hars...@google.com Date: Thu Sep 20 17:49:59 2012 -0700 Instead of emitting absolute addresses to the function patch sections, emit relative addresses. Absolute addresses might require relocation, which is time consuming and fraught with other issues. M gcc/config/i386/i386.c Tested: Ran make check-gcc and manually confirmed that the affected tests pass. ChangeLog: 2012-09-28 Harshit Chopra hars...@google.com * gcc/config/i386/i386.c (ix86_output_function_nops_prologue_epilogue): Emit relative address to function patch sections. diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index f72b0b5..8c9334f 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -11098,7 +11098,7 @@ ix86_output_function_nops_prologue_epilogue (FILE *file, $LFPEL0: pre_instruction 0x90 (repeated num_actual_nops times) - .quad $LFPESL0 + .quad $LFPESL0 - . followed by section 'section_name' which contains the address of instruction at 'label'. */ @@ -0,7 +0,10 @@ ix86_output_function_nops_prologue_epilogue (FILE *file, asm_fprintf (file, ASM_BYTE0x90\n); fprintf (file, ASM_QUAD); + /* Output section_label - . for the relative address of the entry in + the section 'section_name'. */ assemble_name_raw (file, section_label); + fprintf (file, - .); fprintf (file, \n); /* Emit the backpointer section. For functions belonging to comdat group, @@ -11144,7 +11147,7 @@ ix86_output_function_nops_prologue_epilogue (FILE *file, .quad $LFPEL0 */ ASM_OUTPUT_INTERNAL_LABEL (file, section_label); - fprintf(file, ASM_QUAD\t); + fprintf(file, ASM_QUAD); assemble_name_raw (file, label); fprintf (file, \n); -- This patch is available for review at http://codereview.appspot.com/6572065
[google] Minor cleanup and test fixes for -mpatch-functions-for-instrumentation. (issue5877043)
2012-03-21 Harshit Chopra hars...@google.com Minor changes: i386.c: made check_should_patch_current_function C90 compatible. i386.md: Added '\t' to bytes generated by ix86_output_function_nops_prologue_epilogue for proper formatting of assembly. patch-functions-*.c: Fixed verification in tests. Added a test to verify nop-bytes generated for sibling calls and another test to verify a binary with nop-bytes runs properly. * gcc/config/i386/i386.c (check_should_patch_current_function): * gcc/config/i386/i386.md: * gcc/testsuite/gcc.target/i386/patch-functions-1.c (void foo): (int main): * gcc/testsuite/gcc.target/i386/patch-functions-2.c: * gcc/testsuite/gcc.target/i386/patch-functions-3.c: * gcc/testsuite/gcc.target/i386/patch-functions-4.c: * gcc/testsuite/gcc.target/i386/patch-functions-5.c: * gcc/testsuite/gcc.target/i386/patch-functions-6.c: * gcc/testsuite/gcc.target/i386/patch-functions-7.c: * gcc/testsuite/gcc.target/i386/patch-functions-8.c (int foo): (int bar): * gcc/testsuite/gcc.target/i386/patch-functions-sibling-call.c: Testing method: make check-gcc RUNTESTFLAGS=i386.exp=patch-functions* --target_board=\unix{-m32,}\ Patch to be applied to google/main. diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 08bd5f0..be1f7a4 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -10981,6 +10981,7 @@ check_should_patch_current_function (void) const char* func_name = NULL; struct loops loops; int num_loops = 0; + int min_functions_instructions; /* Patch the function if it has at least a loop. */ if (!patch_functions_ignore_loops) @@ -11007,7 +11008,7 @@ check_should_patch_current_function (void) strcmp(main, func_name) == 0) return true; - int min_functions_instructions = + min_functions_instructions = PARAM_VALUE (PARAM_FUNCTION_PATCH_MIN_INSTRUCTIONS); if (min_functions_instructions 0) { diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index 08353ff..38a04ae 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -11688,7 +11688,7 @@ /* Emit 10 nop bytes after ret. */ if (ix86_output_function_nops_prologue_epilogue (asm_out_file, FUNCTION_PATCH_EPILOGUE_SECTION, - ret, + \tret, 10)) return ; } @@ -11712,7 +11712,7 @@ /* Emit 9 nop bytes after rep;ret. */ if (ix86_output_function_nops_prologue_epilogue (asm_out_file, FUNCTION_PATCH_EPILOGUE_SECTION, - rep\;ret, + \trep\;ret, 9)) return ; } diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-1.c b/gcc/testsuite/gcc.target/i386/patch-functions-1.c index 308e8c3..aa1f424 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-1.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-1.c @@ -1,5 +1,5 @@ /* Verify -mpatch-functions-for-instrumentation works. */ -/* { dg-do run } */ +/* { dg-do compile } */ /* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation } */ @@ -8,13 +8,16 @@ /* Check nop-bytes at end. */ /* { dg-final { scan-assembler ret(.*).byte\t0x90(.*).byte\t0x90 } } */ -void foo() { +__attribute__ ((noinline)) +void foo() +{ /* Dummy loop. */ int x = 0; while (++x); } -int main() { +int main() +{ foo(); return 0; } diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-2.c b/gcc/testsuite/gcc.target/i386/patch-functions-2.c index 6baad32..78de867 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-2.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-2.c @@ -1,4 +1,4 @@ -/* { dg-do run } */ +/* { dg-do compile } */ /* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mno-patch-functions-main-always } */ @@ -8,11 +8,14 @@ /* { dg-final { scan-assembler-not .byte\t0xeb,0x09(.*).byte\t0x90 } } */ /* { dg-final { scan-assembler-not ret(.*).byte\t0x90(.*).byte\t0x90 } } */ -void foo() { +__attribute__ ((noinline)) +void foo() +{ int x = 0; } -int main() { +int main() +{ foo(); return 0; } diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-3.c b/gcc/testsuite/gcc.target/i386/patch-functions-3.c index 49b57a8..9e8eb52 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-3.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-3.c @@ -1,4 +1,4 @@ -/* { dg-do run } */ +/* { dg-do compile
[google] Fixing function patching tests (issue5626049)
Fixes the following tests by restricting them to 64-bit target environment. Tested: Using 'make -k check-gcc RUNTESTFLAGS=i386.exp=patch* --target_board=unix\{-m32,,-m64\}' and crosstool-validate.py script. Patch to be applied to google/main branch. 2012-02-03 Harshit Chopra hars...@google.com Restricting the tests to run for 64-bit target since function patching is only supported for 64-bit targets. * gcc/testsuite/gcc.target/i386/patch-functions-1.c: * gcc/testsuite/gcc.target/i386/patch-functions-2.c: * gcc/testsuite/gcc.target/i386/patch-functions-3.c: * gcc/testsuite/gcc.target/i386/patch-functions-4.c: * gcc/testsuite/gcc.target/i386/patch-functions-5.c: * gcc/testsuite/gcc.target/i386/patch-functions-6.c: * gcc/testsuite/gcc.target/i386/patch-functions-7.c: diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-1.c b/gcc/testsuite/gcc.target/i386/patch-functions-1.c index 1d88d45..308e8c3 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-1.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-1.c @@ -1,5 +1,6 @@ /* Verify -mpatch-functions-for-instrumentation works. */ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation } */ /* Check nop-bytes at beginning. */ diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-2.c b/gcc/testsuite/gcc.target/i386/patch-functions-2.c index ed6e7ce..6baad32 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-2.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-2.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mno-patch-functions-main-always } */ /* Function is small to be instrumented with default values. Check there diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-3.c b/gcc/testsuite/gcc.target/i386/patch-functions-3.c index d7449c5..49b57a8 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-3.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-3.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation --param function-patch-min-instructions=0 } */ /* Function should have nop-bytes with -mpatch-function-min-instructions=0. diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-4.c b/gcc/testsuite/gcc.target/i386/patch-functions-4.c index f554d92..5fcbd0f 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-4.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-4.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mpatch-functions-ignore-loops -mno-patch-functions-main-always } */ /* Function is too small to be patched when ignoring the loop. diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-5.c b/gcc/testsuite/gcc.target/i386/patch-functions-5.c index bd1dafb..1f9cccf 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-5.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-5.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mpatch-functions-ignore-loops --param function-patch-min-instructions=0 } */ /* Function should be patched with nop bytes with given options. diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-6.c b/gcc/testsuite/gcc.target/i386/patch-functions-6.c index 46b715d..5bf844f 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-6.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-6.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation } */ /* 'main' function should always be patched, irrespective of how small it is. diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-7.c b/gcc/testsuite/gcc.target/i386/patch-functions-7.c index adeda59..e2c50a0 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-7.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-7.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mno-patch-functions-main-always } */ /* 'main' shouldn't be patched with the option -mno-patch-functions-main-always. -- This patch is available for review at http://codereview.appspot.com/5626049
Re: [google] Fixing function patching tests (issue5626049)
Thanks for the review. Committed to google-main. -- Harshit On Fri, Feb 3, 2012 at 4:33 PM, Xinliang David Li davi...@google.com wrote: ok for google branches. David On Fri, Feb 3, 2012 at 3:20 PM, Harshit Chopra hars...@google.com wrote: Fixes the following tests by restricting them to 64-bit target environment. Tested: Using 'make -k check-gcc RUNTESTFLAGS=i386.exp=patch* --target_board=unix\{-m32,,-m64\}' and crosstool-validate.py script. Patch to be applied to google/main branch. 2012-02-03 Harshit Chopra hars...@google.com Restricting the tests to run for 64-bit target since function patching is only supported for 64-bit targets. * gcc/testsuite/gcc.target/i386/patch-functions-1.c: * gcc/testsuite/gcc.target/i386/patch-functions-2.c: * gcc/testsuite/gcc.target/i386/patch-functions-3.c: * gcc/testsuite/gcc.target/i386/patch-functions-4.c: * gcc/testsuite/gcc.target/i386/patch-functions-5.c: * gcc/testsuite/gcc.target/i386/patch-functions-6.c: * gcc/testsuite/gcc.target/i386/patch-functions-7.c: diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-1.c b/gcc/testsuite/gcc.target/i386/patch-functions-1.c index 1d88d45..308e8c3 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-1.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-1.c @@ -1,5 +1,6 @@ /* Verify -mpatch-functions-for-instrumentation works. */ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation } */ /* Check nop-bytes at beginning. */ diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-2.c b/gcc/testsuite/gcc.target/i386/patch-functions-2.c index ed6e7ce..6baad32 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-2.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-2.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mno-patch-functions-main-always } */ /* Function is small to be instrumented with default values. Check there diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-3.c b/gcc/testsuite/gcc.target/i386/patch-functions-3.c index d7449c5..49b57a8 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-3.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-3.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation --param function-patch-min-instructions=0 } */ /* Function should have nop-bytes with -mpatch-function-min-instructions=0. diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-4.c b/gcc/testsuite/gcc.target/i386/patch-functions-4.c index f554d92..5fcbd0f 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-4.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-4.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mpatch-functions-ignore-loops -mno-patch-functions-main-always } */ /* Function is too small to be patched when ignoring the loop. diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-5.c b/gcc/testsuite/gcc.target/i386/patch-functions-5.c index bd1dafb..1f9cccf 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-5.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-5.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mpatch-functions-ignore-loops --param function-patch-min-instructions=0 } */ /* Function should be patched with nop bytes with given options. diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-6.c b/gcc/testsuite/gcc.target/i386/patch-functions-6.c index 46b715d..5bf844f 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-6.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-6.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation } */ /* 'main' function should always be patched, irrespective of how small it is. diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-7.c b/gcc/testsuite/gcc.target/i386/patch-functions-7.c index adeda59..e2c50a0 100644 --- a/gcc/testsuite/gcc.target/i386/patch-functions-7.c +++ b/gcc/testsuite/gcc.target/i386/patch-functions-7.c @@ -1,4 +1,5 @@ -/* { dg-do run} */ +/* { dg-do run } */ +/* { dg-require-effective-target lp64 } */ /* { dg-options -mpatch-functions-for-instrumentation -mno-patch-functions-main-always } */ /* 'main' shouldn't be patched with the option -mno-patch-functions-main-always. -- This patch is available for review at http://codereview.appspot.com/5626049
MAINTAINERS file diff after adding myself
Index: MAINTAINERS === --- MAINTAINERS (revision 183449) +++ MAINTAINERS (working copy) @@ -336,6 +336,7 @@ Gabriel Charette gch...@google.com Chandra Chavva ccha...@redhat.com Fabien Ch�ne fab...@gcc.gnu.org +Harshit Chopra hars...@google.com William Cohen wco...@redhat.com Josh Conner jcon...@apple.com R. Kelley Cook kc...@gcc.gnu.org
Re: [google] Patch to enable efficient function level instrumentation
Ping! -- Harshit
Patch to enable efficient function level instrumentation (issue5416043)
From f63ec02c0a720174489fe450b3cc43eb00fd4bdd Mon Sep 17 00:00:00 2001 From: Harshit Chopra hars...@google.com Date: Thu, 3 Nov 2011 17:29:23 -0700 Subject: [PATCH] Mechanism to provide efficient function instrumentation (x86-64) Summary: This patch aims at providing an efficient way to instrument a binary at function level for x86-64 architecture. Existing mechanisms (like -finstrument-functions) are expensive, especially when not instrumenting, which calls for the need of separate binaries for deployment and debugging for long running systems. This patch aims to remove this need of two separate binaries. Detail: This patch adds a mechanism to insert a sequence of Nop bytes at function entry and at function exits, which are used to instrument a binary at function level by manipulating these nops at runtime. Emission of these nop bytes is controlled by the flag -mpatch-functions-for-instrumentation and other related flags. This patch only adds support for binaries for x86-64 architecture. For each function in the binary, 11-bytes of nops are added at the entry and 10-bytes of nops at exit points (just after the return). For functions which make a tail call to another function, 11-bytes of nops are added just before the tail call jump. These nops can be patched at runtime to call appropriate instrumentation routines for function entry and exit. The format of 11 bytes of nops (at function prologue and before tail call jump) is: L0: jmp L1 XX .quad L2 .section function_patch_section_name L2: .quad L0 .text L1: ... (function code) The section 'function_patch_section_name' is used to store a back pointer to these nops. If the nops belong to function prologue, the name of this section is _function_patch_prologue, or else if the nops belong to function exit, the name is _function_patch_epilogue. The 'XX' bytes can be anything (0x00-0xff) since they are dead bytes and act as fillers. Similarly at function exit (just after ret instruction), the format of the 10-bytes of nops is: ... L0: ret XX XX .quad L1 .section _function_patch_epilogue L1: .quad L0 In order to handle functions belonging to COMDAT group, the name of the function patch section is function_patch_section_name.function_decl_section. Such sections are later renamed to function_patch_section_name just before emitting the assembly by the routine ix86_elf_asm_named_section(). Conceptually, these nops can be replaced by the following instructions at runtime: mov func_id, %r10d call InstrumentationFunction The 'mov' being a 6 byte instruction and the call being a 5-byte instruction, a total of 11-bytes are needed for patching. Hence, the need for 11-bytes of nops. 10-bytes of nops are needed after the 'ret' instruction since the return is also patched and the call replaced by a jump to InstrumentationFunction (similar to a tail call). To control emission of these nops globally and on a per-function basis (somewhat), the following flags are provided: * -mpatch-functions-for-instrumentation: Master flag to control emission of these nops. The rest of the flags below have no effect if this flag is not provided. * -mpatch-functions-min-instructions: If provided, only those functions having number of instructions greater than this flag value will have these nops at prologue and exit. Default is 200. * -mpatch-functions-ignore-loops: To ignore loops when deciding whether to have these nops for a function. By default, functions having loops always include these nops. * -mno-patch-functions-main-always: The 'main' function always has these nops to have the ability to rely on a function to call initialize instrumentation code. This flag treats main function as any other function. Some issues with this patch: * This patch prohibits garbage collection of dead functions at link time since there are pointers from the sections _function_patch_prologue and _function_patch_epilogue to the dead function. * The existence of random data bytes in the dead code part of these nops in the middle of text section confuses disassembling tools like objdump. Suggestions/comments for this patch are welcome. Also, suggestions on dealing with the above issues will be really helpful. Testing done: Wrote tests to test code generation for the different cases and then ran 'make -k check-gcc' Patch to be applied to google/main and trunk. --- gcc/config/i386/i386-protos.h |4 + gcc/config/i386/i386.c| 199 + gcc/config/i386/i386.md | 26 +++- gcc/config/i386/i386.opt | 16 ++ gcc/testsuite/gcc.target/i386/patch-functions-1.c | 14 ++ gcc/testsuite/gcc.target/i386/patch-functions-2.c | 12 ++ gcc/testsuite/gcc.target/i386/patch-functions-3.c | 12 ++ gcc/testsuite/gcc.target/i386