public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Jason Merrill <jason@redhat.com>
Cc: gcc-patches@gcc.gnu.org, Jan Hubicka <jh@suse.cz>,
	Richard Biener <rguenther@suse.de>,
	Patrick Palka <ppalka@redhat.com>,
	Jonathan Wakely <jwakely@redhat.com>
Subject: Re: [PATCH] c++, v2: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208]
Date: Tue, 23 Apr 2024 18:04:57 +0200	[thread overview]
Message-ID: <ZifcKXwtttpezQFO@tucnak> (raw)
In-Reply-To: <5fe9db40-4bff-4be4-a434-6509133f26e3@redhat.com>

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


  reply	other threads:[~2024-04-23 16:05 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-17  7:42 [PATCH] c++: " Jakub Jelinek
2024-04-17  9:04 ` Jan Hubicka
2024-04-17 12:32   ` Jakub Jelinek
2024-04-17 13:26     ` Jan Hubicka
2024-04-17 14:07       ` Jakub Jelinek
2024-04-17 14:34         ` Jan Hubicka
2024-04-17 14:39           ` Jakub Jelinek
2024-04-22 15:42 ` [PATCH] c++, v2: " Jakub Jelinek
2024-04-23  3:14   ` Jason Merrill
2024-04-23 16:04     ` Jakub Jelinek [this message]
2024-04-24  9:16       ` Jonathan Wakely
2024-04-24 16:16         ` [PATCH] c++, v3: " Jakub Jelinek
2024-04-24 22:39           ` Jason Merrill
2024-04-24 22:47             ` Jakub Jelinek
2024-04-25  0:43               ` Jason Merrill
2024-04-25 12:02                 ` [PATCH] c++, v4: " Jakub Jelinek
2024-04-25 14:22                   ` Jakub Jelinek
2024-04-25 15:30                     ` Jason Merrill
2024-04-25 18:42                       ` [PATCH] c++, v5: " Jakub Jelinek
2024-05-09 18:20                       ` [PATCH] c++: Optimize in maybe_clone_body aliases even when not at_eof [PR113208] Jakub Jelinek
2024-05-09 18:58                         ` Marek Polacek
2024-05-09 19:05                           ` Jakub Jelinek
2024-05-10 19:59                         ` Jason Merrill
2024-05-13 10:19                           ` Jakub Jelinek
2024-05-14 22:20                             ` Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZifcKXwtttpezQFO@tucnak \
    --to=jakub@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jason@redhat.com \
    --cc=jh@suse.cz \
    --cc=jwakely@redhat.com \
    --cc=ppalka@redhat.com \
    --cc=rguenther@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).