diff mbox

[PR,59176] Mark "zombie" call graph nodes to remove verifier false positive

Message ID 20140320160732.GK19304@virgil.suse
State New
Headers show

Commit Message

Martin Jambor March 20, 2014, 4:07 p.m. UTC 
Hi,

in the PR, verifier claims an edge is pointing to a wrong declaration
even though it has successfully verified the edge multiple times
before.  The reason is that symtab_remove_unreachable_nodes decides to
"remove the body" of a node and also clear any information that it is
an alias of another in the process (more detailed analysis in comment
#9 of the bug).

In bugzilla Honza wrote that "silencing the verifier" is the way to
go.  Either we can dedicate a new flag in each cgraph_node or
symtab_node just for the purpose of verification or do something more
hackish like the patch below which re-uses the former_clone_of field
for this purpose.  Since clones are always private nodes, they should
always either survive removal of unreachable nodes or be completely
killed by it and should never enter the in_border zombie state.
Therefore their former_clone_of must always be NULL.  So I added a new
special value, error_mark_node, to mark this zombie state and taught
the verifier to be happy with such nodes.

Bootstrapped and tested on x86_64-linux.  What do you think?

Thanks,

Martin


2014-03-19  Martin Jambor  <mjambor@suse.cz>

	PR ipa/59176
	* ipa.c (symtab_remove_unreachable_nodes): Assert nodes in border
	are not former clones.  Set their former_clone_of to
	error_mark_node.
	* cgraphclones.c (cgraph_materialize_clone): Do not copy
	error_mark_node former_clone_of.
	* cgraph.c (verify_edge_corresponds_to_fndecl): Ignore nodes with
	error_mark_node former_clone_of.

testsuite/
	* g++.dg/torture/pr59176.C: New test.

Comments

Jakub Jelinek March 20, 2014, 6:40 p.m. UTC | #1 
On Thu, Mar 20, 2014 at 05:07:32PM +0100, Martin Jambor wrote:
> in the PR, verifier claims an edge is pointing to a wrong declaration
> even though it has successfully verified the edge multiple times
> before.  The reason is that symtab_remove_unreachable_nodes decides to
> "remove the body" of a node and also clear any information that it is
> an alias of another in the process (more detailed analysis in comment
> #9 of the bug).
> 
> In bugzilla Honza wrote that "silencing the verifier" is the way to
> go.  Either we can dedicate a new flag in each cgraph_node or
> symtab_node just for the purpose of verification or do something more
> hackish like the patch below which re-uses the former_clone_of field
> for this purpose.  Since clones are always private nodes, they should
> always either survive removal of unreachable nodes or be completely
> killed by it and should never enter the in_border zombie state.
> Therefore their former_clone_of must always be NULL.  So I added a new
> special value, error_mark_node, to mark this zombie state and taught
> the verifier to be happy with such nodes.
> 
> Bootstrapped and tested on x86_64-linux.  What do you think?

Don't we have like 22 spare bits in cgraph_node and 20 spare bits in
symtab_node?  I'd find it clearer if you just used a new flag to mark the
zombie nodes.  Though, I'll let Richard or Honza to decide, don't feel
strongly about it.

	Jakub
diff mbox

Patch

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index a15b6bc..3bf5ecd 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1935,6 +1935,8 @@  dump_cgraph_node (FILE *f, struct cgraph_node *node)
     fprintf (f, "  Clone of %s/%i\n",
 	     node->clone_of->asm_name (),
 	     node->clone_of->order);
+  if (node->former_clone_of == error_mark_node)
+    fprintf (f, "  Body removed by symtab_remove_unreachable_nodes\n");
   if (cgraph_function_flags_ready)
     fprintf (f, "  Availability: %s\n",
 	     cgraph_availability_names [cgraph_function_body_availability (node)]);
@@ -2602,7 +2604,10 @@  verify_edge_corresponds_to_fndecl (struct cgraph_edge *e, tree decl)
 
   /* We do not know if a node from a different partition is an alias or what it
      aliases and therefore cannot do the former_clone_of check reliably.  */
-  if (!node || node->in_other_partition || e->callee->in_other_partition)
+  if (!node
+      || node->former_clone_of == error_mark_node
+      || node->in_other_partition
+      || e->callee->in_other_partition)
     return false;
   node = cgraph_function_or_thunk_node (node, NULL);
 
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index ca69033..ba08281 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -914,7 +914,8 @@  cgraph_materialize_clone (struct cgraph_node *node)
 {
   bitmap_obstack_initialize (NULL);
   node->former_clone_of = node->clone_of->decl;
-  if (node->clone_of->former_clone_of)
+  if (node->clone_of->former_clone_of
+      && node->clone_of->former_clone_of != error_mark_node)
     node->former_clone_of = node->clone_of->former_clone_of;
   /* Copy the OLD_VERSION_NODE function tree to the new version.  */
   tree_function_versioning (node->clone_of->decl, node->decl,
diff --git a/gcc/ipa.c b/gcc/ipa.c
index 572dba1..85c73ef 100644
--- a/gcc/ipa.c
+++ b/gcc/ipa.c
@@ -484,6 +484,8 @@  symtab_remove_unreachable_nodes (bool before_inlining_p, FILE *file)
 	    {
 	      if (file)
 		fprintf (file, " %s", node->name ());
+	      gcc_assert (!node->former_clone_of);
+	      node->former_clone_of = error_mark_node;
 	      node->analyzed = false;
 	      node->definition = false;
 	      node->cpp_implicit_alias = false;
diff --git a/gcc/testsuite/g++.dg/ipa/pr59176.C b/gcc/testsuite/g++.dg/ipa/pr59176.C
new file mode 100644
index 0000000..d576bc3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr59176.C
@@ -0,0 +1,41 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+template <class> class A {
+protected:
+  void m_fn2();
+  ~A() { m_fn2(); }
+  virtual void m_fn1();
+};
+
+class D : A<int> {};
+template <class Key> void A<Key>::m_fn2() {
+  m_fn1();
+  m_fn1();
+  m_fn1();
+}
+
+#pragma interface
+class B {
+  D m_cellsAlreadyProcessed;
+  D m_cellsNotToProcess;
+
+public:
+  virtual ~B() {}
+  void m_fn1();
+};
+
+class C {
+  unsigned long m_fn1();
+  B m_fn2();
+  unsigned long m_fn3();
+};
+unsigned long C::m_fn1() {
+CellHierarchy:
+  m_fn2().m_fn1();
+}
+
+unsigned long C::m_fn3() {
+CellHierarchy:
+  m_fn2().m_fn1();
+}