diff mbox series

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

Message ID ZipGV0B+YovJksu7@tucnak
State New
Headers show
Series c++, v4: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208] | expand

Commit Message

Jakub Jelinek April 25, 2024, 12:02 p.m. UTC 
On Wed, Apr 24, 2024 at 08:43:46PM -0400, Jason Merrill wrote:
> > Then can_alias_cdtor would return false, because it ends with:
> >    /* Don't use aliases for weak/linkonce definitions unless we can put both
> >       symbols in the same COMDAT group.  */
> >    return (DECL_INTERFACE_KNOWN (fn)
> >            && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fn))
> >            && (!DECL_ONE_ONLY (fn)
> >                || (HAVE_COMDAT_GROUP && DECL_WEAK (fn))));
> > Should we change that DECL_INTERFACE_KNOWN (fn) in there to
> > (DECL_INTERFACE_KNOWN (fn) || something) then and what that
> > something should be?  HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn)?
> 
> Yes, I think reorganize to
> 
> ((DECL_INTERFACE_KNOWN (fn) && !DECL_WEAK (fn) && !DECL_ONE_ONLY (fn))
>  || (HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn))

I've tried the following patch, but unfortunately that lead to large
number of regressions:
+FAIL: g++.dg/coroutines/torture/co-yield-04-complex-local-state.C (test for excess errors)
+FAIL: g++.dg/coroutines/torture/func-params-08.C (test for excess errors)
+FAIL: g++.dg/coroutines/torture/func-params-09-awaitable-parms.C (test for excess errors)
+FAIL: g++.dg/cpp0x/constexpr-initlist.C  -std=c++11 (test for excess errors)
+FAIL: g++.dg/cpp0x/constexpr-initlist.C  -std=c++14 (test for excess errors)
+FAIL: g++.dg/cpp0x/constexpr-initlist.C  -std=c++17 (test for excess errors)
+FAIL: g++.dg/cpp0x/constexpr-initlist.C  -std=c++20 (test for excess errors)
+FAIL: g++.dg/cpp0x/constexpr-initlist.C  -std=c++23 (test for excess errors)
+FAIL: g++.dg/cpp0x/constexpr-initlist.C  -std=c++26 (test for excess errors)
+FAIL: g++.dg/cpp0x/initlist25.C  -std=c++11 (test for excess errors)
+FAIL: g++.dg/cpp0x/initlist25.C  -std=c++14 (test for excess errors)
+FAIL: g++.dg/cpp0x/initlist25.C  -std=c++17 (test for excess errors)
+FAIL: g++.dg/cpp0x/initlist25.C  -std=c++20 (test for excess errors)
+FAIL: g++.dg/cpp0x/initlist25.C  -std=c++23 (test for excess errors)
+FAIL: g++.dg/cpp0x/initlist25.C  -std=c++26 (test for excess errors)
+FAIL: g++.dg/cpp1y/pr95226.C  -std=c++20 (test for excess errors)
+FAIL: g++.dg/cpp1y/pr95226.C  -std=c++23 (test for excess errors)
+FAIL: g++.dg/cpp1y/pr95226.C  -std=c++26 (test for excess errors)
+FAIL: g++.dg/cpp1z/decomp12.C  -std=c++23 (test for excess errors)
+FAIL: g++.dg/cpp1z/decomp12.C  -std=c++26 (test for excess errors)
+FAIL: g++.dg/cpp1z/eval-order2.C  -std=c++20 (test for excess errors)
+FAIL: g++.dg/cpp1z/eval-order2.C  -std=c++23 (test for excess errors)
+FAIL: g++.dg/cpp1z/eval-order2.C  -std=c++26 (test for excess errors)
+FAIL: g++.dg/cpp2a/srcloc17.C  -std=c++20 (test for excess errors)
+FAIL: g++.dg/cpp2a/srcloc17.C  -std=c++23 (test for excess errors)
+FAIL: g++.dg/cpp2a/srcloc17.C  -std=c++26 (test for excess errors)
+FAIL: g++.old-deja/g++.jason/template31.C  -std=c++20 (test for excess errors)
+FAIL: g++.old-deja/g++.jason/template31.C  -std=c++23 (test for excess errors)
+FAIL: g++.old-deja/g++.jason/template31.C  -std=c++26 (test for excess errors)
+FAIL: 20_util/unique_ptr/creation/for_overwrite.cc  -std=gnu++26 (test for excess errors)
+FAIL: 23_containers/span/cons_1_assert_neg.cc  -std=gnu++20 (test for excess errors)
+FAIL: 23_containers/span/cons_1_assert_neg.cc  -std=gnu++26 (test for excess errors)
+FAIL: 23_containers/span/cons_2_assert_neg.cc  -std=gnu++20 (test for excess errors)
+FAIL: 23_containers/span/cons_2_assert_neg.cc  -std=gnu++26 (test for excess errors)
+FAIL: std/ranges/repeat/1.cc  -std=gnu++23 (test for excess errors)
+FAIL: std/ranges/repeat/1.cc  -std=gnu++26 (test for excess errors)

