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

Reply via email to