[CHKP] Fix LTO cgraph merge for instrumented functions

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
>  - 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?
> 
> 
> Thanks,
> Ilya

Here is an updated version where chkp_maybe_fix_chkp_ref also removes duplicating references which may appear during cgraph nodes merge.  Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?


Thanks,
Ilya
--
gcc/

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.

Ping

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
>>  - 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?
>>
>>
>> Thanks,
>> Ilya
>
> Here is an updated version where chkp_maybe_fix_chkp_ref also removes duplicating references which may appear during cgraph nodes merge.  Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?
>
>
> Thanks,
> Ilya
> --
> gcc/
>
> 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)
> +{
> +  /* Firstly check node needs IPA_REF_CHKP.  */
> +  if (node->instrumentation_clone
> +      || !node->instrumented_version)
> +    return;
> +
> +  /* Check we already have a proper IPA_REF_CHKP.
> +     Remove incorrect refs.  */
> +  int i;
> +  ipa_ref *ref = NULL;
> +  bool found = false;
> +  for (i = 0; node->iterate_reference (i, ref); i++)
> +    if (ref->use == IPA_REF_CHKP)
> +      {
> +       /* Found proper reference.  */
> +       if (ref->referred == node->instrumented_version
> +           && !found)
> +         found = true;
> +       else
> +         {
> +           ref->remove_reference ();
> +           i--;
> +         }
> +      }
> +
> +  if (!found)
> +    node->create_reference (node->instrumented_version, IPA_REF_CHKP, NULL);
> +}
> +
>  /* Return clone created for instrumentation of NODE or NULL.  */
>
>  cgraph_node *
> diff --git a/gcc/ipa-chkp.h b/gcc/ipa-chkp.h
> index 6708fe9..5fa7d88 100644
> --- a/gcc/ipa-chkp.h
> +++ b/gcc/ipa-chkp.h
> @@ -24,5 +24,6 @@ extern tree chkp_copy_function_type_adding_bounds (tree orig_type);
>  extern tree chkp_maybe_clone_builtin_fndecl (tree fndecl);
>  extern cgraph_node *chkp_maybe_create_clone (tree fndecl);
>  extern bool chkp_instrumentable_p (tree fndecl);
> +extern void chkp_maybe_fix_chkp_ref (cgraph_node *node);
>
>  #endif /* GCC_IPA_CHKP_H */
> 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);
> +               }
> +           }
> +
>           /* If any reachable function has simd clones, mark them as
>              reachable as well.  */
>           if (cnode->simd_clones)
> diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
> index c875fed..b9196eb 100644
> --- a/gcc/lto-cgraph.c
> +++ b/gcc/lto-cgraph.c
> @@ -80,6 +80,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "pass_manager.h"
>  #include "ipa-utils.h"
>  #include "omp-low.h"
> +#include "ipa-chkp.h"
>
>  /* True when asm nodes has been output.  */
>  bool asm_nodes_output = false;
> @@ -1888,6 +1889,10 @@ input_symtab (void)
>          context of the nested function.  */
>        if (node->lto_file_data)
>         node->aux = NULL;
> +
> +      /* May need to fix chkp reference because we don't stream
> +        them for boundary symbols.  */
> +      chkp_maybe_fix_chkp_ref (node);
>      }
>  }
>
> diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c
> index 235b735..7d117e9 100644
> --- a/gcc/lto/lto-partition.c
> +++ b/gcc/lto/lto-partition.c
> @@ -877,27 +877,13 @@ validize_symbol_for_target (symtab_node *node)
>      }
>  }
>
> -/* 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.  */
> +/* Helper for privatize_symbol_name.  Mangle NODE symbol name
> +   represented by DECL.  */
>
>  static bool
> -privatize_symbol_name (symtab_node *node)
> +privatize_symbol_name_1 (symtab_node *node, tree decl)
>  {
> -  tree decl = node->decl;
> -  const char *name;
> -  cgraph_node *cnode = dyn_cast <cgraph_node *> (node);
> -
> -  /* If we want to privatize instrumentation clone
> -     then we need to change original function name
> -     which is used via transparent alias chain.  */
> -  if (cnode && cnode->instrumentation_clone)
> -    decl = cnode->orig_decl;
> -
> -  name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
> +  const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
>
>    if (must_not_rename (node, name))
>      return false;
> @@ -906,31 +892,57 @@ privatize_symbol_name (symtab_node *node)
>    symtab->change_decl_assembler_name (decl,
>                                       clone_function_name_1 (name,
>                                                              "lto_priv"));
> +
>    if (node->lto_file_data)
>      lto_record_renamed_decl (node->lto_file_data, name,
>                              IDENTIFIER_POINTER
>                              (DECL_ASSEMBLER_NAME (decl)));
> +
> +  if (symtab->dump_file)
> +    fprintf (symtab->dump_file,
> +            "Privatizing symbol name: %s -> %s\n",
> +            name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)));
> +
> +  return true;
> +}
> +
> +/* 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);
> +         iname = DECL_ASSEMBLER_NAME (cnode->decl);
> +         TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->orig_decl);
> +       }
>        else if (cnode->instrumented_version
> -              && cnode->instrumented_version->orig_decl == decl)
> -       iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl);
> -
> -      if (iname)
> +              && cnode->instrumented_version->orig_decl == cnode->decl)
>         {
> -         gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname));
> -         TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl);
> +         iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl);
> +         TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->decl);
>         }
>      }
> -  if (symtab->dump_file)
> -    fprintf (symtab->dump_file,
> -           "Privatizing symbol name: %s -> %s\n",
> -           name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)));
> +
>    return true;
>  }
>
> diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c
> index c00fd87..e0b9762 100644
> --- a/gcc/lto/lto-symtab.c
> +++ b/gcc/lto/lto-symtab.c
> @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "ipa-prop.h"
>  #include "ipa-inline.h"
>  #include "builtins.h"
> +#include "ipa-chkp.h"
>
>  /* Replace the cgraph node NODE with PREVAILING_NODE in the cgraph, merging
>     all edges and removing the old node.  */
> @@ -116,7 +117,11 @@ lto_cgraph_replace_node (struct cgraph_node *node,
>                   == prevailing_node->instrumentation_clone);
>        node->instrumented_version->instrumented_version = prevailing_node;
>        if (!prevailing_node->instrumented_version)
> -       prevailing_node->instrumented_version = node->instrumented_version;
> +       {
> +         prevailing_node->instrumented_version = node->instrumented_version;
> +         chkp_maybe_fix_chkp_ref (prevailing_node);
> +       }
> +      chkp_maybe_fix_chkp_ref (node->instrumented_version);
>        /* Need to reset node->instrumented_version to NULL,
>          otherwise node removal code would reset
>          node->instrumented_version->instrumented_version.  */
> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c
> new file mode 100644
> index 0000000..2054aa15
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c
> @@ -0,0 +1,17 @@
> +/* { dg-lto-do link } */
> +/* { dg-require-effective-target mpx } */
> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */
> +
> +extern int __attribute__((noinline)) f1 (int i);
> +
> +static int __attribute__((noinline))
> +f2 (int i)
> +{
> +  return i + 6;
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  return f1 (argc) + f2 (argc);
> +}
> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c
> new file mode 100644
> index 0000000..4fa8656
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c
> @@ -0,0 +1,11 @@
> +static int __attribute__((noinline))
> +f2 (int i)
> +{
> +  return 2 * i;
> +}
> +
> +int __attribute__((noinline))
> +f1 (int i)
> +{
> +  return f2 (i) + 10;
> +}
> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c
> new file mode 100644
> index 0000000..be7f601
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c
> @@ -0,0 +1,18 @@
> +/* { dg-lto-do link } */
> +/* { dg-require-effective-target mpx } */
> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */
> +
> +static int
> +__attribute__ ((noinline))
> +func1 (int i)
> +{
> +  return i + 2;
> +}
> +
> +extern int func2 (int i);
> +
> +int
> +main (int argc, char **argv)
> +{
> +  return func1 (argc) + func2 (argc) + 1;
> +}
> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c
> new file mode 100644
> index 0000000..db39e7f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c
> @@ -0,0 +1,8 @@
> +int func1 = 10;
> +
> +int
> +func2 (int i)
> +{
> +  func1++;
> +  return i + func1;
> +}

> 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

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

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

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

Honza
> 
> Thanks
> Ilya

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

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

Yeah, but it already special cases aliases (because we now always preserve IPA_REF_ALIAS link
in the boundary, so I think making IPA_REF_CHKP to be rpeserved should go the same path)
so we can special case the instrumentation thunks too?
> 
> >
> > 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?

Changing of decl_assembler_name is not very common and if we set up the links
only after renaming of statics, i think we are safe. But it would be better to
keep verify it at some point.

I suppose verify_node check that there is no transparent alias link on symbols
were it is not supposed to be and that there is link when it is supposed to be (i.e.
symtab_state is >=EXPANSION and symbol is either weakref on tragets w/o native weakrefs
or instrumentation cone.

Would you please mind adding the test and making sure it triggers when things are
out of sync?

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

OK, which should not be a problem if we stream the links instead of rebuilding them, right?

Honza
> 
> Thanks,
> Ilya
> 
> >
> > Honza
> >>
> >> Thanks
> >> Ilya