Issue 5416043: [google] Patch to enable efficient function level instrumentation
|
Created:
13 years, 4 months ago by harshit Modified:
13 years, 2 months ago CC:
gcc-patches_gcc.gnu.org Visibility:
Public. |
Patch Set 1 #
Total comments: 29
Patch Set 2 : Addressed comments from David for the original patch #
Total comments: 1
Patch Set 3 : Incorporating David's second round of comments #
Total comments: 2
Patch Set 4 : As suggested by David, updated comment on generation of backpointer section for comdat functions. #Patch Set 5 : Fixing broken tests after last patch update #Patch Set 6 : Updated tests to make them runnable #
MessagesTotal messages: 18
From f63ec02c0a720174489fe450b3cc43eb00fd4bdd Mon Sep 17 00:00:00 2001 From: Harshit Chopra <harshit@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/patch-functions-4.c | 13 ++ gcc/testsuite/gcc.target/i386/patch-functions-5.c | 13 ++ gcc/testsuite/gcc.target/i386/patch-functions-6.c | 13 ++ gcc/testsuite/gcc.target/i386/patch-functions-7.c | 13 ++ 11 files changed, 333 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.target/i386/patch-functions-1.c create mode 100644 gcc/testsuite/gcc.target/i386/patch-functions-2.c create mode 100644 gcc/testsuite/gcc.target/i386/patch-functions-3.c create mode 100644 gcc/testsuite/gcc.target/i386/patch-functions-4.c create mode 100644 gcc/testsuite/gcc.target/i386/patch-functions-5.c create mode 100644 gcc/testsuite/gcc.target/i386/patch-functions-6.c create mode 100644 gcc/testsuite/gcc.target/i386/patch-functions-7.c diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h index 900d1c5..f857bfd 100644 --- a/gcc/config/i386/i386-protos.h +++ b/gcc/config/i386/i386-protos.h @@ -29,6 +29,10 @@ extern bool ix86_handle_option (struct gcc_options *opts, extern bool ix86_target_stack_probe (void); extern bool ix86_can_use_return_insn_p (void); extern void ix86_setup_frame_addresses (void); +extern bool ix86_output_function_nops_prologue_epilogue (FILE *, + const char *, + const char *, + unsigned int); extern HOST_WIDE_INT ix86_initial_elimination_offset (int, int); extern void ix86_expand_prologue (void); diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 38fea4e..e572040 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -60,6 +60,7 @@ along with GCC; see the file COPYING3. If not see #include "fibheap.h" #include "opts.h" #include "diagnostic.h" +#include "cfgloop.h" enum upper_128bits_state { @@ -10792,6 +10793,190 @@ ix86_expand_epilogue (int style) m->fs = frame_state_save; } +/* True if the current function should be patched with nops at prologue and + returns. */ +static bool patch_current_function_p = false; + +/* Return true if we patch the current function. */ +static bool +check_should_patch_current_function (void) +{ + int num_insns = 0; + rtx insn; + const char* func_name = NULL; + struct loops loops; + int num_loops = 0; + + /* Patch the function if it has at least a loop. */ + if (!patch_functions_ignore_loops) + { + if (DECL_STRUCT_FUNCTION (current_function_decl)->cfg) + { + num_loops = flow_loops_find (&loops); + /* FIXME - Deallocating the loop causes a seg-fault. */ +#if 0 + flow_loops_free (&loops); +#endif + /* We are not concerned with the function body as a loop. */ + if (num_loops > 1) + return true; + } + } + + /* Borrowed this code from rest_of_handle_final() in final.c. */ + func_name = XSTR (XEXP (DECL_RTL (current_function_decl), 0), 0); + if (!patch_functions_dont_always_patch_main && + func_name && + strcmp("main", func_name) == 0) + return true; + + if (patch_functions_min_instructions > 0) + { + /* Calculate the number of instructions in this function and only emit + function patch for instrumentation if it is greater than + patch_functions_min_instructions. */ + for (insn = get_insns (); insn; insn = NEXT_INSN (insn)) + { + if (INSN_P (insn)) + ++num_insns; + } + if (num_insns < patch_functions_min_instructions) + return false; + } + + return true; +} + +/* Emit the 11-byte patch space for the function prologue for functions that + qualify. */ +static void +ix86_output_function_prologue (FILE *file, + HOST_WIDE_INT size ATTRIBUTE_UNUSED) +{ + /* Only for 64-bit target. */ + if (TARGET_64BIT && patch_functions_for_instrumentation) + { + patch_current_function_p = check_should_patch_current_function(); + /* Emit the instruction 'jmp 09' followed by 9 bytes to make it 11-bytes + of nop. */ + ix86_output_function_nops_prologue_epilogue (file, + "_function_patch_prologue", + ASM_BYTE"0xeb,0x09", + 9); + } +} + +/* Emit the nop bytes at function prologue or return (including tail call + jumps). The number of nop bytes generated is at least 8. + Also emits a section named SECTION_NAME, which is a backpointer section + holding the addresses of the nop bytes in the text section. + SECTION_NAME is either '_function_patch_prologue' or + '_function_patch_epilogue'. + PRE_INSTRUCTIONS are the instructions, if any, at the start of the nop byte + sequence. NUM_REMAINING_NOPS are the number of nop bytes to fill, + excluding the number of bytes in PRE_INSTRUCTIONS. + Returns true if the function was patched, false otherwise. */ +bool +ix86_output_function_nops_prologue_epilogue (FILE *file, + const char *section_name, + const char *pre_instructions, + unsigned int num_remaining_nops) +{ + static int labelno = 0; + char label[32], section_label[32]; + section *section = NULL; + unsigned int num_actual_nops = num_remaining_nops - 8; + unsigned int section_flags = SECTION_RELRO; + char *section_name_comdat = NULL; + const char *decl_section_name = NULL; + size_t len; + + gcc_assert (num_remaining_nops >= 8); + + if (!patch_current_function_p) + return false; + + ASM_GENERATE_INTERNAL_LABEL (label, "LFPEL", labelno); + ASM_GENERATE_INTERNAL_LABEL (section_label, "LFPESL", labelno++); + + /* Align the start of nops to 2-byte boundary so that the 2-byte jump + instruction can be patched atomically at run time. */ + ASM_OUTPUT_ALIGN (file, 1); + + /* Emit nop bytes. They look like the following: + $LFPEL0: + <pre_instruction> + 0x90 (repeated num_actual_nops times) + .quad $LFPESL0 + followed by section 'section_name' which contains the address + of instruction at 'label'. + */ + ASM_OUTPUT_INTERNAL_LABEL (file, label); + if (pre_instructions) + fprintf (file, "%s\n", pre_instructions); + + while (num_actual_nops-- > 0) + asm_fprintf (file, ASM_BYTE"0x90\n"); + + fprintf (file, ASM_QUAD); + assemble_name_raw (file, section_label); + fprintf (file, "\n"); + + /* Emit the backpointer section. For functions belonging to comdat group, + we emit a different section named '<section_name>.foo'. This section + is later renamed to '<section_name>' by ix86_elf_asm_named_section(). */ + if (current_function_decl != NULL_TREE && + DECL_ONE_ONLY (current_function_decl) && + HAVE_COMDAT_GROUP) + { + decl_section_name = + TREE_STRING_POINTER (DECL_SECTION_NAME (current_function_decl)); + len = strlen (decl_section_name) + strlen (section_name) + 1; + 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; + } + section = get_section (section_name, section_flags, current_function_decl); + switch_to_section (section); + /* Align the section to 8-byte boundary. */ + ASM_OUTPUT_ALIGN (file, 3); + + /* Emit address of the start of nop bytes in the section: + $LFPESP0: + .quad $LFPEL0 + */ + ASM_OUTPUT_INTERNAL_LABEL (file, section_label); + fprintf(file, ASM_QUAD"\t"); + assemble_name_raw (file, label); + fprintf (file, "\n"); + + /* Switching back to text section. */ + switch_to_section (function_section (current_function_decl)); + return true; +} + +/* Strips the characters after '_function_patch_prologue' or + '_function_patch_epilogue' and emits the section. */ +static void +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) + { + /* Both section names have the same length. */ + static const int section_name_length = + sizeof("_function_patch_prologue") - 1; + if (strncmp (name, "_function_patch_prologue", section_name_length) == 0) + section_name = "_function_patch_prologue"; + else if (strncmp (name, "_function_patch_epilogue", + section_name_length) == 0) + section_name = "_function_patch_epilogue"; + } + default_elf_asm_named_section (section_name, flags, decl); +} + /* Reset from the function's potential modifications. */ static void @@ -22164,6 +22349,14 @@ ix86_output_call_insn (rtx insn, rtx call_op) else xasm = "jmp\t%A0"; + /* Just before the sibling call, add 11-bytes of nops to patch function + exit: 2 bytes for 'jmp 09' and remaining 9 bytes. */ + if (TARGET_64BIT && patch_functions_for_instrumentation) + ix86_output_function_nops_prologue_epilogue (asm_out_file, + "_function_patch_epilogue", + ASM_BYTE"0xeb, 0x09", + 9); + output_asm_insn (xasm, &call_op); return ""; } @@ -36213,9 +36406,15 @@ ix86_autovectorize_vector_sizes (void) #undef TARGET_BUILTIN_RECIPROCAL #define TARGET_BUILTIN_RECIPROCAL ix86_builtin_reciprocal +#undef TARGET_ASM_FUNCTION_PROLOGUE +#define TARGET_ASM_FUNCTION_PROLOGUE ix86_output_function_prologue + #undef TARGET_ASM_FUNCTION_EPILOGUE #define TARGET_ASM_FUNCTION_EPILOGUE ix86_output_function_epilogue +#undef TARGET_ASM_NAMED_SECTION +#define TARGET_ASM_NAMED_SECTION ix86_elf_asm_named_section + #undef TARGET_ENCODE_SECTION_INFO #ifndef SUBTARGET_ENCODE_SECTION_INFO #define TARGET_ENCODE_SECTION_INFO ix86_encode_section_info diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index 52c57fa..35b943a 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -11676,7 +11676,18 @@ (define_insn "return_internal" [(return)] "reload_completed" - "ret" +{ + if (TARGET_64BIT && patch_functions_for_instrumentation) + { + /* Emit 10 nop bytes after ret. */ + if (ix86_output_function_nops_prologue_epilogue (asm_out_file, + "_function_patch_epilogue", + "ret", + 10)) + return ""; + } + return "ret"; +} [(set_attr "length" "1") (set_attr "atom_unit" "jeu") (set_attr "length_immediate" "0") @@ -11689,7 +11700,18 @@ [(return) (unspec [(const_int 0)] UNSPEC_REP)] "reload_completed" - "rep\;ret" +{ + if (TARGET_64BIT && patch_functions_for_instrumentation) + { + /* Emit 9 nop bytes after rep;ret. */ + if (ix86_output_function_nops_prologue_epilogue (asm_out_file, + "_function_patch_epilogue", + "rep\;ret", + 9)) + return ""; + } + return "rep\;ret"; +} [(set_attr "length" "2") (set_attr "atom_unit" "jeu") (set_attr "length_immediate" "0") diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt index 8e4d51b..85228cf 100644 --- a/gcc/config/i386/i386.opt +++ b/gcc/config/i386/i386.opt @@ -560,3 +560,19 @@ Split 32-byte AVX unaligned load mavx256-split-unaligned-store Target Report Mask(AVX256_SPLIT_UNALIGNED_STORE) Save Split 32-byte AVX unaligned store + +mpatch-functions-for-instrumentation +Target RejectNegative Report Var(patch_functions_for_instrumentation) Save +Patch function prologue and epilogue with custom NOPs for dynamic instrumentation. By default, functions with loops (controlled by -mpatch-functions-without-loop) or functions having instructions more than -mpatch-functions-min-instructions are patched. + +mpatch-functions-min-instructions= +Target Report Joined UInteger Var(patch_functions_min_instructions) Init(200) Save +Minimum number of instructions in the function without loop before the function is qualified for patching for instrumentation (for use with -mpatch-functions-for-instrumentation) + +mpatch-functions-ignore-loops +Target RejectNegative Report Var(patch_functions_ignore_loops) Save +Ignore loops when deciding whether to patch a function for instrumentation (for use with -mpatch-functions-for-instrumentation). + +mno-patch-functions-main-always +Target Report RejectNegative Var(patch_functions_dont_always_patch_main) Save +Treat 'main' as any other function and only patch it if it meets the criteria for loops and minimum number of instructions (for use with -mpatch-functions-for-instrumentation). diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-1.c b/gcc/testsuite/gcc.target/i386/patch-functions-1.c new file mode 100644 index 0000000..9f11945 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-1.c @@ -0,0 +1,14 @@ +/* Verify -mpatch-functions-for-instrumentation works. */ +/* { dg-do compile} */ +/* { dg-options "-mpatch-functions-for-instrumentation" } */ + +/* Check nop-bytes at beginning. */ +/* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* Check nop-bytes at end. */ +/* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +void foo() { + /* Dummy loop. */ + int x = 0; + while (++x); +} diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-2.c b/gcc/testsuite/gcc.target/i386/patch-functions-2.c new file mode 100644 index 0000000..86c0ab7 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-2.c @@ -0,0 +1,12 @@ +/* { dg-do compile} */ +/* { dg-options "-mpatch-functions-for-instrumentation" } */ + +/* Function is small to be instrumented with default values. Check there + aren't any nop-bytes at beginning or end of function. */ + +/* { dg-final { scan-assembler-not ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler-not "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +void foo() { + int x = 0; +} diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-3.c b/gcc/testsuite/gcc.target/i386/patch-functions-3.c new file mode 100644 index 0000000..54cc625 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-3.c @@ -0,0 +1,12 @@ +/* { dg-do compile} */ +/* { dg-options "-mpatch-functions-for-instrumentation -mpatch-functions-min-instructions=0" } */ + +/* Function should have nop-bytes with -mpatch-function-min-instructions=0. + Check there are nop-bytes at beginning and end of function. */ + +/* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +void foo() { + int x = 0; +} diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-4.c b/gcc/testsuite/gcc.target/i386/patch-functions-4.c new file mode 100644 index 0000000..a0d8f18 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-4.c @@ -0,0 +1,13 @@ +/* { dg-do compile} */ +/* { dg-options "-mpatch-functions-for-instrumentation -mpatch-functions-ignore-loops" } */ + +/* Function is too small to be patched when ignoring the loop. + Check there aren't any nop-bytes at beginning and end of function. */ + +/* { dg-final { scan-assembler-not ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler-not "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +void foo() { + int x = 0; + while (++x); +} diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-5.c b/gcc/testsuite/gcc.target/i386/patch-functions-5.c new file mode 100644 index 0000000..8ac8bae --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-5.c @@ -0,0 +1,13 @@ +/* { dg-do compile} */ +/* { dg-options "-mpatch-functions-for-instrumentation -mpatch-functions-ignore-loops -mpatch-functions-min-instructions=0" } */ + +/* Function should be patched with nop bytes with given options. + Check there are nop-bytes at beginning and end of function. */ + +/* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +void foo() { + int x = 0; + while (++x); +} diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-6.c b/gcc/testsuite/gcc.target/i386/patch-functions-6.c new file mode 100644 index 0000000..2af551a --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-6.c @@ -0,0 +1,13 @@ +/* { dg-do compile} */ +/* { dg-options "-mpatch-functions-for-instrumentation" } */ + +/* 'main' function should always be patched, irrespective of how small it is. + Check there are nop-bytes at beginning and end of main. */ + +/* { dg-final { scan-assembler ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +int main(int argc, char **argv) { + int x = 0; + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/patch-functions-7.c b/gcc/testsuite/gcc.target/i386/patch-functions-7.c new file mode 100644 index 0000000..572a00f --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/patch-functions-7.c @@ -0,0 +1,13 @@ +/* { dg-do compile} */ +/* { dg-options "-mpatch-functions-for-instrumentation -mno-patch-functions-main-always" } */ + +/* 'main' shouldn't be patched with the option -mno-patch-functions-main-always. + Check there aren't any nop-bytes at beginning and end of main. */ + +/* { dg-final { scan-assembler-not ".byte\t0xeb,0x09(.*).byte\t0x90" } } */ +/* { dg-final { scan-assembler-not "ret(.*).byte\t0x90(.*).byte\t0x90" } } */ + +int main(int argc, char **argv) { + int x = 0; + return 0; +} -- 1.7.3.1 -- This patch is available for review at http://codereview.appspot.com/5416043
Sign in to reply to this message.
Ping!
Sign in to reply to this message.
Ping!
Sign in to reply to this message.
Please also explain the need for backpointer section. David http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10801 gcc/config/i386/i386.c:10801: +static bool Add an empty line between comment and function. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10811 gcc/config/i386/i386.c:10811: + if (!patch_functions_ignore_loops) What exactly does this option try to do here? http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10822 gcc/config/i386/i386.c:10822: + return true; should it have a an else return false here? http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10841 gcc/config/i386/i386.c:10841: + ++num_insns; NON_DEBUG_INSN_P (insn) http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10859 gcc/config/i386/i386.c:10859: + patch_current_function_p = check_should_patch_current_function(); Why do you need a static variable for this? http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10874 gcc/config/i386/i386.c:10874: + '_function_patch_epilogue'. Explain why a backpointer section is needed. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10888 gcc/config/i386/i386.c:10888: + unsigned int num_actual_nops = num_remaining_nops - 8; hard code of 8? http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10897 gcc/config/i386/i386.c:10897: + return false; Can this be guarded in the caller of this function? http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' by ix86_elf_asm_named_section(). */ Explain more on the comdat handling. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt File gcc/config/i386/i386.opt (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode570 gcc/config/i386/i386.opt:570: Minimum number of instructions in the function without loop before the function is qualified for patching for instrumentation (for use with -mpatch-functions-for-instrumentation) It may be better to define PARAM for it. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode574 gcc/config/i386/i386.opt:574: Ignore loops when deciding whether to patch a function for instrumentation (for use with -mpatch-functions-for-instrumentation). What is the motivation for this option? http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode578 gcc/config/i386/i386.opt:578: Treat 'main' as any other function and only patch it if it meets the criteria for loops and minimum number of instructions (for use with -mpatch-functions-for-instrumentation). What is the motivation for this option?
Sign in to reply to this message.
Thanks David for the review. Addressed your comments. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10801 gcc/config/i386/i386.c:10801: +static bool On 2011/11/28 22:16:27, davidxl wrote: > Add an empty line between comment and function. Done. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10811 gcc/config/i386/i386.c:10811: + if (!patch_functions_ignore_loops) On 2011/11/28 22:16:27, davidxl wrote: > What exactly does this option try to do here? By default a function is patched only if it has loops or else it should have more than patch_functions_min_instructions insns (which roughly translates to that many instructions). This flag controls if checking for loops should be considered when deciding a function should be patched or not. Explaining this as a comment for the function. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10822 gcc/config/i386/i386.c:10822: + return true; On 2011/11/28 22:16:27, davidxl wrote: > should it have a an else return false here? No, since otherwise it also needs to check the number of instructions in the function to decide if the function is worth patching. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10841 gcc/config/i386/i386.c:10841: + ++num_insns; On 2011/11/28 22:16:27, davidxl wrote: > NON_DEBUG_INSN_P (insn) Done. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10859 gcc/config/i386/i386.c:10859: + patch_current_function_p = check_should_patch_current_function(); On 2011/11/28 22:16:27, davidxl wrote: > Why do you need a static variable for this? Because the cached value is needed to decide generation of nops for function exits. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10874 gcc/config/i386/i386.c:10874: + '_function_patch_epilogue'. On 2011/11/28 22:16:27, davidxl wrote: > Explain why a backpointer section is needed. Done. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10888 gcc/config/i386/i386.c:10888: + unsigned int num_actual_nops = num_remaining_nops - 8; On 2011/11/28 22:16:27, davidxl wrote: > hard code of 8? Changed to sizeof(void*) http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10897 gcc/config/i386/i386.c:10897: + return false; On 2011/11/28 22:16:27, davidxl wrote: > Can this be guarded in the caller of this function? For function exits, this function is called from a bunch of places. And I didn't want to duplicate it for no good reason. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' by ix86_elf_asm_named_section(). */ On 2011/11/28 22:16:27, davidxl wrote: > Explain more on the comdat handling. I have limited knowledge about comdat sections, so can't give a detailed explanation on why the assembler emits an error. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt File gcc/config/i386/i386.opt (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode570 gcc/config/i386/i386.opt:570: Minimum number of instructions in the function without loop before the function is qualified for patching for instrumentation (for use with -mpatch-functions-for-instrumentation) On 2011/11/28 22:16:27, davidxl wrote: > It may be better to define PARAM for it. Param for this option? How to do it in options file? I looked at the docs, but didn't find anything on it. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode574 gcc/config/i386/i386.opt:574: Ignore loops when deciding whether to patch a function for instrumentation (for use with -mpatch-functions-for-instrumentation). On 2011/11/28 22:16:27, davidxl wrote: > What is the motivation for this option? To patch every function, we need to bypass the loop check when deciding to patch a function or not. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode578 gcc/config/i386/i386.opt:578: Treat 'main' as any other function and only patch it if it meets the criteria for loops and minimum number of instructions (for use with -mpatch-functions-for-instrumentation). On 2011/11/28 22:16:27, davidxl wrote: > What is the motivation for this option? We don't have a use for this. But I added it in case someone doesn't want to special case main.
Sign in to reply to this message.
Have you uploaded the revised patch? David http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' by ix86_elf_asm_named_section(). */ On 2011/12/02 01:57:17, harshit wrote: > On 2011/11/28 22:16:27, davidxl wrote: > > Explain more on the comdat handling. > > I have limited knowledge about comdat sections, so can't give a detailed > explanation on why the assembler emits an error. What does the assembler error look like? An example file would be helpful. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10970 gcc/config/i386/i386.c:10970: + sizeof("_function_patch_prologue") - 1; Define a macro for the prologue and epilogue name. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10974 gcc/config/i386/i386.c:10974: + section_name_length) == 0) The two section name length happen to be the same, but it is not good to share the same value here. sizeof (..) will be evaluated at compile time. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt File gcc/config/i386/i386.opt (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode570 gcc/config/i386/i386.opt:570: Minimum number of instructions in the function without loop before the function is qualified for patching for instrumentation (for use with -mpatch-functions-for-instrumentation) in params.def On 2011/12/02 01:57:17, harshit wrote: > On 2011/11/28 22:16:27, davidxl wrote: > > It may be better to define PARAM for it. > > Param for this option? How to do it in options file? I looked at the docs, but > didn't find anything on it.
Sign in to reply to this message.
On Fri, Dec 2, 2011 at 11:08 AM, <davidxl@google.com> wrote: > Have you uploaded the revised patch? > I uploaded to Reitveld, but it didn't go through to gcc-patches. > > David > > > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/**i386/i386.c<http:... > File gcc/config/i386/i386.c (right): > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.c#newcode10927<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927> > gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' > by ix86_elf_asm_named_section(). */ > On 2011/12/02 01:57:17, harshit wrote: > >> On 2011/11/28 22:16:27, davidxl wrote: >> > Explain more on the comdat handling. >> > > I have limited knowledge about comdat sections, so can't give a >> > detailed > >> explanation on why the assembler emits an error. >> > > What does the assembler error look like? An example file would be > helpful. > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.c#newcode10970<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10970> > gcc/config/i386/i386.c:10970: + > sizeof("_function_patch_**prologue") - 1; > Define a macro for the prologue and epilogue name. > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.c#newcode10974<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10974> > gcc/config/i386/i386.c:10974: + > section_name_length) == 0) > The two section name length happen to be the same, but it is not good to > share the same value here. sizeof (..) will be evaluated at compile > time. > > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/**i386/i386.opt<htt... > File gcc/config/i386/i386.opt (right): > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.opt#newcode570<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode570> > gcc/config/i386/i386.opt:570: Minimum number of instructions in the > function without loop before the function is qualified for patching for > instrumentation (for use with -mpatch-functions-for-**instrumentation) > in params.def > > > On 2011/12/02 01:57:17, harshit wrote: > >> On 2011/11/28 22:16:27, davidxl wrote: >> > It may be better to define PARAM for it. >> > > Param for this option? How to do it in options file? I looked at the >> > docs, but > >> didn't find anything on it. >> > > http://codereview.appspot.com/**5416043/<http://codereview.appspot.com/5416043/> >
Sign in to reply to this message.
http://codereview.appspot.com/5416043/diff/6001/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5416043/diff/6001/gcc/config/i386/i386.c#newcod... gcc/config/i386/i386.c:10881: + '_function_patch_epilogue'. The backpointer section can be used to navigate Is it strictly necessary? If most of the functions are instrumented, can the runtime instrumenter just recognize functions need patching via pattern matching -- as least for function prologue?
Sign in to reply to this message.
On Fri, Dec 2, 2011 at 11:08 AM, <davidxl@google.com> wrote: > Have you uploaded the revised patch? > > David > > > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/**i386/i386.c<http:... > File gcc/config/i386/i386.c (right): > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.c#newcode10927<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927> > gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' > by ix86_elf_asm_named_section(). */ > On 2011/12/02 01:57:17, harshit wrote: > >> On 2011/11/28 22:16:27, davidxl wrote: >> > Explain more on the comdat handling. >> > > I have limited knowledge about comdat sections, so can't give a >> > detailed > >> explanation on why the assembler emits an error. >> > > What does the assembler error look like? An example file would be > helpful. > It is actually a linker warning something like: third_party/crosstool/v15_function_patch/gcc/x86/bin/ld: blaze-out/gcc-4.6.x-glibc-2.11.1-grte-k8-fastbuild/bin/base/_objs/syslog_util/base/syslog_util.pic.o:(_function_patch_epilogue+0xa0): warning: relocation refers to discarded section > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.c#newcode10970<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10970> > gcc/config/i386/i386.c:10970: + > sizeof("_function_patch_**prologue") - 1; > Define a macro for the prologue and epilogue name. > Done. > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.c#newcode10974<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10974> > gcc/config/i386/i386.c:10974: + > section_name_length) == 0) > The two section name length happen to be the same, but it is not good to > share the same value here. sizeof (..) will be evaluated at compile > time. Changed. > > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/**i386/i386.opt<htt... > File gcc/config/i386/i386.opt (right): > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.opt#newcode570<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode570> > gcc/config/i386/i386.opt:570: Minimum number of instructions in the > function without loop before the function is qualified for patching for > instrumentation (for use with -mpatch-functions-for-**instrumentation) > in params.def Added a param for this in params.def > > > On 2011/12/02 01:57:17, harshit wrote: > >> On 2011/11/28 22:16:27, davidxl wrote: >> > It may be better to define PARAM for it. >> > > Param for this option? How to do it in options file? I looked at the >> > docs, but > >> didn't find anything on it. >> > > http://codereview.appspot.com/**5416043/<http://codereview.appspot.com/5416043/> >
Sign in to reply to this message.
http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c File gcc/config/i386/i386.c (left): http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c#oldco... gcc/config/i386/i386.c:10928: if (current_function_decl != NULL_TREE && I am not sure how the hack you have here makes the linker warning go away (and besides the section name suffix will be stripped right after it is set when switch_section is called). The right solution might be to set the comdat group of the label address section to be the same as the group of the comdat function. Cary, what is your opinion?
Sign in to reply to this message.
http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c File gcc/config/i386/i386.c (left): http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c#oldco... gcc/config/i386/i386.c:10928: if (current_function_decl != NULL_TREE && If I understand correctly, this does arrange for the backpointer section to go into a comdat section with the same group key as the function. We have to give the section a unique name within GCC, however, in order to keep it from combining with the non-COMDAT section that's being used for the non-COMDAT functions. Later, in ix86_elf_asm_named_section, we can strip off the suffix so that it gets the right name in the ELF file.
Sign in to reply to this message.
Ok, Cary's explanation makes sense. Please update the comments to make it clearer. http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c File gcc/config/i386/i386.c (right): http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' by ix86_elf_asm_named_section(). */ Probably better to change the comment to something like -- we emit a unique section name for the back pointer section. This is needed because otherwise the 'get_section' call may return an existing non-comdat section with the same name, leading to references from non-comdat section to comdat functions.
Sign in to reply to this message.
On Fri, Dec 16, 2011 at 3:10 PM, <davidxl@google.com> wrote: > Ok, Cary's explanation makes sense. Please update the comments to make > it clearer. > > > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/**i386/i386.c<http:... > File gcc/config/i386/i386.c (right): > > http://codereview.appspot.com/**5416043/diff/1/gcc/config/** > i386/i386.c#newcode10927<http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927> > gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' > by ix86_elf_asm_named_section(). */ > Probably better to change the comment to something like -- we emit a > unique section name for the back pointer section. This is needed because > otherwise the 'get_section' call may return an existing non-comdat > section with the same name, leading to references from non-comdat > section to comdat functions. > Updated comment as suggested. > > http://codereview.appspot.com/**5416043/<http://codereview.appspot.com/5416043/> >
Sign in to reply to this message.
Ok for google branches when tests are done. Update ChangeLog file properly. David On Tue, Dec 20, 2011 at 1:55 PM, Harshit Chopra <harshit@google.com> wrote: > > > On Fri, Dec 16, 2011 at 3:10 PM, <davidxl@google.com> wrote: >> >> Ok, Cary's explanation makes sense. Please update the comments to make >> it clearer. >> >> >> >> http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c >> File gcc/config/i386/i386.c (right): >> >> >> http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 >> gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' >> by ix86_elf_asm_named_section(). */ >> Probably better to change the comment to something like -- we emit a >> unique section name for the back pointer section. This is needed because >> otherwise the 'get_section' call may return an existing non-comdat >> section with the same name, leading to references from non-comdat >> section to comdat functions. > > > Updated comment as suggested. > >> >> >> http://codereview.appspot.com/5416043/ > >
Sign in to reply to this message.
Updated the tests so that they are runnable. Uploaded the modified patch. FYI, I don't have permission to submit the patch. Is it possible for someone with write permission to submit the patch? Thanks, Harshit On Tue, Dec 27, 2011 at 10:34 AM, Xinliang David Li <davidxl@google.com>wrote: > Ok for google branches when tests are done. Update ChangeLog file properly. > > David > > On Tue, Dec 20, 2011 at 1:55 PM, Harshit Chopra <harshit@google.com> > wrote: > > > > > > On Fri, Dec 16, 2011 at 3:10 PM, <davidxl@google.com> wrote: > >> > >> Ok, Cary's explanation makes sense. Please update the comments to make > >> it clearer. > >> > >> > >> > >> http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c > >> File gcc/config/i386/i386.c (right): > >> > >> > >> > http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 > >> gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' > >> by ix86_elf_asm_named_section(). */ > >> Probably better to change the comment to something like -- we emit a > >> unique section name for the back pointer section. This is needed because > >> otherwise the 'get_section' call may return an existing non-comdat > >> section with the same name, leading to references from non-comdat > >> section to comdat functions. > > > > > > Updated comment as suggested. > > > >> > >> > >> http://codereview.appspot.com/5416043/ > > > > >
Sign in to reply to this message.
By google branches, do you mean google-main and google-integration, or just google-main? -- Harshit On Tue, Dec 27, 2011 at 10:34 AM, Xinliang David Li <davidxl@google.com>wrote: > Ok for google branches when tests are done. Update ChangeLog file properly. > > David > > On Tue, Dec 20, 2011 at 1:55 PM, Harshit Chopra <harshit@google.com> > wrote: > > > > > > On Fri, Dec 16, 2011 at 3:10 PM, <davidxl@google.com> wrote: > >> > >> Ok, Cary's explanation makes sense. Please update the comments to make > >> it clearer. > >> > >> > >> > >> http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c > >> File gcc/config/i386/i386.c (right): > >> > >> > >> > http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 > >> gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' > >> by ix86_elf_asm_named_section(). */ > >> Probably better to change the comment to something like -- we emit a > >> unique section name for the back pointer section. This is needed because > >> otherwise the 'get_section' call may return an existing non-comdat > >> section with the same name, leading to references from non-comdat > >> section to comdat functions. > > > > > > Updated comment as suggested. > > > >> > >> > >> http://codereview.appspot.com/5416043/ > > > > >
Sign in to reply to this message.
Updated gcc/ChangeLog.google-main file and committed the change to google-main branch in svn. Thanks David for the review and Cary for helping me write the patch. -- Harshit On Tue, Dec 27, 2011 at 10:34 AM, Xinliang David Li <davidxl@google.com> wrote: > Ok for google branches when tests are done. Update ChangeLog file properly. > > David > > On Tue, Dec 20, 2011 at 1:55 PM, Harshit Chopra <harshit@google.com> wrote: >> >> >> On Fri, Dec 16, 2011 at 3:10 PM, <davidxl@google.com> wrote: >>> >>> Ok, Cary's explanation makes sense. Please update the comments to make >>> it clearer. >>> >>> >>> >>> http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c >>> File gcc/config/i386/i386.c (right): >>> >>> >>> http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927 >>> gcc/config/i386/i386.c:10927: + is later renamed to '<section_name>' >>> by ix86_elf_asm_named_section(). */ >>> Probably better to change the comment to something like -- we emit a >>> unique section name for the back pointer section. This is needed because >>> otherwise the 'get_section' call may return an existing non-comdat >>> section with the same name, leading to references from non-comdat >>> section to comdat functions. >> >> >> Updated comment as suggested. >> >>> >>> >>> http://codereview.appspot.com/5416043/ >> >>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
