[C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 3)

[C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 3)

[Jakub Jelinek]

Hi!

Here is the latest version of the cdtor optimization patch.
The comdat group name is *[CD]5* and if the dtor is virtual, deleting dtor
is emitted into that comdat group as well.
libstdc++ symbol version script has been adjusted, so that it exports
exactly what it used to export before this patch.
On x86_64-linux this patch saves 21KB of libstdc++.so's .text.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2009-11-30  Jakub Jelinek  <jakub@redhat.com>

	PR c++/3187
	* optimize.c (cdtor_comdat_group): New function.
	(maybe_clone_body): Also optimize DECL_COMDAT base/complete cdtors
	and in that case put also the deleting dtor in the same comdat group
	as base and complete dtor if dtor is virtual.

	* config/abi/pre/gnu.ver: Don't export certain base dtors that
	weren't previously exported.

--- gcc/cp/optimize.c.jj	2009-11-30 15:53:13.000000000 +0100
+++ gcc/cp/optimize.c	2009-11-30 19:18:51.000000000 +0100
@@ -142,6 +142,46 @@ build_delete_destructor_body (tree delet
     }
 }
 
+/* Return name of comdat group for complete and base ctor (or dtor)
+   that have the same body.  If dtor is virtual, deleting dtor goes
+   into this comdat group as well.  */
+
+static tree
+cdtor_comdat_group (tree complete, tree base)
+{
+  tree complete_name = DECL_COMDAT_GROUP (complete);
+  tree base_name = DECL_COMDAT_GROUP (base);
+  char *grp_name;
+  const char *p, *q;
+  bool diff_seen = false;
+  size_t idx;
+  if (complete_name == NULL)
+    complete_name = cxx_comdat_group (complete);
+  if (base_name == NULL)
+    base_name = cxx_comdat_group (base);
+  gcc_assert (IDENTIFIER_LENGTH (complete_name)
+	      == IDENTIFIER_LENGTH (base_name));
+  grp_name = XALLOCAVEC (char, IDENTIFIER_LENGTH (complete_name) + 1);
+  p = IDENTIFIER_POINTER (complete_name);
+  q = IDENTIFIER_POINTER (base_name);
+  for (idx = 0; idx < IDENTIFIER_LENGTH (complete_name); idx++)
+    if (p[idx] == q[idx])
+      grp_name[idx] = p[idx];
+    else
+      {
+	gcc_assert (!diff_seen
+		    && idx > 0
+		    && (p[idx - 1] == 'C' || p[idx - 1] == 'D')
+		    && p[idx] == '1'
+		    && q[idx] == '2');
+	grp_name[idx] = '5';
+	diff_seen = true;
+      }
+  grp_name[idx] = '\0';
+  gcc_assert (diff_seen);
+  return get_identifier (grp_name);
+}
+
 /* FN is a function that has a complete body.  Clone the body as
    necessary.  Returns nonzero if there's no longer any need to
    process the main body.  */
@@ -149,6 +189,7 @@ build_delete_destructor_body (tree delet
 bool
 maybe_clone_body (tree fn)
 {
+  tree comdat_group = NULL_TREE;
   tree clone;
   tree fns[3];
   bool first = true;
@@ -248,12 +289,28 @@ maybe_clone_body (tree fn)
 	  && idx == 1
 	  && !flag_use_repository
 	  && DECL_INTERFACE_KNOWN (fns[0])
-	  && !DECL_ONE_ONLY (fns[0])
+	  && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fns[0]))
+	  && (!DECL_ONE_ONLY (fns[0])
+	      || (HAVE_COMDAT_GROUP && DECL_WEAK (fns[0])))
 	  && cgraph_same_body_alias (clone, fns[0]))
 	{
 	  alias = true;
+	  if (DECL_ONE_ONLY (fns[0]))
+	    {
+	      /* For comdat base and complete cdtors put them
+		 into the same, *[CD]5* comdat group instead of
+		 *[CD][12]*.  */
+	      comdat_group = cdtor_comdat_group (fns[1], fns[0]);
+	      DECL_COMDAT_GROUP (fns[0]) = comdat_group;
+	      DECL_COMDAT_GROUP (fns[1]) = comdat_group;
+	    }
 	  emit_associated_thunks (clone);
 	}
+      else if (idx == 2 && comdat_group)
+	/* If *[CD][12]* dtors go into the *[CD]5* comdat group
+	   and dtor is virtual, it goes into the same comdat group
+	   as well.  */
+	DECL_COMDAT_GROUP (fns[2]) = comdat_group;
 
       /* Build the delete destructor by calling complete destructor
          and delete function.  */