Errors are like:
func-params-08.C:(.text._ZNSt12_Vector_baseIiSaIiEEC2Ev[_ZNSt12_Vector_baseIiSaIiEEC5Ev]+0x14): undefined reference to `_ZNSt12_Vector_baseIiSaIiEE12_Vector_implC1EvQ26is_default_constructible_vIN9__gnu_cxx14__alloc_traitsIT0_NS5_10value_typeEE6rebindIT_E5otherEE'
Though, libstdc++.so.6 abilist is the same.
Trying to debug it now.

2024-04-24  Jakub Jelinek  <jakub@redhat.com>
	    Jason Merrill  <jason@redhat.com>

	PR lto/113208
	* decl2.cc (tentative_decl_linkage): Call maybe_make_one_only
	for implicit instantiations of maybe in charge ctors/dtors
	declared inline.
	* optimize.cc (can_alias_cdtor): Adjust condition, for
	HAVE_COMDAT_GROUP && DECL_ONE_ONLY && DECL_WEAK return true even
	if not DECL_INTERFACE_KNOWN.
	* decl.cc (cxx_comdat_group): For DECL_CLONED_FUNCTION_P
	functions if SUPPORTS_ONE_ONLY return DECL_COMDAT_GROUP if already
	set.

	* g++.dg/abi/comdat2.C: New test.
	* g++.dg/abi/comdat3.C: New test.
	* g++.dg/abi/comdat4.C: New test.
	* g++.dg/abi/comdat5.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.



	Jakub

Comments

Jakub Jelinek April 25, 2024, 2:22 p.m. UTC | #1 
On Thu, Apr 25, 2024 at 02:02:32PM +0200, Jakub Jelinek wrote:
> I've tried the following patch, but unfortunately that lead to large
> number of regressions:
> +FAIL: g++.dg/cpp0x/initlist25.C  -std=c++17 (test for excess errors)

So the reduced testcase for this is
template <typename T, typename U> struct A {
  T a1;
  U a2;
  template <typename V, typename W, bool = true>
  constexpr A(V &&x, W &&y) : a1(x), a2(y) {}
};
template <typename> struct B;
namespace std {
template <class> struct initializer_list {
  int *_M_array;
  decltype (sizeof 0) _M_len;
};
}
template <typename T, typename U> struct C {
  void foo (std::initializer_list<A<const T, U>>);
};
template <class> struct D;
template <typename T, typename = D<T>, typename = B<T>>
struct E { E (const char *); ~E (); };
int
main ()
{
  C<E<char>, E<char>> m;
  m.foo ({{"t", "t"}, {"y", "y"}});
}
Without the patch I've just posted or even with the earlier version
of the patch the
_ZN1AIK1EIc1DIcE1BIcEES5_EC[12]IRA2_KcSB_Lb1EEEOT_OT0_
ctors were emitted, but with this patch they are unresolved externals.

The reason is that the code actually uses (calls) the
_ZN1AIK1EIc1DIcE1BIcEES5_EC1IRA2_KcSB_Lb1EEEOT_OT0_
__ct_comp constructor, that one has TREE_USED, while the
_ZN1AIK1EIc1DIcE1BIcEES5_EC2IRA2_KcSB_Lb1EEEOT_OT0_
__ct_base constructor is not TREE_USED.

But the c_parse_final_cleanups loop over
FOR_EACH_VEC_SAFE_ELT (deferred_fns, i, decl)
will ignore the TREE_USED __ct_comp because it is an alias
and so has !DECL_SAVED_TREE:
5273		  if (!DECL_SAVED_TREE (decl))
5274		    continue;

With the following incremental patch the tests in make check-g++
(haven't tried the coroutine one) which failed with the earlier patch
now pass.

--- gcc/cp/decl2.cc.jj	2024-04-25 10:52:21.057535959 +0200
+++ gcc/cp/decl2.cc	2024-04-25 16:19:17.385547357 +0200
@@ -5271,7 +5271,19 @@ c_parse_final_cleanups (void)
 	    generate_tls_wrapper (decl);
 
 	  if (!DECL_SAVED_TREE (decl))
-	    continue;
+	    {
+	      cgraph_node *node;
+	      tree tgt;
+	      /* Even when maybe_clone_body created same body alias
+		 has no DECL_SAVED_TREE, if its alias target does,
+		 don't skip it.  */
+	      if (!DECL_CLONED_FUNCTION (decl)
+		  || !(node = cgraph_node::get (decl))
+		  || !node->cpp_implicit_alias
+		  || !(tgt = node->get_alias_target_tree ())
+		  || !DECL_SAVED_TREE (tgt))
+		continue;
+	    }
 
 	  cgraph_node *node = cgraph_node::get_create (decl);
 
@@ -5299,7 +5311,7 @@ c_parse_final_cleanups (void)
 		node = node->get_alias_target ();
 
 	      node->call_for_symbol_thunks_and_aliases (clear_decl_external,
-						      NULL, true);
+							NULL, true);
 	      /* If we mark !DECL_EXTERNAL one of the symbols in some comdat
 		 group, we need to mark all symbols in the same comdat group
 		 that way.  */
@@ -5309,7 +5321,7 @@ c_parse_final_cleanups (void)
 		     next != node;
 		     next = dyn_cast<cgraph_node *> (next->same_comdat_group))
 		  next->call_for_symbol_thunks_and_aliases (clear_decl_external,
-							  NULL, true);
+							    NULL, true);
 	    }
 
 	  /* If we're going to need to write this function out, and


	Jakub
Jason Merrill April 25, 2024, 3:30 p.m. UTC | #2 
On 4/25/24 07:22, Jakub Jelinek wrote:
> On Thu, Apr 25, 2024 at 02:02:32PM +0200, Jakub Jelinek wrote:
>> I've tried the following patch, but unfortunately that lead to large
>> number of regressions:
>> +FAIL: g++.dg/cpp0x/initlist25.C  -std=c++17 (test for excess errors)
> 
> So the reduced testcase for this is
> template <typename T, typename U> struct A {
>    T a1;
>    U a2;
>    template <typename V, typename W, bool = true>
>    constexpr A(V &&x, W &&y) : a1(x), a2(y) {}
> };
> template <typename> struct B;
> namespace std {
> template <class> struct initializer_list {
>    int *_M_array;
>    decltype (sizeof 0) _M_len;
> };
> }
> template <typename T, typename U> struct C {
>    void foo (std::initializer_list<A<const T, U>>);
> };
> template <class> struct D;
> template <typename T, typename = D<T>, typename = B<T>>
> struct E { E (const char *); ~E (); };
> int
> main ()
> {
>    C<E<char>, E<char>> m;
>    m.foo ({{"t", "t"}, {"y", "y"}});
> }
> Without the patch I've just posted or even with the earlier version
> of the patch the
> _ZN1AIK1EIc1DIcE1BIcEES5_EC[12]IRA2_KcSB_Lb1EEEOT_OT0_
> ctors were emitted, but with this patch they are unresolved externals.
> 
> The reason is that the code actually uses (calls) the
> _ZN1AIK1EIc1DIcE1BIcEES5_EC1IRA2_KcSB_Lb1EEEOT_OT0_
> __ct_comp constructor, that one has TREE_USED, while the
> _ZN1AIK1EIc1DIcE1BIcEES5_EC2IRA2_KcSB_Lb1EEEOT_OT0_
> __ct_base constructor is not TREE_USED.
> 
> But the c_parse_final_cleanups loop over
> FOR_EACH_VEC_SAFE_ELT (deferred_fns, i, decl)
> will ignore the TREE_USED __ct_comp because it is an alias
> and so has !DECL_SAVED_TREE:
> 5273		  if (!DECL_SAVED_TREE (decl))
> 5274		    continue;

Hmm, maybe maybe_clone_body shouldn't clear DECL_SAVED_TREE for aliases, 
but rather set it to some stub like void_node?

Though with all these changes, it's probably better to go with your 
first patch for GCC 14 and delay this approach to 15.  Your v1 patch is 
OK for 14.

Jason
diff mbox series

Patch

--- gcc/cp/decl2.cc.jj	2024-04-24 18:28:22.299513620 +0200
+++ gcc/cp/decl2.cc	2024-04-25 10:04:18.049476567 +0200
@@ -3312,16 +3312,23 @@  tentative_decl_linkage (tree decl)
 	     linkage of all functions, and as that causes writes to
 	     the data mapped in from the PCH file, it's advantageous
 	     to mark the functions at this point.  */
-	  if (DECL_DECLARED_INLINE_P (decl)
-	      && (!DECL_IMPLICIT_INSTANTIATION (decl)
-		  || DECL_DEFAULTED_FN (decl)))
+	  if (DECL_DECLARED_INLINE_P (decl))
 	    {
-	      /* This function must have external linkage, as
-		 otherwise DECL_INTERFACE_KNOWN would have been
-		 set.  */
-	      gcc_assert (TREE_PUBLIC (decl));
-	      comdat_linkage (decl);
-	      DECL_INTERFACE_KNOWN (decl) = 1;
+	      if (!DECL_IMPLICIT_INSTANTIATION (decl)
+		  || DECL_DEFAULTED_FN (decl))
+		{
+		  /* This function must have external linkage, as
+		     otherwise DECL_INTERFACE_KNOWN would have been
+		     set.  */
+		  gcc_assert (TREE_PUBLIC (decl));
+		  comdat_linkage (decl);
+		  DECL_INTERFACE_KNOWN (decl) = 1;
+		}
+	      else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl))
+		/* For implicit instantiations of cdtors try to make
+		   it comdat, so that maybe_clone_body can use aliases.
+		   See PR113208.  */
+		maybe_make_one_only (decl);
 	    }
 	}
       else if (VAR_P (decl))
--- gcc/cp/optimize.cc.jj	2024-04-25 09:47:24.936736833 +0200
+++ gcc/cp/optimize.cc	2024-04-25 10:19:25.338332564 +0200
@@ -220,10 +220,8 @@  can_alias_cdtor (tree fn)
   gcc_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (fn));
   /* Don't use aliases for weak/linkonce definitions unless we can put both
      symbols in the same COMDAT group.  */
-  return (DECL_INTERFACE_KNOWN (fn)
-	  && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fn))
-	  && (!DECL_ONE_ONLY (fn)
-	      || (HAVE_COMDAT_GROUP && DECL_WEAK (fn))));
+  return (DECL_WEAK (fn) ? (HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn))
+			 : (DECL_INTERFACE_KNOWN (fn) && !DECL_ONE_ONLY (fn)));
 }
 
 /* FN is a [cd]tor, fns is a pointer to an array of length 3.  Fill fns
--- gcc/cp/decl.cc.jj	2024-04-24 18:28:22.231514532 +0200
+++ gcc/cp/decl.cc	2024-04-25 09:47:06.855991224 +0200
@@ -19254,6 +19254,14 @@  cxx_comdat_group (tree decl)
 	  else
 	    break;
 	}
+      /* If a ctor/dtor has already set the comdat group by
+	 maybe_clone_body, don't override it.  */
+      if (SUPPORTS_ONE_ONLY
+	  && TREE_CODE (decl) == FUNCTION_DECL
+	  && DECL_CLONED_FUNCTION_P (decl)
+	  && SUPPORTS_ONE_ONLY)
+	if (tree comdat = DECL_COMDAT_GROUP (decl))
+	  return comdat;
     }
 
   return decl;
--- gcc/testsuite/g++.dg/abi/comdat2.C.jj	2024-04-25 09:47:06.855991224 +0200
+++ gcc/testsuite/g++.dg/abi/comdat2.C	2024-04-25 09:47:06.855991224 +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/abi/comdat3.C.jj	2024-04-25 09:47:06.855991224 +0200
+++ gcc/testsuite/g++.dg/abi/comdat3.C	2024-04-25 09:47:06.855991224 +0200
@@ -0,0 +1,22 @@ 
+// PR lto/113208
+// { dg-do compile { target { c++11 && { *-*-*gnu* } } } }
+// { dg-additional-options "-O2" }
+// { dg-final { scan-assembler "_ZN1M1SINS_1P1TELN1N1LE2EEC5Ev,comdat" } }
+// { dg-final { scan-assembler-not "_ZN1M1SINS_1P1TELN1N1LE2EEC1Ev,comdat" } }
+// { dg-final { scan-assembler-not "_ZN1M1SINS_1P1TELN1N1LE2EEC2Ev,comdat" } }
+
+namespace N { enum L { L1, L2, L3 } const O = L3; }
+namespace M {
+  using N::O;
+  using N::L;
+  template <typename, L = O>
+  struct S { constexpr S () {} };
+  namespace P {
+    struct T;
+    struct U { S<T> u; };
+    void foo () { U (); }
+  }
+  extern template class S<P::T>;
+}
+namespace p = M::P;
+template class M::S<p::T>;
--- gcc/testsuite/g++.dg/abi/comdat4.C.jj	2024-04-25 10:27:54.141172257 +0200
+++ gcc/testsuite/g++.dg/abi/comdat4.C	2024-04-25 10:28:22.493773334 +0200
@@ -0,0 +1,28 @@ 
+// PR lto/113208
+// { dg-do compile { target { c++11 && { *-*-*gnu* } } } }
+// { dg-additional-options "-O2" }
+// { 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);
+}
+
+template struct B<C>;
--- gcc/testsuite/g++.dg/abi/comdat5.C.jj	2024-04-25 10:28:35.842585513 +0200
+++ gcc/testsuite/g++.dg/abi/comdat5.C	2024-04-25 10:28:52.413352362 +0200
@@ -0,0 +1,28 @@ 
+// PR lto/113208
+// { dg-do compile { target { c++11 && { *-*-*gnu* } } } }
+// { dg-additional-options "-O2" }
+// { dg-final { scan-assembler-not "_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);
+}
+
+extern template struct B<C>;
--- gcc/testsuite/g++.dg/lto/pr113208_0.C.jj	2024-04-25 09:47:06.856991210 +0200
+++ gcc/testsuite/g++.dg/lto/pr113208_0.C	2024-04-25 09:47:06.855991224 +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); }
--- gcc/testsuite/g++.dg/lto/pr113208_1.C.jj	2024-04-25 09:47:06.856991210 +0200
+++ gcc/testsuite/g++.dg/lto/pr113208_1.C	2024-04-25 09:47:06.856991210 +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.h.jj	2024-04-25 09:47:06.856991210 +0200
+++ gcc/testsuite/g++.dg/lto/pr113208.h	2024-04-25 09:47:06.856991210 +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) {}
+};