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

[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?
> >>  - 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.
> > 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?
> > diff --git a/gcc/ipa.c b/gcc/ipa.c
> > index b3752de..3054afe 100644
> > --- a/gcc/ipa.c
> > +++ b/gcc/ipa.c
> > @@ -492,7 +492,22 @@ symbol_table::remove_unreachable_nodes (FILE *file)
> >             }
> >           else if (cnode->thunk.thunk_p)
> >             enqueue_node (cnode->callees->callee, &first, &reachable);
> > -
> > +
> > +         /* For instrumentation clones we always need original
> > +            function node for proper LTO privatization.  */
> > +         if (cnode->instrumentation_clone
> > +             && reachable.contains (cnode)
> > +             && cnode->definition)
> > +           {
> > +             gcc_assert (cnode->instrumented_version || in_lto_p);
> > +             if (cnode->instrumented_version)
> > +               {
> > +                 enqueue_node (cnode->instrumented_version, &first,
> > +                               &reachable);
> > +                 reachable.add (cnode->instrumented_version);
> > +               }
> > +           }
> > +
This is OK
> > +/* Mangle NODE symbol name into a local name.
> > +   This is necessary to do
> > +   1) if two or more static vars of same assembler name
> > +      are merged into single ltrans unit.
> > +   2) if previously static var was promoted hidden to avoid possible conflict
> > +      with symbols defined out of the LTO world.  */
> > +
> > +static bool
> > +privatize_symbol_name (symtab_node *node)
> > +{
> > +  if (!privatize_symbol_name_1 (node, node->decl))
> > +    return false;
> > +
> >    /* We could change name which is a target of transparent alias
> >       chain of instrumented function name.  Fix alias chain if so  .*/
> > -  if (cnode)
> > +  if (cgraph_node *cnode = dyn_cast <cgraph_node *> (node))
> >      {
> >        tree iname = NULL_TREE;
> >        if (cnode->instrumentation_clone)
> > -       iname = DECL_ASSEMBLER_NAME (cnode->decl);
> > +       {
> > +         /* If we want to privatize instrumentation clone
> > +            then we also need to privatize original function.  */
> > +         if (cnode->instrumented_version)
> > +           privatize_symbol_name (cnode->instrumented_version);
> > +         else
> > +           privatize_symbol_name_1 (cnode, cnode->orig_decl);

This is because these are TRANSPARENT_ALIASes and thus visibility needs to match?
I plan to add generic support for transparent aliases soon (to fix the FORTIFY_SOURCE
LTO bug), so perhaps this will become easier?
THis is also OK.  We may want to have verifier code that checks that the visibility
match.

Honza