--- libstdc++-v3/config/abi/pre/gnu.ver.jj	2009-11-12 14:14:03.000000000 +0100
+++ libstdc++-v3/config/abi/pre/gnu.ver	2009-11-30 20:47:34.000000000 +0100
@@ -72,13 +72,18 @@ GLIBCXX_3.4 {
       std::c[v-z]*;
 #     std::[d-g]*;
       std::d[a-d]*;
-      std::d[f-z]*;
+      std::d[f-n]*;
+      std::domain_error::d*;
+#     std::domain_error::~d*;
+      std::d[p-z]*;
       std::e[a-q]*;
       std::error[^_]*;
       std::e[s-z]*;
       std::gslice*;
       std::h[^a]*;
-      std::i[a-n]*;
+      std::i[a-m]*;
+      std::invalid_argument::i*;
+#     std::invalid_argument::~i*;
 #     std::ios_base::[A-Ha-z]*;
       std::ios_base::[A-Ha-f]*;
       std::ios_base::goodbit;
@@ -94,7 +99,8 @@ GLIBCXX_3.4 {
       std::istrstream*;
       std::i[t-z]*;
       std::[A-Zj-k]*;
-      std::length_error*;
+      std::length_error::l*;
+#     std::length_error::~l*;
       std::logic_error*;
       std::locale::[A-Za-e]*;
       std::locale::facet::[A-Za-z]*;
@@ -122,10 +128,14 @@ GLIBCXX_3.4 {
       std::nu[^m]*;
       std::num[^e]*;
       std::ostrstream*;
-      std::out_of_range*;
-      std::overflow_error*;
+      std::out_of_range::o*;
+#     std::out_of_range::~o*;
+      std::overflow_error::o*;
+#     std::overflow_error::~o*;
 #     std::[p-q]*;
-      std::r[^e]*;
+      std::r[^ae]*;
+      std::range_error::r*;
+#     std::range_error::~r*;
       std::re[^t]*;
 #     std::rethrow_exception
       std::set_new_handler*;
@@ -143,7 +153,8 @@ GLIBCXX_3.4 {
       std::tr1::h[^a]*;
       std::t[s-z]*;
 #     std::[A-Zu-z]*;
-      std::underflow_error*;
+      std::underflow_error::u*;
+#     std::underflow_error::~u*;
       std::uncaught_exception*;
       std::unexpected*;
       std::[A-Zv-z]*;
@@ -284,7 +295,8 @@ GLIBCXX_3.4 {
     _ZNSt15basic_streambufI[cw]St11char_traitsI[cw]EEaSERKS2_;
 
     # std::basic_stringbuf
-    _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EE[CD]*;
+    _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EEC*;
+    _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EED[^2]*;
     _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EE[0-9][a-r]*;
     _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EE[0-9]seek*;
     _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EE[0-9]set*;
@@ -639,6 +651,16 @@ GLIBCXX_3.4 {
     _ZGVNSt[^1]*;
     _ZGVNSt1[^7]*;
 
+    # complete and deleting destructors where base destructors should not
+    # be exported.
+    _ZNSt11range_errorD[01]Ev;
+    _ZNSt12domain_errorD[01]Ev;
+    _ZNSt12length_errorD[01]Ev;
+    _ZNSt12out_of_rangeD[01]Ev;
+    _ZNSt14overflow_errorD[01]Ev;
+    _ZNSt15underflow_errorD[01]Ev;
+    _ZNSt16invalid_argumentD[01]Ev;
+
     # virtual function thunks
     _ZThn8_NS*;
     _ZThn16_NS*;
@@ -891,7 +913,8 @@ GLIBCXX_3.4.10 {
     _ZNSt15basic_streambufI[cw]St11char_traitsI[cw]EE6stosscEv;
 
     _ZN9__gnu_cxx18stdio_sync_filebufI[cw]St11char_traitsI[cw]EE4syncEv;
-    _ZN9__gnu_cxx18stdio_sync_filebufI[cw]St11char_traitsI[cw]EE[5-9CD]*;
+    _ZN9__gnu_cxx18stdio_sync_filebufI[cw]St11char_traitsI[cw]EE[5-9C]*;
+    _ZN9__gnu_cxx18stdio_sync_filebufI[cw]St11char_traitsI[cw]EED[^2]*;
 
 } GLIBCXX_3.4.9;
 

	Jakub

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 3)

[Jason Merrill]

On 11/30/2009 05:36 PM, Jakub Jelinek wrote:
> 	* config/abi/pre/gnu.ver: Don't export certain base dtors that
> 	weren't previously exported.

Is this necessary?  I thought it was OK to add new exported symbols, but 
you'd know better than I.

> +	  && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fns[0]))

This test seems unnecessary; I don't see the problem with using aliases 
on targets with weak symbols but no one-only support.

Jason

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 3)

[Jakub Jelinek]

On Mon, Nov 30, 2009 at 10:46:31PM -0500, Jason Merrill wrote:
> On 11/30/2009 05:36 PM, Jakub Jelinek wrote:
> >	* config/abi/pre/gnu.ver: Don't export certain base dtors that
> >	weren't previously exported.
> 
> Is this necessary?  I thought it was OK to add new exported symbols, but 
> you'd know better than I.

Yes.  We definitely must not export them with the old symbol versions where
they have not been exported before (GLIBCXX_3.4 and GLIBCXX_3.4.10).
We could export them at @@GLIBCXX_3.4.14 (which would mean the same gnu.ver
changes and mentioning the D2 symbols in GLIBCXX_3.4.14 section), but that
has a problem that we only emit those D2 symbols when HAVE_COMDAT_GROUP, so
in !HAVE_COMDAT_GROUP configurations the exported list would be different.
I have no idea how to force generation of those symbols otherwise so that
they are exported on all targets.

> 
> >+	  && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fns[0]))
> 
> This test seems unnecessary; I don't see the problem with using aliases 
> on targets with weak symbols but no one-only support.

