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.
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
