| 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). | expand |
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 > >
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; +}
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 >>> >>> >
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 > >>> > >>> > >
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; +}