From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 108895 invoked by alias); 29 May 2015 05:13:45 -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 108883 invoked by uid 89); 29 May 2015 05:13:44 -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_50,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: nikam.ms.mff.cuni.cz Received: from nikam.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 29 May 2015 05:13:42 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id EEF035419FC; Fri, 29 May 2015 07:13:37 +0200 (CEST) Date: Fri, 29 May 2015 06:33:00 -0000 From: Jan Hubicka To: Ilya Enkovich Cc: Jan Hubicka , gcc-patches Subject: Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions Message-ID: <20150529051337.GA21936@kam.mff.cuni.cz> References: <20150402170115.GB21276@atrey.karlin.mff.cuni.cz> <20150403184915.GS21276@atrey.karlin.mff.cuni.cz> <20150410011532.GA17443@kam.mff.cuni.cz> <20150414091414.GA50171@msticlxl57.ims.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2015-05/txt/msg02719.txt.bz2 > Ping I am really sorry for ignoring this so long - I would like to reorg the code and replace instrumentaiton thunks by the notion of transparent aliases, but did not have time to do that yet. Have quite busy time now. > >>> > >>> 2015-04-14 Ilya Enkovich > >>> > >>> * ipa.c (symbol_table::remove_unreachable_nodes): Don't > >>> remove instumentation thunks calling reachable functions. > >>> * lto-cgraph.c (output_refs): Always output IPA_REF_CHKP. > >>> * lto/lto-partition.c (privatize_symbol_name_1): New. > >>> (privatize_symbol_name): Privatize both decl and orig_decl > >>> names for instrumented functions. > >>> > >>> gcc/testsuite/ > >>> > >>> 2015-04-14 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.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) reachable.contains (cnode) is !in_boundary_p. > >>> + && 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); Why do you need the other tests. Can we have instrumentation node that is not definition? I suppose you can remove if (cnode->instrumented_version) because you assert it anyway. > >>> - 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))); > >>> + I think we ought to have verify_symtab_node checking for this. All the handling of the name links seems somewhat fragile (I am mostly concerned about getting it right for LTO before remaning takes place) OK with the changes above for mainline and for branch if it does not cause problems for a week. Honza > >>> return true; > >>> } > >>> > >>> 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; > >>> +}