Do not ICE for incomplete types in ICF (PR ipa/85607).

Message ID b72d6a61-bf51-cd45-f2b0-fcf41a92580c@suse.cz
State New
Headers show
Series
  • Do not ICE for incomplete types in ICF (PR ipa/85607).
Related show

Commit Message

Martin Liška May 10, 2018, 7:58 a.m.
Hi.

It's removal of an assert at place where we calculate hash of a type.
For incomplete types, let's skip it.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Martin


gcc/ChangeLog:

2018-05-09  Martin Liska  <mliska@suse.cz>

	PR ipa/85607
	* ipa-icf.c (sem_item::add_type): Do not ICE for incomplete types.

gcc/testsuite/ChangeLog:

2018-05-09  Martin Liska  <mliska@suse.cz>

	PR ipa/85607
	* g++.dg/ipa/pr85606.C: New test.
---
 gcc/ipa-icf.c                      |  5 ++++-
 gcc/testsuite/g++.dg/ipa/pr85606.C | 14 ++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr85606.C

Comments

Richard Biener May 11, 2018, 9:35 a.m. | #1
On Thu, May 10, 2018 at 9:58 AM, Martin Liška <mliska@suse.cz> wrote:
> Hi.
>
> It's removal of an assert at place where we calculate hash of a type.
> For incomplete types, let's skip it.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

Seems to be a redundant check in the !val case as well.  Also why not
at least do

          hstate2.add_int (RECORD_TYPE);

for incomplete types?  That said, your patch fixes the ICE but what
is supposed to happen for incomplete types?  Note that with LTO
we no longer "complete" types so you can see a mix of struct S;
and struct S { .... }; in the IL.  It looks like comparison later just
looks at types_compatible_p here.

Anyway, please at least remove the other redundant assert.

Thanks,
Richard.

> Martin
>
>
> gcc/ChangeLog:
>
> 2018-05-09  Martin Liska  <mliska@suse.cz>
>
>         PR ipa/85607
>         * ipa-icf.c (sem_item::add_type): Do not ICE for incomplete types.
>
> gcc/testsuite/ChangeLog:
>
> 2018-05-09  Martin Liska  <mliska@suse.cz>
>
>         PR ipa/85607
>         * g++.dg/ipa/pr85606.C: New test.
> ---
>  gcc/ipa-icf.c                      |  5 ++++-
>  gcc/testsuite/g++.dg/ipa/pr85606.C | 14 ++++++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr85606.C
>
>
Martin Liška May 11, 2018, 1:12 p.m. | #2
On 05/11/2018 11:35 AM, Richard Biener wrote:
> On Thu, May 10, 2018 at 9:58 AM, Martin Liška <mliska@suse.cz> wrote:
>> Hi.
>>
>> It's removal of an assert at place where we calculate hash of a type.
>> For incomplete types, let's skip it.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
> 
> Seems to be a redundant check in the !val case as well.  Also why not
> at least do
> 
>           hstate2.add_int (RECORD_TYPE);
> 
> for incomplete types? 

Thanks, done that.

 That said, your patch fixes the ICE but what
> is supposed to happen for incomplete types?  Note that with LTO
> we no longer "complete" types so you can see a mix of struct S;
> and struct S { .... }; in the IL.  It looks like comparison later just
> looks at types_compatible_p here.

Yes, that should be fine. The hashing of types is only an optimization.

> 
> Anyway, please at least remove the other redundant assert.

Done and tested.

Martin

> 
> Thanks,
> Richard.
> 
>> Martin
>>
>>
>> gcc/ChangeLog:
>>
>> 2018-05-09  Martin Liska  <mliska@suse.cz>
>>
>>         PR ipa/85607
>>         * ipa-icf.c (sem_item::add_type): Do not ICE for incomplete types.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2018-05-09  Martin Liska  <mliska@suse.cz>
>>
>>         PR ipa/85607
>>         * g++.dg/ipa/pr85606.C: New test.
>> ---
>>  gcc/ipa-icf.c                      |  5 ++++-
>>  gcc/testsuite/g++.dg/ipa/pr85606.C | 14 ++++++++++++++
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr85606.C
>>
>>
From d19f292793d2a034d6d29e85718ab1f42219a56a Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 9 May 2018 15:10:38 +0200
Subject: [PATCH] Do not ICE for incomplete types in ICF (PR ipa/85607).

