Re: [PATCH] c++, v2: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208]

[Jonathan Wakely <jwakely@redhat.com>]

On Tue, 23 Apr 2024 at 17:05, Jakub Jelinek <jakub@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  <jakub@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
>