From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31432 invoked by alias); 2 Apr 2015 17:01:21 -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 31421 invoked by uid 89); 2 Apr 2015 17:01:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: atrey.karlin.mff.cuni.cz Received: from atrey.karlin.mff.cuni.cz (HELO atrey.karlin.mff.cuni.cz) (195.113.26.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Apr 2015 17:01:18 +0000 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 4018) id 9445E81EFA; Thu, 2 Apr 2015 19:01:15 +0200 (CEST) Date: Thu, 02 Apr 2015 17:01:00 -0000 From: Jan Hubicka To: Ilya Enkovich Cc: gcc-patches Subject: Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions Message-ID: <20150402170115.GB21276@atrey.karlin.mff.cuni.cz> References: <20150312102706.GL27860@msticlxl57.ims.intel.com> <20150319083455.GD64546@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-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg00071.txt.bz2 > Ping It would help if you add hubicka@ucw.cz to CC for IPA related patches. > > 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 Why do you need to detect this one? > >> - 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? Next stage1 I definitly hope we will be able to reduce number of special cases we need for chck nodes. I will try to read the code and your design document and give it some tought. > > 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) OK, so you have the chkp references that links to instrumented_version. You do not stream them for boundary symbols but for some reason they are needed, but when you can end up with wrong CHKP link? > > 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); > > + } > > + } > > + This is OK > > +/* 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); This is because these are TRANSPARENT_ALIASes and thus visibility needs to match? I plan to add generic support for transparent aliases soon (to fix the FORTIFY_SOURCE LTO bug), so perhaps this will become easier? THis is also OK. We may want to have verifier code that checks that the visibility match. Honza