Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

[Ilya Enkovich <enkovich.gnu@gmail.com>]

2015-04-03 21:49 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
>> Assembler name of instrumented function is a transparent alias of
>> original function's name.  Alias chains are not taken into account by
>> analysis. Thus we see no conflict between instrumented function's name
>> and a variable name but emit them with the same assembler name. To
>> detect name conflict I keep original function node alive.
>
> Hmm, so lets see if I understand your situation.  You always have transparent alias TA
> and function F connected by IPA_REF_CHKP and because TA is represented as thunk
> you also have a fake call from TA to F?
>
> I do not understand how you can miss the link these days.  I extended lto-cgraph to
> always keep thunk and its target in every unit that reffers to thunk. That should
> solve you problem?  How IPA_REF_CHKP can link get lost?

References are not streamed out for nodes which are referenced in a
partition but don't belong to it ('continue' condition in output_refs
loop).

>
> I suppose we still need to
>  1) write_symbol and lto-symtab should know that transparent aliases are not real
>     symbols and ignore them. We have predicate symtab_node::real_symbol_p,
>     I suppose it should return true.
>
>     I think cgraph_merge and varpool_merge in lto-symtab needs to know what to do
>     when merging two symbols with transparent aliases.
>  2) At some point we need to populate transparent alias links as discussed in the
>     other thread.
>  3) For safety, I guess we want symbol_table::change_decl_assembler_name to either
>     sanity check there are no trasparent alias links pointing to old assembler
>     names or update it.

Wouldn't search for possible referring via transparent aliases nodes
be too expensive?

>  4) I think we want verify_node to check that transparent alias links and chkp points
>     as intended and only on those symbols where they need to be
>
> There is also logic in lto-partition to always insert alias target
>> > 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?
>>
>> Wrong links may appear when cgraph nodes are merged.
>
> I see, I think you really want to fix these at merging time as sugested in 1).
> In this case even the node->instrumented_version pointer may be out of date and
> point to a node that lost in merging?

node->instrumented_version is redirected and never points to lost
symbol. But during merge we may have situations when
(node->instrumented_version->instrumented_version != node) because of
multiple not merged (yet) symbols referencing the same merged target.

Thanks,
Ilya

>
> Honza
>>
>> Thanks
>> Ilya