[PATCH, CHKP] Restore transparent alias chains

[PATCH, CHKP] Restore transparent alias chains

[Ilya Enkovich]

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);
 	    }
 	}

Re: [PATCH, CHKP] Restore transparent alias chains

[Ilya Enkovich]

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);
>             }
>         }

Re: [PATCH, CHKP] Restore transparent alias chains

[Jeff Law]

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

Re: [PATCH, CHKP] Restore transparent alias chains

[Jan Hubicka]

> 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

Re: [PATCH, CHKP] Restore transparent alias chains

[Jeff Law]

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

Re: [PATCH, CHKP] Restore transparent alias chains

[Ilya Enkovich]

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

Re: [PATCH, CHKP] Restore transparent alias chains

[Jan Hubicka]

> > 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

Re: [PATCH, CHKP] Restore transparent alias chains

[Ilya Enkovich]

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