From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 43463 invoked by alias); 19 Mar 2015 08:35:07 -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 43396 invoked by uid 89); 19 Mar 2015 08:35:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=4.4 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,KAM_FROM_URIBL_PCCC,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-ie0-f182.google.com Received: from mail-ie0-f182.google.com (HELO mail-ie0-f182.google.com) (209.85.223.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 19 Mar 2015 08:35:04 +0000 Received: by iecvj10 with SMTP id vj10so60489665iec.0 for ; Thu, 19 Mar 2015 01:35:02 -0700 (PDT) X-Received: by 10.50.124.164 with SMTP id mj4mr14011047igb.38.1426754102776; Thu, 19 Mar 2015 01:35:02 -0700 (PDT) Received: from msticlxl57.ims.intel.com ([192.55.54.40]) by mx.google.com with ESMTPSA id g195sm490033iog.24.2015.03.19.01.35.01 for (version=TLSv1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 19 Mar 2015 01:35:02 -0700 (PDT) Date: Thu, 19 Mar 2015 08:35:00 -0000 From: Ilya Enkovich To: gcc-patches@gcc.gnu.org Subject: Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions Message-ID: <20150319083455.GD64546@msticlxl57.ims.intel.com> References: <20150312102706.GL27860@msticlxl57.ims.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150312102706.GL27860@msticlxl57.ims.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2015-03/txt/msg00992.txt.bz2 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; +}