[PATCH] Fix PR ipa/65722

[PATCH] Fix PR ipa/65722

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

Hello.

Attached patch fixes PR, where we should consider just cgraph_nodes as
objects referred by virtual table.

Patch survives regression tests on x86_64-linux-pc with enabled checking.

Ready for trunk?
Thanks,
Martin

[-- Attachment #2: 0001-Fix-PR-ipa-65722.patch --]
[-- Type: text/x-patch, Size: 1783 bytes --]

From 8bff38438f6ba57732a6c3ccc3632f6789ca4d7e Mon Sep 17 00:00:00 2001
From: mliska <mliska@suse.cz>
Date: Fri, 10 Apr 2015 11:37:04 +0200
Subject: [PATCH] Fix PR ipa/65722.

gcc/testsuite/ChangeLog:

2015-04-10  Martin Liska  <mliska@suse.cz>

	* g++.dg/ipa/pr65722.C: New test.

gcc/ChangeLog:

2015-04-10  Martin Liska  <mliska@suse.cz>

	PR ipa/65722
	* ipa-icf.c (sem_variable::equals_wpa): Consider comparsion just
	for references coming from cgraph nodes.
---
 gcc/ipa-icf.c                      |  4 ++++
 gcc/testsuite/g++.dg/ipa/pr65722.C | 21 +++++++++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr65722.C

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 8f8a0cf..9e5d19c 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1670,6 +1670,10 @@ sem_variable::equals_wpa (sem_item *item,
       /* DECL_FINAL_P flag on methods referred by virtual tables is used
 	 to decide on completeness possible_polymorphic_call_targets lists
 	 and therefore it must match.  */
+      if (!is_a <cgraph_node *> (ref->referred)
+	  || !is_a <cgraph_node *> (ref2->referred))
+	continue;
+
       if ((DECL_VIRTUAL_P (decl) || DECL_VIRTUAL_P (item->decl))
 	  && (DECL_VIRTUAL_P (ref->referred->decl)
 	      || DECL_VIRTUAL_P (ref2->referred->decl))
diff --git a/gcc/testsuite/g++.dg/ipa/pr65722.C b/gcc/testsuite/g++.dg/ipa/pr65722.C
new file mode 100644
index 0000000..ee4ea24
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr65722.C
@@ -0,0 +1,21 @@
+// { dg-do compile }
+// { dg-options "-O -fipa-icf -fno-rtti" }
+
+struct A
+{
+  virtual void f ()
+  {
+    __builtin_abort ();
+  }
+  virtual void g ();
+};
+
+struct B : virtual A { };
+struct C : B, virtual A { };
+
+void foo()
+{
+  C c;
+  C *p = &c;
+  p->f ();
+}
-- 
2.1.4

Re: [PATCH] Fix PR ipa/65722

[Jakub Jelinek]

On Fri, Apr 10, 2015 at 06:15:01PM +0200, Martin Liška wrote:
> Attached patch fixes PR, where we should consider just cgraph_nodes as
> objects referred by virtual table.
> 
> Patch survives regression tests on x86_64-linux-pc with enabled checking.
> 
> Ready for trunk?
> Thanks,
> Martin

> >From 8bff38438f6ba57732a6c3ccc3632f6789ca4d7e Mon Sep 17 00:00:00 2001
> From: mliska <mliska@suse.cz>
> Date: Fri, 10 Apr 2015 11:37:04 +0200
> Subject: [PATCH] Fix PR ipa/65722.
> 
> gcc/testsuite/ChangeLog:
> 
> 2015-04-10  Martin Liska  <mliska@suse.cz>
> 

Please add PR line here too.

> 	* g++.dg/ipa/pr65722.C: New test.
> 
> gcc/ChangeLog:
> 
> 2015-04-10  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/65722
> 	* ipa-icf.c (sem_variable::equals_wpa): Consider comparsion just
> 	for references coming from cgraph nodes.
> ---
>  gcc/ipa-icf.c                      |  4 ++++
>  gcc/testsuite/g++.dg/ipa/pr65722.C | 21 +++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr65722.C
> 
> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index 8f8a0cf..9e5d19c 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -1670,6 +1670,10 @@ sem_variable::equals_wpa (sem_item *item,
>        /* DECL_FINAL_P flag on methods referred by virtual tables is used
>  	 to decide on completeness possible_polymorphic_call_targets lists
>  	 and therefore it must match.  */
> +      if (!is_a <cgraph_node *> (ref->referred)
> +	  || !is_a <cgraph_node *> (ref2->referred))
> +	continue;
> +

Wouldn't it be better to move this check before the DECL_FINAL_P comment, so
that it is closer to the uses?

	Jakub

Re: [PATCH] Fix PR ipa/65722

[Jan Hubicka]

> 2015-04-10  Martin Liska  <mliska@suse.cz>
> 
> 	* g++.dg/ipa/pr65722.C: New test.
> 
> gcc/ChangeLog:
> 
> 2015-04-10  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/65722
> 	* ipa-icf.c (sem_variable::equals_wpa): Consider comparsion just
> 	for references coming from cgraph nodes.

Please add into compare_cgraph_references to never return true when one
parameter is function and other variable. How it comes we do not get different
hash values here? Perhaps when adding the hash values of references, we sould
iteratively hash in some identifier saying if object is function or variable.

For vtables, we want to test DECL_VIRTUAL_P for match even for variables,
because we do not want to match RTTI and vtable (though it may be unlikely
that they are the same).
Refactor the code to test

/* For virtual tables we need to check flags used by ipa-devirt.  */
if (DECL_VIRTUAL_P (decl) || DECL_VIRTUAL_P (item->decl))
  {
    if (DECL_VIRTUAL_P (ref->referred->decl) != DECL_VIRTUAL_P (ref2->referred->decl))
      fail claiming that virutal flag mismatched
    if (is_a_funtion && DECL_VIRTUAL_P (ref->referred->decl)
        && DECL_FINAL (ref->referred->decl) != DECL_FINAL (ref2->referred->decl))
      fail chaliming that final flag mismatched
  }

The conditional is quite confusing written as it is. (probably by myself :)
Thanks!
Honza
> ---
>  gcc/ipa-icf.c                      |  4 ++++
>  gcc/testsuite/g++.dg/ipa/pr65722.C | 21 +++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr65722.C
> 
> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index 8f8a0cf..9e5d19c 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -1670,6 +1670,10 @@ sem_variable::equals_wpa (sem_item *item,
>        /* DECL_FINAL_P flag on methods referred by virtual tables is used
>  	 to decide on completeness possible_polymorphic_call_targets lists
>  	 and therefore it must match.  */
> +      if (!is_a <cgraph_node *> (ref->referred)
> +	  || !is_a <cgraph_node *> (ref2->referred))
> +	continue;
> +
>        if ((DECL_VIRTUAL_P (decl) || DECL_VIRTUAL_P (item->decl))
>  	  && (DECL_VIRTUAL_P (ref->referred->decl)
>  	      || DECL_VIRTUAL_P (ref2->referred->decl))
> diff --git a/gcc/testsuite/g++.dg/ipa/pr65722.C b/gcc/testsuite/g++.dg/ipa/pr65722.C
> new file mode 100644
> index 0000000..ee4ea24
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/pr65722.C
> @@ -0,0 +1,21 @@
> +// { dg-do compile }
> +// { dg-options "-O -fipa-icf -fno-rtti" }
> +
> +struct A
> +{
> +  virtual void f ()
> +  {
> +    __builtin_abort ();
> +  }
> +  virtual void g ();
> +};
> +
> +struct B : virtual A { };
> +struct C : B, virtual A { };
> +
> +void foo()
> +{
> +  C c;
> +  C *p = &c;
> +  p->f ();
> +}
> -- 
> 2.1.4
> 

Re: [PATCH] Fix PR ipa/65722

[Jan Hubicka]

> > 2015-04-10  Martin Liska  <mliska@suse.cz>
> > 
> > 	* g++.dg/ipa/pr65722.C: New test.
> > 
> > gcc/ChangeLog:
> > 
> > 2015-04-10  Martin Liska  <mliska@suse.cz>
> > 
> > 	PR ipa/65722
> > 	* ipa-icf.c (sem_variable::equals_wpa): Consider comparsion just
> > 	for references coming from cgraph nodes.
> 
> Please add into compare_cgraph_references to never return true when one
> parameter is function and other variable. How it comes we do not get different
> hash values here? Perhaps when adding the hash values of references, we sould
> iteratively hash in some identifier saying if object is function or variable.
> 
> For vtables, we want to test DECL_VIRTUAL_P for match even for variables,
> because we do not want to match RTTI and vtable (though it may be unlikely
> that they are the same).
> Refactor the code to test
> 
> /* For virtual tables we need to check flags used by ipa-devirt.  */
> if (DECL_VIRTUAL_P (decl) || DECL_VIRTUAL_P (item->decl))
>   {
>     if (DECL_VIRTUAL_P (ref->referred->decl) != DECL_VIRTUAL_P (ref2->referred->decl))
>       fail claiming that virutal flag mismatched
>     if (is_a_funtion && DECL_VIRTUAL_P (ref->referred->decl)
>         && DECL_FINAL (ref->referred->decl) != DECL_FINAL (ref2->referred->decl))
>       fail chaliming that final flag mismatched
>   }
> 
> The conditional is quite confusing written as it is. (probably by myself :)
> Thanks!
> Honza

Hi,
this is variant I am testing. 

Index: ipa-icf.c
===================================================================
--- ipa-icf.c	(revision 222010)
+++ ipa-icf.c	(working copy)
@@ -368,6 +368,10 @@ sem_item::compare_cgraph_references (
   if (n1 == n2)
     return true;
 
+  /* Never match variable and function.  */
+  if (is_a <varpool_node *> (n1) != is_a <varpool_node *> (n2))
+    return false;
+
   /* Merging two definitions with a reference to equivalent vtables, but
      belonging to a different type may result in ipa-polymorphic-call analysis
      giving a wrong answer about the dynamic type of instance.  */
@@ -587,9 +591,6 @@ void
 sem_item::update_hash_by_addr_refs (hash_map <symtab_node *,
 				    sem_item *> &m_symtab_node_map)
 {
-  if (is_a <varpool_node *> (node) && DECL_VIRTUAL_P (node->decl))
-    return;
-
   ipa_ref* ref;
   inchash::hash hstate (hash);
   for (unsigned i = 0; i < node->num_references (); i++)
@@ -1667,17 +1668,19 @@ sem_variable::equals_wpa (sem_item *item
 				      ref->address_matters_p ()))
 	return false;
 
-      /* DECL_FINAL_P flag on methods referred by virtual tables is used
-	 to decide on completeness possible_polymorphic_call_targets lists
-	 and therefore it must match.  */
-      if ((DECL_VIRTUAL_P (decl) || DECL_VIRTUAL_P (item->decl))
-	  && (DECL_VIRTUAL_P (ref->referred->decl)
-	      || DECL_VIRTUAL_P (ref2->referred->decl))
-	  && ((DECL_VIRTUAL_P (ref->referred->decl)
-	       != DECL_VIRTUAL_P (ref2->referred->decl))
-	      || (DECL_FINAL_P (ref->referred->decl)
-		  != DECL_FINAL_P (ref2->referred->decl))))
-        return return_false_with_msg ("virtual or final flag mismatch");
+      /* When matching virtual tables, be sure to also match information
+ 	 relevant for polymorphic call analysis.  */
+      if (DECL_VIRTUAL_P (decl) || DECL_VIRTUAL_P (item->decl))
+	{
+	  if (DECL_VIRTUAL_P (ref->referred->decl)
+	      != DECL_VIRTUAL_P (ref2->referred->decl))
+            return return_false_with_msg ("virtual flag mismatch");
+	  if (DECL_VIRTUAL_P (ref->referred->decl)
+	      && is_a <cgraph_node *> (ref->referred)
+	      && (DECL_FINAL_P (ref->referred->decl)
+		  != DECL_FINAL_P (ref2->referred->decl)))
+            return return_false_with_msg ("final flag mismatch");
+	}
     }
 
   return true;

Re: [PATCH] Fix PR ipa/65722

[Jan Hubicka]

Hi,
this is version of patch I comitted after testing at x86_64-linux firefox build
and bootstrapped/regtested ppc64-linux.

Honza

	Jan Hubicka  <hubicka@ucw.cz>
	Martin Liska  <mliska@suse.cz>

	PR ipa/65722
	* g++.dg/ipa/pr65722.C: New testcase.

	* ipa-icf.c (sem_item::compare_cgraph_references): function and
	variable can not match.
	(sem_item::update_hash_by_addr_refs): Fix handling of virtual tables.
	(sem_variable::equals_wpa): Fix checking of DECL_FINAL_P patch.
Index: testsuite/g++.dg/ipa/pr65722.C
===================================================================
--- testsuite/g++.dg/ipa/pr65722.C	(revision 0)
+++ testsuite/g++.dg/ipa/pr65722.C	(revision 0)
@@ -0,0 +1,21 @@
+// { dg-do compile }
+// { dg-options "-O -fipa-icf -fno-rtti" }
+
+struct A
+{
+  virtual void f ()
+  {
+    __builtin_abort ();
+  }
+  virtual void g ();
+};
+
+struct B : virtual A { };
+struct C : B, virtual A { };
+
+void foo()
+{
+  C c;
+  C *p = &c;
+  p->f ();
+}
Index: ipa-icf.c
===================================================================
--- ipa-icf.c	(revision 221977)
+++ ipa-icf.c	(working copy)
@@ -368,6 +368,10 @@ sem_item::compare_cgraph_references (
   if (n1 == n2)
     return true;
 
+  /* Never match variable and function.  */
+  if (is_a <varpool_node *> (n1) != is_a <varpool_node *> (n2))
+    return false;
+
   /* Merging two definitions with a reference to equivalent vtables, but
      belonging to a different type may result in ipa-polymorphic-call analysis
      giving a wrong answer about the dynamic type of instance.  */
@@ -587,9 +591,6 @@ void
 sem_item::update_hash_by_addr_refs (hash_map <symtab_node *,
 				    sem_item *> &m_symtab_node_map)
 {
-  if (is_a <varpool_node *> (node) && DECL_VIRTUAL_P (node->decl))
-    return;
-
   ipa_ref* ref;
   inchash::hash hstate (hash);
   for (unsigned i = 0; i < node->num_references (); i++)
@@ -1667,17 +1668,19 @@ sem_variable::equals_wpa (sem_item *item
 				      ref->address_matters_p ()))
 	return false;
 
-      /* DECL_FINAL_P flag on methods referred by virtual tables is used
-	 to decide on completeness possible_polymorphic_call_targets lists
-	 and therefore it must match.  */
-      if ((DECL_VIRTUAL_P (decl) || DECL_VIRTUAL_P (item->decl))
-	  && (DECL_VIRTUAL_P (ref->referred->decl)
-	      || DECL_VIRTUAL_P (ref2->referred->decl))
-	  && ((DECL_VIRTUAL_P (ref->referred->decl)
-	       != DECL_VIRTUAL_P (ref2->referred->decl))
-	      || (DECL_FINAL_P (ref->referred->decl)
-		  != DECL_FINAL_P (ref2->referred->decl))))
-        return return_false_with_msg ("virtual or final flag mismatch");
+      /* When matching virtual tables, be sure to also match information
+ 	 relevant for polymorphic call analysis.  */
+      if (DECL_VIRTUAL_P (decl) || DECL_VIRTUAL_P (item->decl))
+	{
+	  if (DECL_VIRTUAL_P (ref->referred->decl)
+	      != DECL_VIRTUAL_P (ref2->referred->decl))
+            return return_false_with_msg ("virtual flag mismatch");
+	  if (DECL_VIRTUAL_P (ref->referred->decl)
+	      && is_a <cgraph_node *> (ref->referred)
+	      && (DECL_FINAL_P (ref->referred->decl)
+		  != DECL_FINAL_P (ref2->referred->decl)))
+            return return_false_with_msg ("final flag mismatch");
+	}
     }
 
   return true;