This is to fix AIX (and is actually unrelated to this patch, it can be dealt
with separately).  In theory it should work well, but David Edelsohn
reported that some symbols were missing from libstdc++.so without it.  I
have no access to AIX and have no idea what AIX linker is doing with the
symbols.  !SUPPORTS_ONE_ONLY targets don't have DECL_ONE_ONLY, but instead
just use DECL_WEAK symbols, so I thought when we don't do this optimization
on !HAVE_COMDAT_GROUP SUPPORTS_ONE_ONLY targets for comdat and comdat-like
symbols, the world wouldn't end if we didn't support it even on the
far rarer !HAVE_COMDAT_GROUP !SUPPORTS_ONE_ONLY targets.

	Jakub

[C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 4)

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

Hi!

On Mon, Nov 30, 2009 at 05:36:53PM -0500, Jakub Jelinek wrote:
> 
> Here is the latest version of the cdtor optimization patch.
> The comdat group name is *[CD]5* and if the dtor is virtual, deleting dtor
> is emitted into that comdat group as well.
> libstdc++ symbol version script has been adjusted, so that it exports
> exactly what it used to export before this patch.
> On x86_64-linux this patch saves 21KB of libstdc++.so's .text.

Apparently the previous patch didn't actually put deleting dtors in the D5
comdat group, because expand_or_defer_fn changed again its DECL_COMDAT_GROUP
to the D0 one.  The first patch fixes that by setting DECL_COMDAT_GROUP of
the alias (which doesn't matter much) and of the deleting dtor after that
call.

Unfortunately, when bootstrapping with extra checking patch (second patch)
where I wanted to verify that for classes with comdat virtual dtors we
always emit either both D2 (and D1 alias to it) and D0 dtor, or none of
them, build in libstdc++ revealed that we emit e.g. in complex_io.cc
(reduced testcase attached last, cc1plus -O2 cplx.ii) only the
_ZN1DIi1BIiE1CIiEED2Ev (and _ZN1DIi1BIiE1CIiEED1Ev alias) dtor in
the _ZN1DIi1BIiE1CIiEED5Ev comdat group, but not _ZN1DIi1BIiE1CIiEED0Ev.
The class is extern template class, so either the g++ is buggy and emits
something it never should, because the template is extern, or
putting the *D0* dtors also in the *D5* comdat group is not a good idea.

	Jakub

[-- Attachment #2: Y477 --]
[-- Type: text/plain, Size: 6526 bytes --]

2009-12-01  Jakub Jelinek  <jakub@redhat.com>

	PR c++/3187
	* optimize.c (cdtor_comdat_group): New function.
	(maybe_clone_body): Also optimize DECL_COMDAT base/complete cdtors
	and in that case put also the deleting dtor in the same comdat group
	as base and complete dtor if dtor is virtual.

	* config/abi/pre/gnu.ver: Don't export certain base dtors that
	weren't previously exported.

--- gcc/cp/optimize.c.jj	2009-11-30 15:53:13.000000000 +0100
+++ gcc/cp/optimize.c	2009-11-30 19:18:51.000000000 +0100
@@ -142,6 +142,46 @@ build_delete_destructor_body (tree delet
     }
 }
 
+/* Return name of comdat group for complete and base ctor (or dtor)
+   that have the same body.  If dtor is virtual, deleting dtor goes
+   into this comdat group as well.  */
+
+static tree
+cdtor_comdat_group (tree complete, tree base)
+{
+  tree complete_name = DECL_COMDAT_GROUP (complete);
+  tree base_name = DECL_COMDAT_GROUP (base);
+  char *grp_name;
+  const char *p, *q;
+  bool diff_seen = false;
+  size_t idx;
+  if (complete_name == NULL)
+    complete_name = cxx_comdat_group (complete);
+  if (base_name == NULL)
+    base_name = cxx_comdat_group (base);
+  gcc_assert (IDENTIFIER_LENGTH (complete_name)
+	      == IDENTIFIER_LENGTH (base_name));
+  grp_name = XALLOCAVEC (char, IDENTIFIER_LENGTH (complete_name) + 1);
+  p = IDENTIFIER_POINTER (complete_name);
+  q = IDENTIFIER_POINTER (base_name);
+  for (idx = 0; idx < IDENTIFIER_LENGTH (complete_name); idx++)
+    if (p[idx] == q[idx])
+      grp_name[idx] = p[idx];
+    else
+      {
+	gcc_assert (!diff_seen
+		    && idx > 0
+		    && (p[idx - 1] == 'C' || p[idx - 1] == 'D')
+		    && p[idx] == '1'
+		    && q[idx] == '2');
+	grp_name[idx] = '5';
+	diff_seen = true;
+      }
+  grp_name[idx] = '\0';
+  gcc_assert (diff_seen);
+  return get_identifier (grp_name);
+}
+
 /* FN is a function that has a complete body.  Clone the body as
    necessary.  Returns nonzero if there's no longer any need to
    process the main body.  */
@@ -149,6 +189,7 @@ build_delete_destructor_body (tree delet
 bool
 maybe_clone_body (tree fn)
 {
+  tree comdat_group = NULL_TREE;
   tree clone;
   tree fns[3];
   bool first = true;
@@ -248,10 +289,20 @@ maybe_clone_body (tree fn)
 	  && idx == 1
 	  && !flag_use_repository
 	  && DECL_INTERFACE_KNOWN (fns[0])
-	  && !DECL_ONE_ONLY (fns[0])
+	  && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fns[0]))
+	  && (!DECL_ONE_ONLY (fns[0])
+	      || (HAVE_COMDAT_GROUP && DECL_WEAK (fns[0])))
 	  && cgraph_same_body_alias (clone, fns[0]))
 	{
 	  alias = true;
+	  if (DECL_ONE_ONLY (fns[0]))
+	    {
+	      /* For comdat base and complete cdtors put them
+		 into the same, *[CD]5* comdat group instead of
+		 *[CD][12]*.  */
+	      comdat_group = cdtor_comdat_group (fns[1], fns[0]);
+	      DECL_COMDAT_GROUP (fns[0]) = comdat_group;
+	    }
 	  emit_associated_thunks (clone);
 	}
 
