| Message ID | 20150320082033.GG64546@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
Ping. This patch doesn't affect not instrumented code. Thanks, Ilya 2015-03-20 11:20 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > Hi, > > Identifiers read with input_identifier miss IDENTIFIER_TRANSPARENT_ALIAS bit. We always expect it for instrumentation clones, thus restore it input_cgraph_1. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? > > Thanks, > Ilya > -- > 2015-03-20 Ilya Enkovich <ilya.enkovich@intel.com> > > * lto-cgraph.c (input_cgraph_1): Always link instrumented > assembler name with original one. > > > diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c > index c875fed..d782327 100644 > --- a/gcc/lto-cgraph.c > +++ b/gcc/lto-cgraph.c > @@ -1613,9 +1613,8 @@ input_cgraph_1 (struct lto_file_decl_data *file_data, > } > > /* Restore decl names reference. */ > - if (IDENTIFIER_TRANSPARENT_ALIAS (DECL_ASSEMBLER_NAME (cnode->decl)) > - && !TREE_CHAIN (DECL_ASSEMBLER_NAME (cnode->decl))) > - TREE_CHAIN (DECL_ASSEMBLER_NAME (cnode->decl)) > + IDENTIFIER_TRANSPARENT_ALIAS (DECL_ASSEMBLER_NAME (cnode->decl)) = 1; > + TREE_CHAIN (DECL_ASSEMBLER_NAME (cnode->decl)) > = DECL_ASSEMBLER_NAME (cnode->orig_decl); > } > }
On 03/20/2015 02:20 AM, Ilya Enkovich wrote: > Hi, > > Identifiers read with input_identifier miss IDENTIFIER_TRANSPARENT_ALIAS bit. We always expect it for instrumentation clones, thus restore it input_cgraph_1. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? > > Thanks, > Ilya > -- > 2015-03-20 Ilya Enkovich <ilya.enkovich@intel.com> > > * lto-cgraph.c (input_cgraph_1): Always link instrumented > assembler name with original one. This appears to be a code path that only triggers when MPX is enabled and is roughly analogous to the code in chkp_build_instrumented_fndecl links things up. OK for the trunk. jeff
> On 03/20/2015 02:20 AM, Ilya Enkovich wrote: > >Hi, > > > >Identifiers read with input_identifier miss IDENTIFIER_TRANSPARENT_ALIAS bit. We always expect it for instrumentation clones, thus restore it input_cgraph_1. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? > > > >Thanks, > >Ilya > >-- > >2015-03-20 Ilya Enkovich <ilya.enkovich@intel.com> > > > > * lto-cgraph.c (input_cgraph_1): Always link instrumented > > assembler name with original one. > This appears to be a code path that only triggers when MPX is > enabled and is roughly analogous to the code in > chkp_build_instrumented_fndecl links things up. > > OK for the trunk. I think this will lead to wrong code. At this time, we may have multple declarations sharing single assembler name (and thus IDENTIFIER_TRANSPARENT_ALIAS link). Think of case where one unit defines static function and other global function of the same name. We may end up renaming the symbol but keeping bogus transparent alias link that will trigger on other symbol. During WPA the assembler names are never fully unique, but we also probably do not need to set IDENTIFIER_TRANSPARENT_ALIAS. I think IDENTIFIER_TRANSPARENT_ALIAS should be set in rename_statics and separately in ltrans on the place we skip lto_symtab merging. At least it is the place I link it with my patch for supporting transparent aliases in cgraph. I will try to find time to audit chkp code - I missed the addition of transparent aliases in the initial chkp patches. Symbol table code expect 1-1 correspondence between symbol table entries and real symbols. Basically all places we special case aliases or weakrefs needs to be revisited for transparent aliases. Honza
On 04/02/2015 12:48 PM, Jan Hubicka wrote: >> On 03/20/2015 02:20 AM, Ilya Enkovich wrote: >>> Hi, >>> >>> Identifiers read with input_identifier miss IDENTIFIER_TRANSPARENT_ALIAS bit. We always expect it for instrumentation clones, thus restore it input_cgraph_1. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? >>> >>> Thanks, >>> Ilya >>> -- >>> 2015-03-20 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> * lto-cgraph.c (input_cgraph_1): Always link instrumented >>> assembler name with original one. >> This appears to be a code path that only triggers when MPX is >> enabled and is roughly analogous to the code in >> chkp_build_instrumented_fndecl links things up. >> >> OK for the trunk. > > I think this will lead to wrong code. At this time, we may have multple > declarations sharing single assembler name (and thus > IDENTIFIER_TRANSPARENT_ALIAS link). Think of case where one unit defines static > function and other global function of the same name. We may end up renaming the > symbol but keeping bogus transparent alias link that will trigger on other > symbol. Then I think the code in ipa-chkp chkp_build_instrumented_fndecl (and more generally how we're using transparent aliases) may need some rethinking. jeff
2015-04-02 21:48 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>: >> On 03/20/2015 02:20 AM, Ilya Enkovich wrote: >> >Hi, >> > >> >Identifiers read with input_identifier miss IDENTIFIER_TRANSPARENT_ALIAS bit. We always expect it for instrumentation clones, thus restore it input_cgraph_1. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? >> > >> >Thanks, >> >Ilya >> >-- >> >2015-03-20 Ilya Enkovich <ilya.enkovich@intel.com> >> > >> > * lto-cgraph.c (input_cgraph_1): Always link instrumented >> > assembler name with original one. >> This appears to be a code path that only triggers when MPX is >> enabled and is roughly analogous to the code in >> chkp_build_instrumented_fndecl links things up. >> >> OK for the trunk. > > I think this will lead to wrong code. At this time, we may have multple > declarations sharing single assembler name (and thus > IDENTIFIER_TRANSPARENT_ALIAS link). Think of case where one unit defines static > function and other global function of the same name. We may end up renaming the > symbol but keeping bogus transparent alias link that will trigger on other > symbol. That is the reason for fixing chains in privatize_symbol_name. > > During WPA the assembler names are never fully unique, but we also probably do > not need to set IDENTIFIER_TRANSPARENT_ALIAS. > > I think IDENTIFIER_TRANSPARENT_ALIAS should be set in rename_statics and > separately in ltrans on the place we skip lto_symtab merging. At least it is > the place I link it with my patch for supporting transparent aliases in cgraph. > > I will try to find time to audit chkp code - I missed the addition of > transparent aliases in the initial chkp patches. Symbol table code expect 1-1 > correspondence between symbol table entries and real symbols. Basically all > places we special case aliases or weakrefs needs to be revisited for > transparent aliases. I don't see how 1-1 matching may be achieved now for instrumented functions. We have two cgraph_nodes which actually represent the same function. Thus single real symbol for two nodes. Thanks, Ilya > > Honza
> > I think this will lead to wrong code. At this time, we may have multple > > declarations sharing single assembler name (and thus > > IDENTIFIER_TRANSPARENT_ALIAS link). Think of case where one unit defines static > > function and other global function of the same name. We may end up renaming the > > symbol but keeping bogus transparent alias link that will trigger on other > > symbol. > > That is the reason for fixing chains in privatize_symbol_name. OK, so with your proposed patch you produce the links during lto-stream-in but because the links may be wrong due to multiple different symbol sharing same assembler name you rely on not using it until you fixup in privatize_symbol_name. What happen when you have two symbols with different links sharing assembler name originally, but you rename one and do not rename other during privatization? It still looks much safer to simply install the links only after names become unique and probably don't do that during WPA compilation at all, since we never output any assembly. > > > > > During WPA the assembler names are never fully unique, but we also probably do > > not need to set IDENTIFIER_TRANSPARENT_ALIAS. > > > > I think IDENTIFIER_TRANSPARENT_ALIAS should be set in rename_statics and > > separately in ltrans on the place we skip lto_symtab merging. At least it is > > the place I link it with my patch for supporting transparent aliases in cgraph. > > > > I will try to find time to audit chkp code - I missed the addition of > > transparent aliases in the initial chkp patches. Symbol table code expect 1-1 > > correspondence between symbol table entries and real symbols. Basically all > > places we special case aliases or weakrefs needs to be revisited for > > transparent aliases. > > I don't see how 1-1 matching may be achieved now for instrumented > functions. We have two cgraph_nodes which actually represent the same > function. Thus single real symbol for two nodes. Yeah, this is quite important change in the design assumptions for symtab that is why we need so many places to fix... Honza > > Thanks, > Ilya > > > > > Honza
2015-04-03 20:09 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>: >> >> That is the reason for fixing chains in privatize_symbol_name. > > OK, so with your proposed patch you produce the links during lto-stream-in > but because the links may be wrong due to multiple different symbol sharing same > assembler name you rely on not using it until you fixup in privatize_symbol_name. > What happen when you have two symbols with different links sharing assembler name > originally, but you rename one and do not rename other during privatization? > > It still looks much safer to simply install the links only after names become unique > and probably don't do that during WPA compilation at all, since we never output > any assembly. Original links are not wrong. Links just may be shared by many decl pairs. If we always privatize whole pair and never privatize only one its member, then we don't break existing links but create new ones for newly created assembler names. Agree it may be simplified by producing links later and it is useless for WPA because links are not streamed out. Probably set links in lto_main before symbol_table::compile call? Thanks, Ilya >> >> I don't see how 1-1 matching may be achieved now for instrumented >> functions. We have two cgraph_nodes which actually represent the same >> function. Thus single real symbol for two nodes. > > Yeah, this is quite important change in the design assumptions for symtab that > is why we need so many places to fix... > > Honza >> >> Thanks, >> Ilya >> >> > >> > Honza
diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c index c875fed..d782327 100644 --- a/gcc/lto-cgraph.c +++ b/gcc/lto-cgraph.c @@ -1613,9 +1613,8 @@ input_cgraph_1 (struct lto_file_decl_data *file_data, } /* Restore decl names reference. */ - if (IDENTIFIER_TRANSPARENT_ALIAS (DECL_ASSEMBLER_NAME (cnode->decl)) - && !TREE_CHAIN (DECL_ASSEMBLER_NAME (cnode->decl))) - TREE_CHAIN (DECL_ASSEMBLER_NAME (cnode->decl)) + IDENTIFIER_TRANSPARENT_ALIAS (DECL_ASSEMBLER_NAME (cnode->decl)) = 1; + TREE_CHAIN (DECL_ASSEMBLER_NAME (cnode->decl)) = DECL_ASSEMBLER_NAME (cnode->orig_decl); } }