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

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