From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61463 invoked by alias); 2 Apr 2015 14:52:59 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 59715 invoked by uid 89); 2 Apr 2015 14:52:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ob0-f178.google.com Received: from mail-ob0-f178.google.com (HELO mail-ob0-f178.google.com) (209.85.214.178) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 02 Apr 2015 14:52:56 +0000 Received: by obvd1 with SMTP id d1so132955892obv.0 for ; Thu, 02 Apr 2015 07:52:55 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.182.125.130 with SMTP id mq2mr18643765obb.52.1427986372948; Thu, 02 Apr 2015 07:52:52 -0700 (PDT) Received: by 10.202.229.72 with HTTP; Thu, 2 Apr 2015 07:52:52 -0700 (PDT) In-Reply-To: <20150319083455.GD64546@msticlxl57.ims.intel.com> References: <20150312102706.GL27860@msticlxl57.ims.intel.com> <20150319083455.GD64546@msticlxl57.ims.intel.com> Date: Thu, 02 Apr 2015 14:52:00 -0000 Message-ID: Subject: Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions From: Ilya Enkovich To: gcc-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg00059.txt.bz2 Ping 2015-03-19 11:34 GMT+03:00 Ilya Enkovich : > On 12 Mar 13:27, Ilya Enkovich wrote: >> Hi, >> >> Currently cgraph merge has several issues with instrumented code: >> - original function node may be removed => no assembler name conflict is detected between function and variable >> - only orig_decl name is privatized for instrumented function => node still shares assembler name which causes infinite privatization loop >> - information about changed name is stored in file_data of instrumented node => original section name may be not found for original function >> - chkp reference is not fixed when nodes are merged >> >> This patch should fix theese problems by keeping instrumentation thunks reachable, privatizing both nodes and fixing chkp references. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? >> >> >> Thanks, >> Ilya > > Here is an updated version where chkp_maybe_fix_chkp_ref also removes duplicating references which may appear during cgraph nodes merge. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? > > > Thanks, > Ilya > -- > gcc/ > > 2015-03-19 Ilya Enkovich > > * ipa-chkp.h (chkp_maybe_fix_chkp_ref): New. > * ipa-chkp.c (chkp_maybe_fix_chkp_ref): New. > * ipa.c (symbol_table::remove_unreachable_nodes): Don't > remove instumentation thunks calling reachable functions. > * lto-cgraph.c: Include ipa-chkp.h. > (input_symtab): Fix chkp references for boundary nodes. > * lto/lto-partition.c (privatize_symbol_name_1): New. > (privatize_symbol_name): Privatize both decl and orig_decl > names for instrumented functions. > * lto/lto-symtab.c: Include ipa-chkp.h. > (lto_cgraph_replace_node): Fix chkp references for merged > function nodes. > > gcc/testsuite/ > > 2015-03-19 Ilya Enkovich > > * gcc.dg/lto/chkp-privatize-1_0.c: New. > * gcc.dg/lto/chkp-privatize-1_1.c: New. > * gcc.dg/lto/chkp-privatize-2_0.c: New. > * gcc.dg/lto/chkp-privatize-2_1.c: New. > > > diff --git a/gcc/ipa-chkp.c b/gcc/ipa-chkp.c > index 0b857ff..7a7fc3c 100644 > --- a/gcc/ipa-chkp.c > +++ b/gcc/ipa-chkp.c > @@ -414,6 +414,40 @@ chkp_instrumentable_p (tree fndecl) > && (!fn || !copy_forbidden (fn, fndecl))); > } > > +/* Check NODE has a correct IPA_REF_CHKP reference. > + Create a new reference if required. */ > + > +void > +chkp_maybe_fix_chkp_ref (cgraph_node *node) > +{ > + /* Firstly check node needs IPA_REF_CHKP. */ > + if (node->instrumentation_clone > + || !node->instrumented_version) > + return; > + > + /* Check we already have a proper IPA_REF_CHKP. > + Remove incorrect refs. */ > + int i; > + ipa_ref *ref = NULL; > + bool found = false; > + for (i = 0; node->iterate_reference (i, ref); i++) > + if (ref->use == IPA_REF_CHKP) > + { > + /* Found proper reference. */ > + if (ref->referred == node->instrumented_version > + && !found) > + found = true; > + else > + { > + ref->remove_reference (); > + i--; > + } > + } > + > + if (!found) > + node->create_reference (node->instrumented_version, IPA_REF_CHKP, NULL); > +} > + > /* Return clone created for instrumentation of NODE or NULL. */ > > cgraph_node * > diff --git a/gcc/ipa-chkp.h b/gcc/ipa-chkp.h > index 6708fe9..5fa7d88 100644 > --- a/gcc/ipa-chkp.h > +++ b/gcc/ipa-chkp.h > @@ -24,5 +24,6 @@ extern tree chkp_copy_function_type_adding_bounds (tree orig_type); > extern tree chkp_maybe_clone_builtin_fndecl (tree fndecl); > extern cgraph_node *chkp_maybe_create_clone (tree fndecl); > extern bool chkp_instrumentable_p (tree fndecl); > +extern void chkp_maybe_fix_chkp_ref (cgraph_node *node); > > #endif /* GCC_IPA_CHKP_H */ > diff --git a/gcc/ipa.c b/gcc/ipa.c > index b3752de..3054afe 100644 > --- a/gcc/ipa.c > +++ b/gcc/ipa.c > @@ -492,7 +492,22 @@ symbol_table::remove_unreachable_nodes (FILE *file) > } > else if (cnode->thunk.thunk_p) > enqueue_node (cnode->callees->callee, &first, &reachable); > - > + > + /* For instrumentation clones we always need original > + function node for proper LTO privatization. */ > + if (cnode->instrumentation_clone > + && reachable.contains (cnode) > + && cnode->definition) > + { > + gcc_assert (cnode->instrumented_version || in_lto_p); > + if (cnode->instrumented_version) > + { > + enqueue_node (cnode->instrumented_version, &first, > + &reachable); > + reachable.add (cnode->instrumented_version); > + } > + } > + > /* If any reachable function has simd clones, mark them as > reachable as well. */ > if (cnode->simd_clones) > diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c > index c875fed..b9196eb 100644 > --- a/gcc/lto-cgraph.c > +++ b/gcc/lto-cgraph.c > @@ -80,6 +80,7 @@ along with GCC; see the file COPYING3. If not see > #include "pass_manager.h" > #include "ipa-utils.h" > #include "omp-low.h" > +#include "ipa-chkp.h" > > /* True when asm nodes has been output. */ > bool asm_nodes_output = false; > @@ -1888,6 +1889,10 @@ input_symtab (void) > context of the nested function. */ > if (node->lto_file_data) > node->aux = NULL; > + > + /* May need to fix chkp reference because we don't stream > + them for boundary symbols. */ > + chkp_maybe_fix_chkp_ref (node); > } > } > > diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c > index 235b735..7d117e9 100644 > --- a/gcc/lto/lto-partition.c > +++ b/gcc/lto/lto-partition.c > @@ -877,27 +877,13 @@ validize_symbol_for_target (symtab_node *node) > } > } > > -/* Mangle NODE symbol name into a local name. > - This is necessary to do > - 1) if two or more static vars of same assembler name > - are merged into single ltrans unit. > - 2) if previously static var was promoted hidden to avoid possible conflict > - with symbols defined out of the LTO world. */ > +/* Helper for privatize_symbol_name. Mangle NODE symbol name > + represented by DECL. */ > > static bool > -privatize_symbol_name (symtab_node *node) > +privatize_symbol_name_1 (symtab_node *node, tree decl) > { > - tree decl = node->decl; > - const char *name; > - cgraph_node *cnode = dyn_cast (node); > - > - /* If we want to privatize instrumentation clone > - then we need to change original function name > - which is used via transparent alias chain. */ > - if (cnode && cnode->instrumentation_clone) > - decl = cnode->orig_decl; > - > - name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); > + const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); > > if (must_not_rename (node, name)) > return false; > @@ -906,31 +892,57 @@ privatize_symbol_name (symtab_node *node) > symtab->change_decl_assembler_name (decl, > clone_function_name_1 (name, > "lto_priv")); > + > if (node->lto_file_data) > lto_record_renamed_decl (node->lto_file_data, name, > IDENTIFIER_POINTER > (DECL_ASSEMBLER_NAME (decl))); > + > + if (symtab->dump_file) > + fprintf (symtab->dump_file, > + "Privatizing symbol name: %s -> %s\n", > + name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); > + > + return true; > +} > + > +/* Mangle NODE symbol name into a local name. > + This is necessary to do > + 1) if two or more static vars of same assembler name > + are merged into single ltrans unit. > + 2) if previously static var was promoted hidden to avoid possible conflict > + with symbols defined out of the LTO world. */ > + > +static bool > +privatize_symbol_name (symtab_node *node) > +{ > + if (!privatize_symbol_name_1 (node, node->decl)) > + return false; > + > /* We could change name which is a target of transparent alias > chain of instrumented function name. Fix alias chain if so .*/ > - if (cnode) > + if (cgraph_node *cnode = dyn_cast (node)) > { > tree iname = NULL_TREE; > if (cnode->instrumentation_clone) > - iname = DECL_ASSEMBLER_NAME (cnode->decl); > + { > + /* If we want to privatize instrumentation clone > + then we also need to privatize original function. */ > + if (cnode->instrumented_version) > + privatize_symbol_name (cnode->instrumented_version); > + else > + privatize_symbol_name_1 (cnode, cnode->orig_decl); > + iname = DECL_ASSEMBLER_NAME (cnode->decl); > + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->orig_decl); > + } > else if (cnode->instrumented_version > - && cnode->instrumented_version->orig_decl == decl) > - iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); > - > - if (iname) > + && cnode->instrumented_version->orig_decl == cnode->decl) > { > - gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname)); > - TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl); > + iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); > + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->decl); > } > } > - if (symtab->dump_file) > - fprintf (symtab->dump_file, > - "Privatizing symbol name: %s -> %s\n", > - name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); > + > return true; > } > > diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c > index c00fd87..e0b9762 100644 > --- a/gcc/lto/lto-symtab.c > +++ b/gcc/lto/lto-symtab.c > @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. If not see > #include "ipa-prop.h" > #include "ipa-inline.h" > #include "builtins.h" > +#include "ipa-chkp.h" > > /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging > all edges and removing the old node. */ > @@ -116,7 +117,11 @@ lto_cgraph_replace_node (struct cgraph_node *node, > == prevailing_node->instrumentation_clone); > node->instrumented_version->instrumented_version = prevailing_node; > if (!prevailing_node->instrumented_version) > - prevailing_node->instrumented_version = node->instrumented_version; > + { > + prevailing_node->instrumented_version = node->instrumented_version; > + chkp_maybe_fix_chkp_ref (prevailing_node); > + } > + chkp_maybe_fix_chkp_ref (node->instrumented_version); > /* Need to reset node->instrumented_version to NULL, > otherwise node removal code would reset > node->instrumented_version->instrumented_version. */ > diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c > new file mode 100644 > index 0000000..2054aa15 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c > @@ -0,0 +1,17 @@ > +/* { dg-lto-do link } */ > +/* { dg-require-effective-target mpx } */ > +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ > + > +extern int __attribute__((noinline)) f1 (int i); > + > +static int __attribute__((noinline)) > +f2 (int i) > +{ > + return i + 6; > +} > + > +int > +main (int argc, char **argv) > +{ > + return f1 (argc) + f2 (argc); > +} > diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c > new file mode 100644 > index 0000000..4fa8656 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c > @@ -0,0 +1,11 @@ > +static int __attribute__((noinline)) > +f2 (int i) > +{ > + return 2 * i; > +} > + > +int __attribute__((noinline)) > +f1 (int i) > +{ > + return f2 (i) + 10; > +} > diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c > new file mode 100644 > index 0000000..be7f601 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c > @@ -0,0 +1,18 @@ > +/* { dg-lto-do link } */ > +/* { dg-require-effective-target mpx } */ > +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ > + > +static int > +__attribute__ ((noinline)) > +func1 (int i) > +{ > + return i + 2; > +} > + > +extern int func2 (int i); > + > +int > +main (int argc, char **argv) > +{ > + return func1 (argc) + func2 (argc) + 1; > +} > diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c > new file mode 100644 > index 0000000..db39e7f > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c > @@ -0,0 +1,8 @@ > +int func1 = 10; > + > +int > +func2 (int i) > +{ > + func1++; > + return i + func1; > +}