@@ -333,6 +384,15 @@ maybe_clone_body (tree fn)
     }
   pop_from_top_level ();
 
+  if (comdat_group)
+    {
+      DECL_COMDAT_GROUP (fns[1]) = comdat_group;
+      if (fns[2])
+	/* If *[CD][12]* dtors go into the *[CD]5* comdat group and dtor is
+	   virtual, it goes into the same comdat group as well.  */
+	DECL_COMDAT_GROUP (fns[2]) = comdat_group;
+    }
+
   /* We don't need to process the original function any further.  */
   return 1;
 }
--- libstdc++-v3/config/abi/pre/gnu.ver.jj	2009-11-12 14:14:03.000000000 +0100
+++ libstdc++-v3/config/abi/pre/gnu.ver	2009-11-30 20:47:34.000000000 +0100
@@ -72,13 +72,18 @@ GLIBCXX_3.4 {
       std::c[v-z]*;
 #     std::[d-g]*;
       std::d[a-d]*;
-      std::d[f-z]*;
+      std::d[f-n]*;
+      std::domain_error::d*;
+#     std::domain_error::~d*;
+      std::d[p-z]*;
       std::e[a-q]*;
       std::error[^_]*;
       std::e[s-z]*;
       std::gslice*;
       std::h[^a]*;
-      std::i[a-n]*;
+      std::i[a-m]*;
+      std::invalid_argument::i*;
+#     std::invalid_argument::~i*;
 #     std::ios_base::[A-Ha-z]*;
       std::ios_base::[A-Ha-f]*;
       std::ios_base::goodbit;
@@ -94,7 +99,8 @@ GLIBCXX_3.4 {
       std::istrstream*;
       std::i[t-z]*;
       std::[A-Zj-k]*;
-      std::length_error*;
+      std::length_error::l*;
+#     std::length_error::~l*;
       std::logic_error*;
       std::locale::[A-Za-e]*;
       std::locale::facet::[A-Za-z]*;
@@ -122,10 +128,14 @@ GLIBCXX_3.4 {
       std::nu[^m]*;
       std::num[^e]*;
       std::ostrstream*;
-      std::out_of_range*;
-      std::overflow_error*;
+      std::out_of_range::o*;
+#     std::out_of_range::~o*;
+      std::overflow_error::o*;
+#     std::overflow_error::~o*;
 #     std::[p-q]*;
-      std::r[^e]*;
+      std::r[^ae]*;
+      std::range_error::r*;
+#     std::range_error::~r*;
       std::re[^t]*;
 #     std::rethrow_exception
       std::set_new_handler*;
@@ -143,7 +153,8 @@ GLIBCXX_3.4 {
       std::tr1::h[^a]*;
       std::t[s-z]*;
 #     std::[A-Zu-z]*;
-      std::underflow_error*;
+      std::underflow_error::u*;
+#     std::underflow_error::~u*;
       std::uncaught_exception*;
       std::unexpected*;
       std::[A-Zv-z]*;
@@ -284,7 +295,8 @@ GLIBCXX_3.4 {
     _ZNSt15basic_streambufI[cw]St11char_traitsI[cw]EEaSERKS2_;
 
     # std::basic_stringbuf
-    _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EE[CD]*;
+    _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EEC*;
+    _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EED[^2]*;
     _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EE[0-9][a-r]*;
     _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EE[0-9]seek*;
     _ZNSt15basic_stringbufI[cw]St11char_traitsI[cw]ESaI[cw]EE[0-9]set*;
@@ -639,6 +651,16 @@ GLIBCXX_3.4 {
     _ZGVNSt[^1]*;
     _ZGVNSt1[^7]*;
 
+    # complete and deleting destructors where base destructors should not
+    # be exported.
+    _ZNSt11range_errorD[01]Ev;
+    _ZNSt12domain_errorD[01]Ev;
+    _ZNSt12length_errorD[01]Ev;
+    _ZNSt12out_of_rangeD[01]Ev;
+    _ZNSt14overflow_errorD[01]Ev;
+    _ZNSt15underflow_errorD[01]Ev;
+    _ZNSt16invalid_argumentD[01]Ev;
+
     # virtual function thunks
     _ZThn8_NS*;
     _ZThn16_NS*;
@@ -891,7 +913,8 @@ GLIBCXX_3.4.10 {
     _ZNSt15basic_streambufI[cw]St11char_traitsI[cw]EE6stosscEv;
 
     _ZN9__gnu_cxx18stdio_sync_filebufI[cw]St11char_traitsI[cw]EE4syncEv;
-    _ZN9__gnu_cxx18stdio_sync_filebufI[cw]St11char_traitsI[cw]EE[5-9CD]*;
+    _ZN9__gnu_cxx18stdio_sync_filebufI[cw]St11char_traitsI[cw]EE[5-9C]*;
+    _ZN9__gnu_cxx18stdio_sync_filebufI[cw]St11char_traitsI[cw]EED[^2]*;
 
 } GLIBCXX_3.4.9;
 

[-- Attachment #3: Y478 --]
[-- Type: text/plain, Size: 1784 bytes --]

--- gcc/cp/decl2.c.jj	2009-11-18 15:48:50.000000000 +0100
+++ gcc/cp/decl2.c	2009-12-01 08:33:36.000000000 +0100
@@ -3449,6 +3449,16 @@ no_linkage_error (tree decl)
 	       "is used but never defined", decl, t);
 }
 
+#ifdef ENABLE_CHECKING
+static GTY(()) tree vdtor_pairs;
+void add_vdtor_pair (tree, tree);
+void
+add_vdtor_pair (tree base_dtor, tree deleting_dtor)
+{
+  vdtor_pairs = tree_cons (base_dtor, deleting_dtor, vdtor_pairs);
+}
+#endif
+
 /* This routine is called at the end of compilation.
    Its job is to create all the code needed to initialize and
    destroy the global aggregates.  We do the destruction
@@ -3823,6 +3833,9 @@ cp_write_global_declarations (void)
 
 #ifdef ENABLE_CHECKING
   validate_conversion_obstack ();
+
+  for (; vdtor_pairs; vdtor_pairs = TREE_CHAIN (vdtor_pairs))
+    gcc_assert (!(TREE_ASM_WRITTEN (TREE_VALUE (vdtor_pairs)) ^ TREE_ASM_WRITTEN (TREE_PURPOSE (vdtor_pairs))));
 #endif /* ENABLE_CHECKING */
 }
 
--- gcc/cp/optimize.c.jj	2009-12-01 10:32:13.000000000 +0100
+++ gcc/cp/optimize.c	2009-12-01 00:02:09.000000000 +0100
@@ -388,9 +388,15 @@ maybe_clone_body (tree fn)
     {
       DECL_COMDAT_GROUP (fns[1]) = comdat_group;
       if (fns[2])
-	/* If *[CD][12]* dtors go into the *[CD]5* comdat group and dtor is
-	   virtual, it goes into the same comdat group as well.  */
-	DECL_COMDAT_GROUP (fns[2]) = comdat_group;
+	{
+#ifdef ENABLE_CHECKING
+extern void add_vdtor_pair (tree base_dtor, tree deleting_dtor);
+add_vdtor_pair (fns[0], fns[2]);
+#endif
+	  /* If *[CD][12]* dtors go into the *[CD]5* comdat group and dtor is
+	     virtual, it goes into the same comdat group as well.  */
+	  DECL_COMDAT_GROUP (fns[2]) = comdat_group;
+	}
     }
 
   /* We don't need to process the original function any further.  */

[-- Attachment #4: cplx.ii --]
[-- Type: text/plain, Size: 1163 bytes --]

template <typename> class C;
template <class> struct B;
template <typename A, typename = B <A>, typename = C <A> > class D;
template <typename A, typename = B <A>, typename = C <A> > class E;
template <typename _Tp> class C { };
template <typename A, typename F, typename G> struct H
{
  struct I
  {
    void fn1 (G __a) { fn2 (__a); }
    void fn2 (G) throw ();
  };
  I *fn3 () {}
  H ();
  ~H () { fn3 ()->fn1 (fn4 ()); }
  G fn4 () {}
};
struct J
{
  static int j1;
  static int j2;
};
template <typename A, typename> struct K
{
  virtual ~ K () {}
};
template <typename A, typename F> struct L
{
  void fn5 (K <A, F> *);
};
template <typename A, typename F, typename G> struct D : public K <A, F>
{
  H <A, F, G> d;
  D (int = J::j1) : K <A, F> (), d () {}
};
template <typename A, typename F, typename G> struct E : public L <A, F>
{
  D <A, F, G> e;
  E (int = J::j2) : L <A, F> (), e () { fn5 (&e); }
};
extern template class D <int>;
template <typename> class M;
template <typename _Tp, typename A, class F>
L <A, F> &operator<<(L <A, F> &, const M <_Tp> &)
{
  E <A, F> e;
}
template L <int, B <int> >
&operator<<(L <int, B <int> >&, const M <long>&);

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 3)

[Jason Merrill]

On 12/01/2009 02:45 AM, Jakub Jelinek wrote:
> On Mon, Nov 30, 2009 at 10:46:31PM -0500, Jason Merrill wrote:
>> On 11/30/2009 05:36 PM, Jakub Jelinek wrote:
>>> 	* config/abi/pre/gnu.ver: Don't export certain base dtors that
>>> 	weren't previously exported.
>>
>> Is this necessary?
>
> Yes.

OK.

>>> +	&&  (SUPPORTS_ONE_ONLY || !DECL_WEAK (fns[0]))
>>
>> This test seems unnecessary; I don't see the problem with using aliases
>> on targets with weak symbols but no one-only support.
>
> This is to fix AIX (and is actually unrelated to this patch, it can be dealt
> with separately).  In theory it should work well, but David Edelsohn
> reported that some symbols were missing from libstdc++.so without it.  I
> have no access to AIX and have no idea what AIX linker is doing with the
> symbols.

OK.

The patch is OK.

Jason

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 4)

[Jakub Jelinek]

On Tue, Dec 01, 2009 at 04:46:52AM -0500, Jakub Jelinek wrote:
> Unfortunately, when bootstrapping with extra checking patch (second patch)
> where I wanted to verify that for classes with comdat virtual dtors we
> always emit either both D2 (and D1 alias to it) and D0 dtor, or none of
> them, build in libstdc++ revealed that we emit e.g. in complex_io.cc
> (reduced testcase attached last, cc1plus -O2 cplx.ii) only the
> _ZN1DIi1BIiE1CIiEED2Ev (and _ZN1DIi1BIiE1CIiEED1Ev alias) dtor in
> the _ZN1DIi1BIiE1CIiEED5Ev comdat group, but not _ZN1DIi1BIiE1CIiEED0Ev.
> The class is extern template class, so either the g++ is buggy and emits
> something it never should, because the template is extern, or
> putting the *D0* dtors also in the *D5* comdat group is not a good idea.

Here is the additional patch that allowed the 2 patches to bootstrap.
The problem is with implicitly defined methods in extern template class XXX<NNN>
instantiations.  The synthetized methods don't have
DECL_TEMPLATE_INSTANTIATION set, thus don't go through
instantiate_class_member and thus are emitted whenever something needs them
(and the side with explicit non-extern instantiation doesn't necessarily
define all the functions).  In this light the patch below is an ABI change
(not sure what other compilers do though), as with the patch newly compiled
code with extern template class XXX<NNN> would no longer emit routines
it needs it, instead rely on the CU with
extern template class XXX<NNN>; template class XXX<NNN>;
to emit them.  But as the code emitted by older G++ doesn't guarantee all
the implicitly declared methods are emitted there, mixing new code and old
code could result in undefined references.  The methods are usually inlined,
sure, but e.g. if ipa-inline.c considers them called in cold blocks, it
decides not to inline them.

--- gcc/cp/pt.c.jj	2009-12-01 12:09:10.000000000 +0100
+++ gcc/cp/pt.c	2009-12-01 13:33:39.000000000 +0100
@@ -16055,6 +16055,31 @@ do_type_instantiation (tree t, tree stor
 	if (TREE_CODE (tmp) == FUNCTION_DECL
 	    && DECL_TEMPLATE_INSTANTIATION (tmp))
 	  instantiate_class_member (tmp, extern_p);
+	else if (TREE_CODE (tmp) == FUNCTION_DECL
+		 && DECL_ARTIFICIAL (tmp)
+		 && (DECL_CONSTRUCTOR_P (tmp)
+		     || DECL_DESTRUCTOR_P (tmp)
+		     || DECL_ASSIGNMENT_OPERATOR_P (tmp)))
+	  {
+	    /* Ensure the implicitly declared ctors/dtors/assignment operators
+	       will be either inlined, or extern calls for extern_p,
+	       or defined in the current TU otherwise.  */
+	    DECL_EXTERNAL (tmp) = 1;
+	    if (extern_p)
+	      DECL_NOT_REALLY_EXTERN (tmp) = 0;
+	    else
+	      {
+		mark_definable (tmp);
+		/* Always make artificials weak.  */
+		if (flag_weak)
+		  comdat_linkage (tmp);
+		/* For WIN32 we also want to put explicit instantiations in
+		   linkonce sections.  */
+		else if (TREE_PUBLIC (tmp))
+		  maybe_make_one_only (tmp);
+	      }
+	    DECL_INTERFACE_KNOWN (tmp) = 1;
+	  }
 
     for (tmp = TYPE_FIELDS (t); tmp; tmp = TREE_CHAIN (tmp))
       if (TREE_CODE (tmp) == VAR_DECL && DECL_TEMPLATE_INSTANTIATION (tmp))


	Jakub

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 4)

[Dave Korn]

Jakub Jelinek wrote:

> +		mark_definable (tmp);
> +		/* Always make artificials weak.  */
> +		if (flag_weak)
> +		  comdat_linkage (tmp);
> +		/* For WIN32 we also want to put explicit instantiations in
> +		   linkonce sections.  */
> +		else if (TREE_PUBLIC (tmp))
> +		  maybe_make_one_only (tmp);
> +	      }
> +	    DECL_INTERFACE_KNOWN (tmp) = 1;
> +	  }

  Hi Jakub, I'd like to test this on cygwin for you, as I sometimes get a bit
nervous about how robust the win32 backend declspec handling code necessarily
is w.r.t. changes in the handling of DECL_EXTERNAL et. al. (there was in fact
some transient breakage recently that fortunately got fixed on head before I
had to track it down).  So I have just a couple of questions:

- Can you confirm the exact combination of patches I should test?  I think
it's the Y477 and Y478 patches in the first "take 4" post followed by the
patch above from the reply to that post, yes?

- Do you happen to know off the top of your head of an area of the g++ or
libstdc testsuite that would be worth running first because you would suspect
that's where problems might show up?  A full run takes over a day (groan,
can't parallelise it owing to some kind of tasking/threading bug extant in the
cygwin dll at the moment) so it'd be handy if there was a smaller subset worth
running as a quick first check.

    cheers,
      DaveK

