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

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2015-04-02 20:01 GMT+03:00 Jan Hubicka <hubicka@ucw.cz>:
>> Ping
> It would help if you add hubicka@ucw.cz to CC for IPA related patches.
>>
>> 2015-03-19 11:34 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
>> > On 12 Mar 13:27, Ilya Enkovich wrote:
>> >> Hi,
>> >>
>> >> Currently cgraph merge has several issues with instrumented code:
>> >>  - original function node may be removed => no assembler name conflict is detected between function and variable
>
> Why do you need to detect this one?

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.

>> >>  - only orig_decl name is privatized for instrumented function => node still shares assembler name which causes infinite privatization loop
>> >>  - information about changed name is stored in file_data of instrumented node => original section name may be not found for original function
>> >>  - chkp reference is not fixed when nodes are merged
>> >>
>> >> This patch should fix theese problems by keeping instrumentation thunks reachable, privatizing both nodes and fixing chkp references.  Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?
>
> Next stage1 I definitly hope we will be able to reduce number of special cases
> we need for chck nodes.  I will try to read the code and your design document
> and give it some tought.

Original design didn't take into account several LTO aspect and
additional patches appeared to fix various LTO issues. It would be
nice to improve it with your help. I'll need to update a design
document to reflect recent problems and used fixes.

>> > 2015-03-19  Ilya Enkovich  <ilya.enkovich@intel.com>
>> >
>> >         * ipa-chkp.h (chkp_maybe_fix_chkp_ref): New.
>> >         * ipa-chkp.c (chkp_maybe_fix_chkp_ref): New.
>> >         * ipa.c (symbol_table::remove_unreachable_nodes): Don't
>> >         remove instumentation thunks calling reachable functions.
>> >         * lto-cgraph.c: Include ipa-chkp.h.
>> >         (input_symtab): Fix chkp references for boundary nodes.
>> >         * lto/lto-partition.c (privatize_symbol_name_1): New.
>> >         (privatize_symbol_name): Privatize both decl and orig_decl
>> >         names for instrumented functions.
>> >         * lto/lto-symtab.c: Include ipa-chkp.h.
>> >         (lto_cgraph_replace_node): Fix chkp references for merged
>> >         function nodes.
>> >
>> > gcc/testsuite/
>> >
>> > 2015-03-19  Ilya Enkovich  <ilya.enkovich@intel.com>
>> >
>> >         * gcc.dg/lto/chkp-privatize-1_0.c: New.
>> >         * gcc.dg/lto/chkp-privatize-1_1.c: New.
>> >         * gcc.dg/lto/chkp-privatize-2_0.c: New.
>> >         * gcc.dg/lto/chkp-privatize-2_1.c: New.
>> >
>> >
>> > diff --git a/gcc/ipa-chkp.c b/gcc/ipa-chkp.c
>> > index 0b857ff..7a7fc3c 100644
>> > --- a/gcc/ipa-chkp.c
>> > +++ b/gcc/ipa-chkp.c
>> > @@ -414,6 +414,40 @@ chkp_instrumentable_p (tree fndecl)
>> >           && (!fn || !copy_forbidden (fn, fndecl)));
>> >  }
>> >
>> > +/* Check NODE has a correct IPA_REF_CHKP reference.
>> > +   Create a new reference if required.  */
>> > +
>> > +void
>> > +chkp_maybe_fix_chkp_ref (cgraph_node *node)
>
> 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.

Thanks
Ilya