gcc/ChangeLog:

2018-05-09  Martin Liska  <mliska@suse.cz>

	PR ipa/85607
	* ipa-icf.c (sem_item::add_type): Do not ICE for incomplete types.

gcc/testsuite/ChangeLog:

2018-05-09  Martin Liska  <mliska@suse.cz>

	PR ipa/85607
	* g++.dg/ipa/pr85606.C: New test.
---
 gcc/ipa-icf.c                      | 10 +++++++---
 gcc/testsuite/g++.dg/ipa/pr85606.C | 14 ++++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr85606.C

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index f974d9f769f..37e63fc2ba8 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1580,7 +1580,13 @@ sem_item::add_type (const_tree type, inchash::hash &hstate)
     }
   else if (RECORD_OR_UNION_TYPE_P (type))
     {
-      gcc_checking_assert (COMPLETE_TYPE_P (type));
+      /* Incomplete types must be skipped here.  */
+      if (!COMPLETE_TYPE_P (type))
+	{
+	  hstate.add_int (RECORD_TYPE);
+	  return;
+	}
+
       hashval_t *val = optimizer->m_type_hash_cache.get (type);
 
       if (!val)
@@ -1591,8 +1597,6 @@ sem_item::add_type (const_tree type, inchash::hash &hstate)
 	  hashval_t hash;
 
 	  hstate2.add_int (RECORD_TYPE);
-	  gcc_assert (COMPLETE_TYPE_P (type));
-
 	  for (f = TYPE_FIELDS (type), nf = 0; f; f = TREE_CHAIN (f))
 	    if (TREE_CODE (f) == FIELD_DECL)
 	      {
diff --git a/gcc/testsuite/g++.dg/ipa/pr85606.C b/gcc/testsuite/g++.dg/ipa/pr85606.C
new file mode 100644
index 00000000000..b47aba2167d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr85606.C
@@ -0,0 +1,14 @@
+// { dg-do compile }
+/* { dg-options "-O2" } */
+
+class A;	// { dg-message "forward declaration of 'class A'" }
+
+A *a;		// { dg-warning "'a' has incomplete type" }
+
+int
+main (int argc, char **argv)
+{
+  delete a;	// { dg-warning "delete" "warn" }
+  // { dg-message "note" "note" { target *-*-* } .-1 }
+  return 0;
+}
Martin Liška May 21, 2018, 7:27 a.m. | #3
PING^1

On 05/11/2018 03:12 PM, Martin Liška wrote:
> On 05/11/2018 11:35 AM, Richard Biener wrote:
>> On Thu, May 10, 2018 at 9:58 AM, Martin Liška <mliska@suse.cz> wrote:
>>> Hi.
>>>
>>> It's removal of an assert at place where we calculate hash of a type.
>>> For incomplete types, let's skip it.
>>>
>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>
>>> Ready to be installed?
>>
>> Seems to be a redundant check in the !val case as well.  Also why not
>> at least do
>>
>>           hstate2.add_int (RECORD_TYPE);
>>
>> for incomplete types? 
> 
> Thanks, done that.
> 
>  That said, your patch fixes the ICE but what
>> is supposed to happen for incomplete types?  Note that with LTO
>> we no longer "complete" types so you can see a mix of struct S;
>> and struct S { .... }; in the IL.  It looks like comparison later just
>> looks at types_compatible_p here.
> 
> Yes, that should be fine. The hashing of types is only an optimization.
> 
>>
>> Anyway, please at least remove the other redundant assert.
> 
> Done and tested.
> 
> Martin
> 
>>
>> Thanks,
>> Richard.
>>
>>> Martin
>>>
>>>
>>> gcc/ChangeLog:
>>>
>>> 2018-05-09  Martin Liska  <mliska@suse.cz>
>>>
>>>         PR ipa/85607
>>>         * ipa-icf.c (sem_item::add_type): Do not ICE for incomplete types.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2018-05-09  Martin Liska  <mliska@suse.cz>
>>>
>>>         PR ipa/85607
>>>         * g++.dg/ipa/pr85606.C: New test.
>>> ---
>>>  gcc/ipa-icf.c                      |  5 ++++-
>>>  gcc/testsuite/g++.dg/ipa/pr85606.C | 14 ++++++++++++++
>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr85606.C
>>>
>>>
>
Richard Biener May 22, 2018, 9:12 a.m. | #4
On Mon, May 21, 2018 at 9:27 AM Martin Liška <mliska@suse.cz> wrote:

> PING^1

OK.

> On 05/11/2018 03:12 PM, Martin Liška wrote:
> > On 05/11/2018 11:35 AM, Richard Biener wrote:
> >> On Thu, May 10, 2018 at 9:58 AM, Martin Liška <mliska@suse.cz> wrote:
> >>> Hi.
> >>>
> >>> It's removal of an assert at place where we calculate hash of a type.
> >>> For incomplete types, let's skip it.
> >>>
> >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>>
> >>> Ready to be installed?
> >>
> >> Seems to be a redundant check in the !val case as well.  Also why not
> >> at least do
> >>
> >>           hstate2.add_int (RECORD_TYPE);
> >>
> >> for incomplete types?
> >
> > Thanks, done that.
> >
> >  That said, your patch fixes the ICE but what
> >> is supposed to happen for incomplete types?  Note that with LTO
> >> we no longer "complete" types so you can see a mix of struct S;
> >> and struct S { .... }; in the IL.  It looks like comparison later just
> >> looks at types_compatible_p here.
> >
> > Yes, that should be fine. The hashing of types is only an optimization.
> >
> >>
> >> Anyway, please at least remove the other redundant assert.
> >
> > Done and tested.
> >
> > Martin
> >
> >>
> >> Thanks,
> >> Richard.
> >>
> >>> Martin
> >>>
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> 2018-05-09  Martin Liska  <mliska@suse.cz>
> >>>
> >>>         PR ipa/85607
> >>>         * ipa-icf.c (sem_item::add_type): Do not ICE for incomplete
types.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>> 2018-05-09  Martin Liska  <mliska@suse.cz>
> >>>
> >>>         PR ipa/85607
> >>>         * g++.dg/ipa/pr85606.C: New test.
> >>> ---
> >>>  gcc/ipa-icf.c                      |  5 ++++-
> >>>  gcc/testsuite/g++.dg/ipa/pr85606.C | 14 ++++++++++++++
> >>>  2 files changed, 18 insertions(+), 1 deletion(-)
> >>>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr85606.C
> >>>
> >>>
> >

Patch

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index f974d9f769f..7ecd0380fb7 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1580,7 +1580,10 @@  sem_item::add_type (const_tree type, inchash::hash &hstate)
     }
   else if (RECORD_OR_UNION_TYPE_P (type))
     {
-      gcc_checking_assert (COMPLETE_TYPE_P (type));
+      /* Incomplete types must be skipped here.  */
+      if (!COMPLETE_TYPE_P (type))
+	return;
+
       hashval_t *val = optimizer->m_type_hash_cache.get (type);
 
       if (!val)
diff --git a/gcc/testsuite/g++.dg/ipa/pr85606.C b/gcc/testsuite/g++.dg/ipa/pr85606.C
new file mode 100644
index 00000000000..b47aba2167d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr85606.C
@@ -0,0 +1,14 @@ 
+// { dg-do compile }
+/* { dg-options "-O2" } */
+
+class A;	// { dg-message "forward declaration of 'class A'" }
+
+A *a;		// { dg-warning "'a' has incomplete type" }
+
+int
+main (int argc, char **argv)
+{
+  delete a;	// { dg-warning "delete" "warn" }
+  // { dg-message "note" "note" { target *-*-* } .-1 }
+  return 0;
+}