[C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 5)

[Jakub Jelinek]

Hi!

After discussions on IRC, here is the latest version of the patch.
For now the problematic implicitly defined virtual comdat destructors aren't
optimized, even when the base and complete dtor have the same body, as the
deleting dtor isn't necessarily emitted iff the base/complete dtors are
emitted.

The nice effect of that patch is that the libstdc++ symbol version script
changes aren't needed.

Bootstrapped/regtested on x86_64-linux (with the checking patch on top of
it), ok for trunk?

2009-12-01  Jakub Jelinek  <jakub@redhat.com>

	PR c++/3187
	* optimize.c (cdtor_comdat_group): New function.
	(maybe_clone_body): Also optimize DECL_COMDAT base/complete cdtors
	and in that case put also the deleting dtor in the same comdat group
	as base and complete dtor if dtor is virtual.

--- gcc/cp/optimize.c.jj	2009-11-30 15:53:13.000000000 +0100
+++ gcc/cp/optimize.c	2009-11-30 19:18:51.000000000 +0100
@@ -142,6 +142,46 @@ build_delete_destructor_body (tree delet
     }
 }
 
+/* Return name of comdat group for complete and base ctor (or dtor)
+   that have the same body.  If dtor is virtual, deleting dtor goes
+   into this comdat group as well.  */
+
+static tree
+cdtor_comdat_group (tree complete, tree base)
+{
+  tree complete_name = DECL_COMDAT_GROUP (complete);
+  tree base_name = DECL_COMDAT_GROUP (base);
+  char *grp_name;
+  const char *p, *q;
+  bool diff_seen = false;
+  size_t idx;
+  if (complete_name == NULL)
+    complete_name = cxx_comdat_group (complete);
+  if (base_name == NULL)
+    base_name = cxx_comdat_group (base);
+  gcc_assert (IDENTIFIER_LENGTH (complete_name)
+	      == IDENTIFIER_LENGTH (base_name));
+  grp_name = XALLOCAVEC (char, IDENTIFIER_LENGTH (complete_name) + 1);
+  p = IDENTIFIER_POINTER (complete_name);
+  q = IDENTIFIER_POINTER (base_name);
+  for (idx = 0; idx < IDENTIFIER_LENGTH (complete_name); idx++)
+    if (p[idx] == q[idx])
+      grp_name[idx] = p[idx];
+    else
+      {
+	gcc_assert (!diff_seen
+		    && idx > 0
+		    && (p[idx - 1] == 'C' || p[idx - 1] == 'D')
+		    && p[idx] == '1'
+		    && q[idx] == '2');
+	grp_name[idx] = '5';
+	diff_seen = true;
+      }
+  grp_name[idx] = '\0';
+  gcc_assert (diff_seen);
+  return get_identifier (grp_name);
+}
+
 /* FN is a function that has a complete body.  Clone the body as
    necessary.  Returns nonzero if there's no longer any need to
    process the main body.  */
