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

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

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;
> +}