On Tue, 23 Apr 2024 at 17:05, Jakub Jelinek <ja...@redhat.com> wrote: > > On Mon, Apr 22, 2024 at 11:14:35PM -0400, Jason Merrill wrote: > > > > The following testcase regressed with Marek's r14-5979 change, > > > > when pr113208_0.C is compiled where the ctor is marked constexpr, > > > > we no longer perform this optimization, where > > > > _ZN6vectorI12QualityValueEC2ERKS1_ was emitted in the > > > > _ZN6vectorI12QualityValueEC5ERKS1_ comdat group and > > > > _ZN6vectorI12QualityValueEC1ERKS1_ was made an alias to it, > > > > instead we emit _ZN6vectorI12QualityValueEC2ERKS1_ in > > > > _ZN6vectorI12QualityValueEC2ERKS1_ comdat group and the same > > > > content _ZN6vectorI12QualityValueEC1ERKS1_ as separate symbol in > > > > _ZN6vectorI12QualityValueEC1ERKS1_ comdat group. > > > > This seems like an ABI bug that could use a non-LTO testcase. > > Well, except for the issues it causes to LTO I think it is compatible, > worst case we get the body of the ctor duplicated in the executable > and the linker picks some of the weak symbols as the symbol definitions. > Anyway, I've added a non-LTO testcase for that in the patch below. > > > Hmm, cloning the bodies and then discarding them later seems like more extra > > work than creating the cgraph nodes. > > So, I've tried to handle that in tentative_decl_linkage, like that function > already handles functions declared inline except for implicit template > instantiations. If we expect that import_export_decl will do comdat_linkage > for the ctor later on do it right away. > > That fixes the testcases too, but seems to regress > +FAIL: libstdc++-abi/abi_check > on both x86_64-linux and i686-linux, in each case 8 symbols disappeared from > libstdc++.so.6: > _ZNSt12__shared_ptrINSt10filesystem7__cxx1128recursive_directory_iterator10_Dir_stackELN9__gnu_cxx12_Lock_policyE2EEC1Ev > _ZNSt12__shared_ptrINSt10filesystem4_DirELN9__gnu_cxx12_Lock_policyE2EEC1Ev > _ZNSt12__shared_ptrINSt10filesystem28recursive_directory_iterator10_Dir_stackELN9__gnu_cxx12_Lock_policyE2EEC1Ev > _ZNSt12__shared_ptrINSt10filesystem4_DirELN9__gnu_cxx12_Lock_policyE2EEC2Ev > _ZNSt12__shared_ptrINSt10filesystem7__cxx114_DirELN9__gnu_cxx12_Lock_policyE2EEC1Ev > _ZNSt12__shared_ptrINSt10filesystem7__cxx1128recursive_directory_iterator10_Dir_stackELN9__gnu_cxx12_Lock_policyE2EEC2Ev > _ZNSt12__shared_ptrINSt10filesystem28recursive_directory_iterator10_Dir_stackELN9__gnu_cxx12_Lock_policyE2EEC2Ev > _ZNSt12__shared_ptrINSt10filesystem7__cxx114_DirELN9__gnu_cxx12_Lock_policyE2EEC2Ev > > Will need to study why that happened, it might be that it was ok because > I think the filesystem stuff is unlike the rest compiled with no exported > templates, but would need at least some hacks in libstdc++ to preserve > previously exported symbols.
There are explicit instantiation definitions that should instantiate those types: src/c++17/fs_dir.cc:template class std::__shared_ptr<fs::_Dir>; src/c++17/fs_dir.cc:template class std::__shared_ptr<fs::recursive_directory_iterator::_Dir_stack>; src/c++17/fs_path.cc:template class std::__shared_ptr<const fs::filesystem_error::_Impl>; So the missing symbols should be present in cow-fs_dir.o and cow-fs_path.o > Still, feels like a risky change this late if it wouldn't break ABI of other > libraries. > > 2024-04-23 Jakub Jelinek <ja...@redhat.com> > > PR lto/113208 > * decl2.cc (tentative_decl_linkage): Use comdat_linkage also > for implicit instantiations of maybe in charge ctors/dtors > if -fimplicit-templates or -fimplicit-inline-templates and > -fweak and target supports aliases. > > * g++.dg/abi/comdat2.C: New test. > * g++.dg/lto/pr113208_0.C: New test. > * g++.dg/lto/pr113208_1.C: New file. > * g++.dg/lto/pr113208.h: New file. > > --- gcc/cp/decl2.cc.jj 2024-04-22 15:16:55.328548807 +0200 > +++ gcc/cp/decl2.cc 2024-04-23 09:52:18.993250442 +0200 > @@ -3314,7 +3314,16 @@ tentative_decl_linkage (tree decl) > to mark the functions at this point. */ > if (DECL_DECLARED_INLINE_P (decl) > && (!DECL_IMPLICIT_INSTANTIATION (decl) > - || DECL_DEFAULTED_FN (decl))) > + || DECL_DEFAULTED_FN (decl) > + /* For implicit instantiations of cdtors, > + if import_export_decl would use comdat linkage, > + make sure to use it right away, so that maybe_clone_body > + can use aliases. See PR113208. */ > + || (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl) > + && (flag_implicit_templates > + || flag_implicit_inline_templates) > + && flag_weak > + && TARGET_SUPPORTS_ALIASES))) > { > /* This function must have external linkage, as > otherwise DECL_INTERFACE_KNOWN would have been > --- gcc/testsuite/g++.dg/abi/comdat2.C.jj 2024-04-23 10:04:28.485964610 > +0200 > +++ gcc/testsuite/g++.dg/abi/comdat2.C 2024-04-23 10:05:24.757171194 +0200 > @@ -0,0 +1,26 @@ > +// PR lto/113208 > +// { dg-do compile { target { c++11 && { *-*-*gnu* } } } } > +// { dg-additional-options "-O2 -fkeep-inline-functions" } > +// { dg-final { scan-assembler "_ZN1BI1CEC5ERKS1_,comdat" } } > +// { dg-final { scan-assembler-not "_ZN1BI1CEC1ERKS1_,comdat" } } > +// { dg-final { scan-assembler-not "_ZN1BI1CEC2ERKS1_,comdat" } } > + > +template <typename T> > +struct A { > + int foo () const; > + A (int, int); > +}; > +template <typename T> > +struct B : A<T> { > + constexpr B (const B &x) : A<T> (1, x.foo ()) {} > + B () : A<T> (1, 2) {} > +}; > +struct C; > +struct D : B<C> {}; > +void bar (D); > + > +void > +baz (D x) > +{ > + bar (x); > +} > --- gcc/testsuite/g++.dg/lto/pr113208.h.jj 2024-04-23 09:44:07.523179110 > +0200 > +++ gcc/testsuite/g++.dg/lto/pr113208.h 2024-04-23 09:44:07.523179110 +0200 > @@ -0,0 +1,10 @@ > +template <typename _Tp> struct _Vector_base { > + int g() const; > + _Vector_base(int, int); > +}; > +template <typename _Tp> > +struct vector : _Vector_base<_Tp> { > + CONSTEXPR vector(const vector &__x) > + : _Vector_base<_Tp>(1, __x.g()) {} > + vector() : _Vector_base<_Tp>(1, 2) {} > +}; > --- gcc/testsuite/g++.dg/lto/pr113208_1.C.jj 2024-04-23 09:44:07.523179110 > +0200 > +++ gcc/testsuite/g++.dg/lto/pr113208_1.C 2024-04-23 09:44:07.523179110 > +0200 > @@ -0,0 +1,6 @@ > +#define CONSTEXPR > +#include "pr113208.h" > + > +struct QualityValue; > +vector<QualityValue> values1; > +vector<QualityValue> values{values1}; > --- gcc/testsuite/g++.dg/lto/pr113208_0.C.jj 2024-04-23 09:44:07.523179110 > +0200 > +++ gcc/testsuite/g++.dg/lto/pr113208_0.C 2024-04-23 09:44:07.523179110 > +0200 > @@ -0,0 +1,13 @@ > +// { dg-lto-do link } > +// { dg-lto-options { {-O1 -std=c++20 -flto}} } > +// { dg-extra-ld-options "-r -nostdlib -flinker-output=nolto-rel" } > +// { dg-require-linker-plugin "" } > + > +#define CONSTEXPR constexpr > +#include "pr113208.h" > + > +struct QualityValue; > +struct k : vector<QualityValue> {}; > + > +void m(k); > +void n(k i) { m(i); } > > > Jakub >