@@ -149,6 +189,7 @@ build_delete_destructor_body (tree delet
 bool
 maybe_clone_body (tree fn)
 {
+  tree comdat_group = NULL_TREE;
   tree clone;
   tree fns[3];
   bool first = true;
@@ -248,10 +289,26 @@ maybe_clone_body (tree fn)
 	  && idx == 1
 	  && !flag_use_repository
 	  && DECL_INTERFACE_KNOWN (fns[0])
-	  && !DECL_ONE_ONLY (fns[0])
+	  && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fns[0]))
+	  && (!DECL_ONE_ONLY (fns[0])
+	      || (HAVE_COMDAT_GROUP
+		  && DECL_WEAK (fns[0])
+		  /* Don't optimize synthetized virtual dtors, because then
+		     the deleting and other dtors are emitted when needed
+		     and so it is not certain we would emit both
+		     deleting and complete/base dtors in the comdat group.  */
+		  && (fns[2] == NULL || !DECL_ARTIFICIAL (fn))))
 	  && cgraph_same_body_alias (clone, fns[0]))
 	{
 	  alias = true;
+	  if (DECL_ONE_ONLY (fns[0]))
+	    {
+	      /* For comdat base and complete cdtors put them
+		 into the same, *[CD]5* comdat group instead of
+		 *[CD][12]*.  */
+	      comdat_group = cdtor_comdat_group (fns[1], fns[0]);
+	      DECL_COMDAT_GROUP (fns[0]) = comdat_group;
+	    }
 	  emit_associated_thunks (clone);
 	}
 
