| Message ID | 20140528104154.GA18451@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 05/28/14 10:06, Ilya Enkovich wrote: > Hi, > > This patch keeps instrumented and original versions together and preserve tranparent alias chain during symbol name privatization. > > Bootstrapped and tested on linux-x86_64. > > Thanks, > Ilya > -- > gcc/ > > 2013-05-28 Ilya Enkovich <ilya.enkovich@intel.com> > > * lto/lto-partition.c (add_symbol_to_partition_1): Keep original > and instrumented versions together. This part is OK. Note lto/ has its own ChangeLog, so put the ChangeLog entry there and don't use the "lto/" prefix in the ChangeLog entry. > (privatize_symbol_name): Restore transparent alias chain if required. What exactly are you doing here? The comment in the code doesn't really make it clear what you are doing or why. > + /* We could change name which is a target of transparent alias > + chain of instrumented function name. Fix alias chain if so .*/ So are you saying that we want to change the name? Or that it could have been changed and we have to adjust something because it was changed? I'm certainly not as familiar with the LTO stuff as I should be -- what is the purpose behing chaining the DECL_ASSEMBLER_NAME nodes? jeff
On Fri, May 30, 2014 at 7:10 PM, Jeff Law <law@redhat.com> wrote: > On 05/28/14 10:06, Ilya Enkovich wrote: >> >> Hi, >> >> This patch keeps instrumented and original versions together and preserve >> tranparent alias chain during symbol name privatization. >> >> Bootstrapped and tested on linux-x86_64. >> >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2013-05-28 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * lto/lto-partition.c (add_symbol_to_partition_1): Keep original >> and instrumented versions together. > > This part is OK. Note lto/ has its own ChangeLog, so put the ChangeLog > entry there and don't use the "lto/" prefix in the ChangeLog entry. > > >> (privatize_symbol_name): Restore transparent alias chain if >> required. > > What exactly are you doing here? The comment in the code doesn't really > make it clear what you are doing or why. > > >> + /* We could change name which is a target of transparent alias >> + chain of instrumented function name. Fix alias chain if so .*/ > > So are you saying that we want to change the name? Or that it could have > been changed and we have to adjust something because it was changed? > > I'm certainly not as familiar with the LTO stuff as I should be -- what is > the purpose behing chaining the DECL_ASSEMBLER_NAME nodes? Something gross: /* Nonzero in an IDENTIFIER_NODE if the name is a local alias, whose uses are to be substituted for uses of the TREE_CHAINed identifier. */ #define IDENTIFIER_TRANSPARENT_ALIAS(NODE) \ (IDENTIFIER_NODE_CHECK (NODE)->base.deprecated_flag) this should be all moved to the symbol table level. (and IDENTIFIER_NODE shouldn't have to have tree_common.chain and thus become smaller). Richard. > jeff >
On 30 May 11:10, Jeff Law wrote: > On 05/28/14 10:06, Ilya Enkovich wrote: > >Hi, > > > >This patch keeps instrumented and original versions together and preserve tranparent alias chain during symbol name privatization. > > > >Bootstrapped and tested on linux-x86_64. > > > >Thanks, > >Ilya > >-- > >gcc/ > > > >2013-05-28 Ilya Enkovich <ilya.enkovich@intel.com> > > > > * lto/lto-partition.c (add_symbol_to_partition_1): Keep original > > and instrumented versions together. > This part is OK. Note lto/ has its own ChangeLog, so put the > ChangeLog entry there and don't use the "lto/" prefix in the > ChangeLog entry. > > > (privatize_symbol_name): Restore transparent alias chain if required. > What exactly are you doing here? The comment in the code doesn't > really make it clear what you are doing or why. > > >+ /* We could change name which is a target of transparent alias > >+ chain of instrumented function name. Fix alias chain if so .*/ > So are you saying that we want to change the name? Or that it could > have been changed and we have to adjust something because it was > changed? > > I'm certainly not as familiar with the LTO stuff as I should be -- > what is the purpose behing chaining the DECL_ASSEMBLER_NAME nodes? I'm just adjusting renaming to support alias chains. LTO renames functions to avoid two static functions with the same assembler name. Instrumented functions have names chained with original names using IDENTIFIER_TRANSPARENT_ALIAS. If name of the original function is privatized, then IDENTIFIER_TRANSPARENT_ALIAS still points to the old name which is wrong. My patch fixes it. > > jeff > Here is fixed ChangeLog. Thanks, Ilya -- gcc/lto 2014-06-02 Ilya Enkovich <ilya.enkovich@intel.com> * lto-partition.c (add_symbol_to_partition_1): Keep original and instrumented versions together. (privatize_symbol_name): Restore transparent alias chain if required.
On 06/02/14 05:41, Richard Biener wrote: > > this should be all moved to the symbol table level. (and IDENTIFIER_NODE > shouldn't have to have tree_common.chain and thus become smaller). Which ought to be independent of the pointer checking work. There's been some proposals for making first class symbol tables in GCC, but nothing that's made any progress at this point. Maybe I'll sick David on it at some point. jeff
On 06/02/14 05:50, Ilya Enkovich wrote: > On 30 May 11:10, Jeff Law wrote: >> On 05/28/14 10:06, Ilya Enkovich wrote: >>> Hi, >>> >>> This patch keeps instrumented and original versions together and preserve tranparent alias chain during symbol name privatization. >>> >>> Bootstrapped and tested on linux-x86_64. >>> >>> Thanks, >>> Ilya >>> -- >>> gcc/ >>> >>> 2013-05-28 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> * lto/lto-partition.c (add_symbol_to_partition_1): Keep original >>> and instrumented versions together. >> This part is OK. Note lto/ has its own ChangeLog, so put the >> ChangeLog entry there and don't use the "lto/" prefix in the >> ChangeLog entry. >> >>> (privatize_symbol_name): Restore transparent alias chain if required. >> What exactly are you doing here? The comment in the code doesn't >> really make it clear what you are doing or why. >> >>> + /* We could change name which is a target of transparent alias >>> + chain of instrumented function name. Fix alias chain if so .*/ >> So are you saying that we want to change the name? Or that it could >> have been changed and we have to adjust something because it was >> changed? >> >> I'm certainly not as familiar with the LTO stuff as I should be -- >> what is the purpose behing chaining the DECL_ASSEMBLER_NAME nodes? > > I'm just adjusting renaming to support alias chains. LTO renames functions to avoid two static functions with the same assembler name. Instrumented functions have names chained with original names using IDENTIFIER_TRANSPARENT_ALIAS. If name of the original function is privatized, then IDENTIFIER_TRANSPARENT_ALIAS still points to the old name which is wrong. My patch fixes it. > >> >> jeff >> > > Here is fixed ChangeLog. > > Thanks, > Ilya > -- > gcc/lto > > 2014-06-02 Ilya Enkovich <ilya.enkovich@intel.com> > > * lto-partition.c (add_symbol_to_partition_1): Keep original > and instrumented versions together. > (privatize_symbol_name): Restore transparent alias chain if required. OK. This is fine once all the other pointer checking bits are approved. jeff
On Tue, Jun 3, 2014 at 11:24 PM, Jeff Law <law@redhat.com> wrote: > On 06/02/14 05:41, Richard Biener wrote: >> >> >> this should be all moved to the symbol table level. (and IDENTIFIER_NODE >> shouldn't have to have tree_common.chain and thus become smaller). > > Which ought to be independent of the pointer checking work. There's been > some proposals for making first class symbol tables in GCC, but nothing > that's made any progress at this point. Maybe I'll sick David on it at some > point. You probably missed that we now have such symbol table (ok, it still lacks labels and CONST_DECLs). Richard. > jeff >
diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c index 1ee5fbb..2967d73 100644 --- a/gcc/lto/lto-partition.c +++ b/gcc/lto/lto-partition.c @@ -163,6 +163,11 @@ add_symbol_to_partition_1 (ltrans_partition part, symtab_node *node) for (e = cnode->callers; e; e = e->next_caller) if (e->caller->thunk.thunk_p) add_symbol_to_partition_1 (part, e->caller); + + /* Instrumented version is actually the same function. + Therefore put it into the same partition. */ + if (cnode->instrumented_version) + add_symbol_to_partition_1 (part, cnode->instrumented_version); } add_references_to_partition (part, node); @@ -745,6 +750,7 @@ privatize_symbol_name (symtab_node *node) { tree decl = node->decl; const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); + cgraph_node *cnode; /* Our renaming machinery do not handle more than one change of assembler name. We should not need more than one anyway. */ @@ -774,6 +780,18 @@ privatize_symbol_name (symtab_node *node) lto_record_renamed_decl (node->lto_file_data, name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); + /* We could change name which is a target of transparent alias + chain of instrumented function name. Fix alias chain if so .*/ + if ((cnode = dyn_cast <cgraph_node> (node)) + && !cnode->instrumentation_clone + && cnode->instrumented_version + && cnode->instrumented_version->orig_decl == decl) + { + tree iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); + + gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname)); + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl); + } if (cgraph_dump_file) fprintf (cgraph_dump_file, "Privatizing symbol name: %s -> %s\n",