Re: Fix two ICEs with C++ destructors and LTO
Hi Jan, > these two PRs are about C++ destructors that are not virtual but have > virtual alias. The devirtualization machinery walks from alias to symbol > and is then confused by not knowing the class symbol belongs to. > > I think it would make more sense to avoid these walks but that require > more of surgery, so for GCC9 I think it is best to stop freeing contexts > for desctructors. This does not save any stremaing because THIS pointer > of the destructor has the same type. > > Bootstrapped/regtested x86_64-linux, OK? > > Honza > > PR lto/87809 > PR lto/89335 > * tree.c (free_lang_data_in_decl): Do not free context of C++ > destrutors. > > * g++.dg/lto/pr87089_0.C: New testcase. > * g++.dg/lto/pr87089_1.C: New testcase. > * g++.dg/lto/pr89335_0.C: New testcase. this test FAILs on Solaris with the native ld: FAIL: g++.dg/lto/pr89335 cp_lto_pr89335_0.o assemble, -O2 -flto -Wsuggest-final-methods /vol/gcc/src/hg/trunk/local/gcc/testsuite/g++.dg/lto/pr89335_0.C:9:7: warning: Declaring virtual destructor of 'struct List' final would enable devirtualization of 1 call [-Wsuggest-final-methods] The same failure can be seen on Linux/x86_64 with -fno-use-linker-plugin. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: Fix two ICEs with C++ destructors and LTO
On Mon, 18 Mar 2019, Jan Hubicka wrote: > Hi, > these two PRs are about C++ destructors that are not virtual but have > virtual alias. The devirtualization machinery walks from alias to symbol > and is then confused by not knowing the class symbol belongs to. > > I think it would make more sense to avoid these walks but that require > more of surgery, so for GCC9 I think it is best to stop freeing contexts > for desctructors. This does not save any stremaing because THIS pointer > of the destructor has the same type. > > Bootstrapped/regtested x86_64-linux, OK? OK. Richard. > Honza > > PR lto/87809 > PR lto/89335 > * tree.c (free_lang_data_in_decl): Do not free context of C++ > destrutors. > > * g++.dg/lto/pr87089_0.C: New testcase. > * g++.dg/lto/pr87089_1.C: New testcase. > * g++.dg/lto/pr89335_0.C: New testcase. > Index: tree.c > === > --- tree.c(revision 269704) > +++ tree.c(working copy) > @@ -5772,10 +5772,16 @@ free_lang_data_in_decl (tree decl, struc > not do well with TREE_CHAIN pointers linking them. > > Also do not drop containing types for virtual methods and tables because > - these are needed by devirtualization. */ > + these are needed by devirtualization. > + C++ destructors are special because C++ frontends sometimes produces > + virtual destructor as an alias of non-virtual destructor. In > + devirutalization code we always walk through aliases and we need > + context to be preserved too. See PR89335 */ >if (TREE_CODE (decl) != FIELD_DECL >&& ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL) > - || !DECL_VIRTUAL_P (decl))) > + || (!DECL_VIRTUAL_P (decl) > + && (TREE_CODE (decl) != FUNCTION_DECL > + || !DECL_CXX_DESTRUCTOR_P (decl) > DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl)); > } > > Index: testsuite/g++.dg/lto/pr87089_0.C > === > --- testsuite/g++.dg/lto/pr87089_0.C (nonexistent) > +++ testsuite/g++.dg/lto/pr87089_0.C (working copy) > @@ -0,0 +1,21 @@ > +// { dg-lto-do link } > +// { dg-extra-ld-options "-r -nostdlib -flinker-output=nolto-rel" } > +namespace itpp { > +template void b(a *c) { c[0].~a(); } > +class CFix; > +template class d { > + void e(const char *); > + CFix *data; > +}; > +class CFix { > +public: > + virtual ~CFix(); > +}; > +template <> void d::e(const char *) { b(data); } > +} // namespace itpp > + > +int > +main (void) > +{ > + return 0; > +} > Index: testsuite/g++.dg/lto/pr87089_1.C > === > --- testsuite/g++.dg/lto/pr87089_1.C (nonexistent) > +++ testsuite/g++.dg/lto/pr87089_1.C (working copy) > @@ -0,0 +1,12 @@ > +namespace itpp { > +enum a { b }; > +class CFix { > +public: > + virtual ~CFix(); > +}; > +template class c : CFix { > + ~c() {} > +}; > +template class c<>; > +} // namespace itpp > + > Index: testsuite/g++.dg/lto/pr89335_0.C > === > --- testsuite/g++.dg/lto/pr89335_0.C (nonexistent) > +++ testsuite/g++.dg/lto/pr89335_0.C (working copy) > @@ -0,0 +1,16 @@ > +// { dg-lto-do link } > +// { dg-lto-options {{-O2 -flto -Wsuggest-final-methods}} } > +// { dg-extra-ld-options "-r -nostdlib -flinker-output=nolto-rel" } > +class Container > +{ > +public: > + virtual ~Container (); > +}; > +class List : public Container // { dg-lto-message "final would enable > devirtualization" } > +{ > +}; > +static List cache[256]; > +int main (void) > +{ > + return 0; > +} > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Fix two ICEs with C++ destructors and LTO
Hi, these two PRs are about C++ destructors that are not virtual but have virtual alias. The devirtualization machinery walks from alias to symbol and is then confused by not knowing the class symbol belongs to. I think it would make more sense to avoid these walks but that require more of surgery, so for GCC9 I think it is best to stop freeing contexts for desctructors. This does not save any stremaing because THIS pointer of the destructor has the same type. Bootstrapped/regtested x86_64-linux, OK? Honza PR lto/87809 PR lto/89335 * tree.c (free_lang_data_in_decl): Do not free context of C++ destrutors. * g++.dg/lto/pr87089_0.C: New testcase. * g++.dg/lto/pr87089_1.C: New testcase. * g++.dg/lto/pr89335_0.C: New testcase. Index: tree.c === --- tree.c (revision 269704) +++ tree.c (working copy) @@ -5772,10 +5772,16 @@ free_lang_data_in_decl (tree decl, struc not do well with TREE_CHAIN pointers linking them. Also do not drop containing types for virtual methods and tables because - these are needed by devirtualization. */ + these are needed by devirtualization. + C++ destructors are special because C++ frontends sometimes produces + virtual destructor as an alias of non-virtual destructor. In + devirutalization code we always walk through aliases and we need + context to be preserved too. See PR89335 */ if (TREE_CODE (decl) != FIELD_DECL && ((TREE_CODE (decl) != VAR_DECL && TREE_CODE (decl) != FUNCTION_DECL) - || !DECL_VIRTUAL_P (decl))) + || (!DECL_VIRTUAL_P (decl) + && (TREE_CODE (decl) != FUNCTION_DECL + || !DECL_CXX_DESTRUCTOR_P (decl) DECL_CONTEXT (decl) = fld_decl_context (DECL_CONTEXT (decl)); } Index: testsuite/g++.dg/lto/pr87089_0.C === --- testsuite/g++.dg/lto/pr87089_0.C(nonexistent) +++ testsuite/g++.dg/lto/pr87089_0.C(working copy) @@ -0,0 +1,21 @@ +// { dg-lto-do link } +// { dg-extra-ld-options "-r -nostdlib -flinker-output=nolto-rel" } +namespace itpp { +template void b(a *c) { c[0].~a(); } +class CFix; +template class d { + void e(const char *); + CFix *data; +}; +class CFix { +public: + virtual ~CFix(); +}; +template <> void d::e(const char *) { b(data); } +} // namespace itpp + +int +main (void) +{ + return 0; +} Index: testsuite/g++.dg/lto/pr87089_1.C === --- testsuite/g++.dg/lto/pr87089_1.C(nonexistent) +++ testsuite/g++.dg/lto/pr87089_1.C(working copy) @@ -0,0 +1,12 @@ +namespace itpp { +enum a { b }; +class CFix { +public: + virtual ~CFix(); +}; +template class c : CFix { + ~c() {} +}; +template class c<>; +} // namespace itpp + Index: testsuite/g++.dg/lto/pr89335_0.C === --- testsuite/g++.dg/lto/pr89335_0.C(nonexistent) +++ testsuite/g++.dg/lto/pr89335_0.C(working copy) @@ -0,0 +1,16 @@ +// { dg-lto-do link } +// { dg-lto-options {{-O2 -flto -Wsuggest-final-methods}} } +// { dg-extra-ld-options "-r -nostdlib -flinker-output=nolto-rel" } +class Container +{ +public: + virtual ~Container (); +}; +class List : public Container // { dg-lto-message "final would enable devirtualization" } +{ +}; +static List cache[256]; +int main (void) +{ + return 0; +}