@@ -333,6 +390,15 @@ maybe_clone_body (tree fn)
     }
   pop_from_top_level ();
 
+  if (comdat_group)
+    {
+      DECL_COMDAT_GROUP (fns[1]) = comdat_group;
+      if (fns[2])
+	/* If *[CD][12]* dtors go into the *[CD]5* comdat group and dtor is
+	   virtual, it goes into the same comdat group as well.  */
+	DECL_COMDAT_GROUP (fns[2]) = comdat_group;
+    }
+
   /* We don't need to process the original function any further.  */
   return 1;
 }

	Jakub

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 5)

[Jason Merrill]

On 12/01/2009 02:22 PM, Jakub Jelinek wrote:
> After discussions on IRC, here is the latest version of the patch.
> For now the problematic implicitly defined virtual comdat destructors aren't
> optimized, even when the base and complete dtor have the same body, as the
> deleting dtor isn't necessarily emitted iff the base/complete dtors are
> emitted.

OK.

Jason

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 5)

[Dave Korn]

Jakub Jelinek wrote:

> Bootstrapped/regtested on x86_64-linux (with the checking patch on top of
> it)

  Is that the Y478 one?

    cheers,
      DaveK

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 3)

[Daniel Jacobowitz]

On Mon, Nov 30, 2009 at 05:36:53PM -0500, Jakub Jelinek wrote:
> Hi!
> 
> Here is the latest version of the cdtor optimization patch.
> The comdat group name is *[CD]5* and if the dtor is virtual, deleting dtor
> is emitted into that comdat group as well.
> libstdc++ symbol version script has been adjusted, so that it exports
> exactly what it used to export before this patch.
> On x86_64-linux this patch saves 21KB of libstdc++.so's .text.

