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

[Jan Hubicka <hubicka@ucw.cz>]

> Ping

I am really sorry for ignoring this so long - I would like to reorg the code
and replace instrumentaiton thunks by the notion of transparent aliases,
but did not have time to do that yet.  Have quite busy time now.
> >>>
> >>> 2015-04-14  Ilya Enkovich  <ilya.enkovich@intel.com>
> >>>
> >>>         * ipa.c (symbol_table::remove_unreachable_nodes): Don't
> >>>         remove instumentation thunks calling reachable functions.
> >>>         * lto-cgraph.c (output_refs): Always output IPA_REF_CHKP.
> >>>         * lto/lto-partition.c (privatize_symbol_name_1): New.
> >>>         (privatize_symbol_name): Privatize both decl and orig_decl
> >>>         names for instrumented functions.
> >>>
> >>> gcc/testsuite/
> >>>
> >>> 2015-04-14  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.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)

reachable.contains (cnode) is !in_boundary_p.

> >>> +             && 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);

Why do you need the other tests.  Can we have instrumentation node that is not definition?
I suppose you can remove if (cnode->instrumented_version) because you assert it anyway.

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

I think we ought to have verify_symtab_node checking for this.  All the handling of the
name links seems somewhat fragile (I am mostly concerned about getting it right for
LTO before remaning takes place)

OK with the changes above for mainline and for branch if it does not cause problems
for a week.

Honza
> >>>    return true;
> >>>  }
> >>>
> >>> 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;
> >>> +}