Can you explain how this patch is ABI-safe at the object (.o) level?

Suppose that some other or older compiler creates a C0 comdat group in
first.o.  second.o references the C0 symbol.  Then suppose that GCC
creates a C5 comdat group in third.o, and defines C0 and C1 in that
group.  A reference to C1 will now pull in the group in third.o and
we have two definitions of the global symbol for the C0 constructor.
Neither can be discarded at this point.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 3)

[Jakub Jelinek]

On Tue, Dec 01, 2009 at 04:03:25PM -0500, Daniel Jacobowitz wrote:
> On Mon, Nov 30, 2009 at 05:36:53PM -0500, Jakub Jelinek wrote:
> > Hi!
> > 
> > Here is the latest version of the cdtor optimization patch.
> > The comdat group name is *[CD]5* and if the dtor is virtual, deleting dtor
> > is emitted into that comdat group as well.
> > libstdc++ symbol version script has been adjusted, so that it exports
> > exactly what it used to export before this patch.
> > On x86_64-linux this patch saves 21KB of libstdc++.so's .text.
> 
> Can you explain how this patch is ABI-safe at the object (.o) level?
> 
> Suppose that some other or older compiler creates a C0 comdat group in
> first.o.  second.o references the C0 symbol.  Then suppose that GCC
> creates a C5 comdat group in third.o, and defines C0 and C1 in that
> group.  A reference to C1 will now pull in the group in third.o and
> we have two definitions of the global symbol for the C0 constructor.
> Neither can be discarded at this point.

That's why this is done only if the symbols are weak.  If you mix .o files
from different G++ versions (pre-4.5 and 4.5+), then worst case you'll
end up with some .text duplication, but as the symbols are weak, the linker
will just pick one of the definitions for the symbols.  When you don't mix
.o files from different G++ versions in one .so or executable, you on the
other side gain code size savings.

	Jakub

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 3)

[Daniel Jacobowitz]

On Tue, Dec 01, 2009 at 04:15:38PM -0500, Jakub Jelinek wrote:
> That's why this is done only if the symbols are weak.  If you mix .o files
> from different G++ versions (pre-4.5 and 4.5+), then worst case you'll
> end up with some .text duplication, but as the symbols are weak, the linker
> will just pick one of the definitions for the symbols.  When you don't mix
> .o files from different G++ versions in one .so or executable, you on the
> other side gain code size savings.

I see.  Unless the ABI has something to say on the matter, I would
suggest using a GNU-specific string for the COMDAT signature; IMO
that's less confusing.

As I said on IRC, I tried to test my theories with ARM's compiler.
But that compiler already causes GNU ld fits; among other things, it
puts both C1 and C2 constructors into the group with a C1 signature.
And something involving discarded hidden symbols made my linker
fall down, anyway.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 5)

[Dave Korn]

Dave Korn wrote:
> Jakub Jelinek wrote:
> 
>> Bootstrapped/regtested on x86_64-linux (with the checking patch on top of
>> it)
> 
>   Is that the Y478 one?

  I assumed it was, but for reasons stated elsewhere it wasn't practical to
run with full checking on so I tested it unchecked; nothing untoward happened.
 JFTR.

    cheers,
      DaveK

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 5)

[Jakub Jelinek]

On Fri, Dec 04, 2009 at 06:33:41PM +0000, Dave Korn wrote:
>   I assumed it was, but for reasons stated elsewhere it wasn't practical to
> run with full checking on so I tested it unchecked; nothing untoward happened.

I think cygwin doesn't HAVE_COMDAT_GROUP anyway, so it wouldn't make any
difference.

	Jakub

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 5)

[Dave Korn]

Jakub Jelinek wrote:
> On Fri, Dec 04, 2009 at 06:33:41PM +0000, Dave Korn wrote:
>>   I assumed it was, but for reasons stated elsewhere it wasn't practical to
>> run with full checking on so I tested it unchecked; nothing untoward happened.
> 
> I think cygwin doesn't HAVE_COMDAT_GROUP anyway, so it wouldn't make any
> difference.

  Heh, you're right.  I was worrying for nothing(*).

    cheers,
      DaveK
-- 
(*) - Well, not quite for nothing, I was worrying because /something/ recently
caused some kind of breakage in that area somewhere between r.154010 and
r.154114, and although it got fixed again within the week I thought I'd do
some proactive keeping-an-eye-out-for-problems-before-they-arise.

Re: [C++ PATCH] Optimize C++ comdat ctors/dtors in classes without virtual bases (PR c++/3187, take 5)

[H.J. Lu]

On Tue, Dec 1, 2009 at 11:22 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> After discussions on IRC, here is the latest version of the patch.
> For now the problematic implicitly defined virtual comdat destructors aren't
> optimized, even when the base and complete dtor have the same body, as the
> deleting dtor isn't necessarily emitted iff the base/complete dtors are
> emitted.
>
> The nice effect of that patch is that the libstdc++ symbol version script
> changes aren't needed.
>
> Bootstrapped/regtested on x86_64-linux (with the checking patch on top of
> it), ok for trunk?
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42508


-- 
H.J.