diff mbox series

[C++] Fix clone_body (PR c++/86669)

Message ID 20181128084247.GX12380@tucnak
State New
Headers show
Series [C++] Fix clone_body (PR c++/86669) | expand

Commit Message

Jakub Jelinek Nov. 28, 2018, 8:42 a.m. UTC 
Hi!

Whenever we need to clone a cdtor (either because the target doesn't support
aliases the way we need, e.g. initlist105.C testcase on darwin, where it has
been originally reported, or when it has virtual bases, like the made up
initlist106.C on x86_64-linux), we rely on DECL_INITIAL of the local
variables being unshared, because the tree unsharing gimplify.c performs
doesn't unshare DECL_INITIAL.  clone_body has some code to recurse on the
DECL_INITIAL, but it handles just decls FOR_EACH_LOCAL_DECL walks.  I
believe it is generally ok that not all temporaries are covered in local
decls, the gimplifier should take care of those fine if we don't need
debug info for them.
On the initlist10*.C testcases there is a temporary created for the
initializer list that isn't among the local decls and thus doesn't have
the DECL_INITIAL unshared, when we then gimplify one of the ctors, the
DECL_INITIAL is modified and when we gimplify the other ctor, the
initialization of the len is lost.

The following patch fixes it by hooking into the callback that creates new
decls, whenever it creates one and it is an automatic variable in the new
clone, it walks the initializer right away and clone_body only walks other
vars (like statics etc.).  The pr86669.C testcase covers a thinko I've made
in the first version of the patch, without the insert_decl_map call we
recurse infinitely on that testcase.  The callback isn't walking
DECL_INITIAL of all the decls, just the automatic ones, because I'm not
really sure if we want to walk those e.g. for global VAR_DECLs or some
others that copy_decl_no_change handles.

Bootstrapped/regtested on x86_64-linux and i686-linux (earlier version
without the insert_decl_map call), ok for trunk if it passes another
bootstrap/regtest in the current form?

2018-11-28  Jakub Jelinek  <jakub@redhat.com>

	PR c++/86669
	* optimize.c (clone_body_copy_decl): New function.
	(clone_body): Use it for base cdtors.  Remap here only
	DECL_INITIAL of decls that don't have DECL_CONTEXT of the
	new clone.

	* g++.dg/cpp0x/initlist105.C: New test.
	* g++.dg/cpp0x/initlist106.C: New test.
	* g++.dg/other/pr86669.C: New test.

 
	Jakub

Comments

Jakub Jelinek Nov. 29, 2018, 7:45 a.m. UTC | #1 
On Wed, Nov 28, 2018 at 09:42:47AM +0100, Jakub Jelinek wrote:
> Bootstrapped/regtested on x86_64-linux and i686-linux (earlier version
> without the insert_decl_map call), ok for trunk if it passes another
> bootstrap/regtest in the current form?

FYI, bootstrapped/regtested successfully on both.

> 2018-11-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/86669
> 	* optimize.c (clone_body_copy_decl): New function.
> 	(clone_body): Use it for base cdtors.  Remap here only
> 	DECL_INITIAL of decls that don't have DECL_CONTEXT of the
> 	new clone.
> 
> 	* g++.dg/cpp0x/initlist105.C: New test.
> 	* g++.dg/cpp0x/initlist106.C: New test.
> 	* g++.dg/other/pr86669.C: New test.

	Jakub
Jakub Jelinek Dec. 5, 2018, 11:03 a.m. UTC | #2 
Hi!

On Wed, Nov 28, 2018 at 09:42:47AM +0100, Jakub Jelinek wrote:
> 2018-11-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/86669
> 	* optimize.c (clone_body_copy_decl): New function.
> 	(clone_body): Use it for base cdtors.  Remap here only
> 	DECL_INITIAL of decls that don't have DECL_CONTEXT of the
> 	new clone.
> 
> 	* g++.dg/cpp0x/initlist105.C: New test.
> 	* g++.dg/cpp0x/initlist106.C: New test.
> 	* g++.dg/other/pr86669.C: New test.

I've like to ping this patch.

	Jakub
Jason Merrill Dec. 5, 2018, 8:49 p.m. UTC | #3 
On 11/28/18 3:42 AM, Jakub Jelinek wrote:
> Whenever we need to clone a cdtor (either because the target doesn't support
> aliases the way we need, e.g. initlist105.C testcase on darwin, where it has
> been originally reported, or when it has virtual bases, like the made up
> initlist106.C on x86_64-linux), we rely on DECL_INITIAL of the local
> variables being unshared, because the tree unsharing gimplify.c performs
> doesn't unshare DECL_INITIAL.  clone_body has some code to recurse on the
> DECL_INITIAL, but it handles just decls FOR_EACH_LOCAL_DECL walks.  I
> believe it is generally ok that not all temporaries are covered in local
> decls, the gimplifier should take care of those fine if we don't need
> debug info for them.

I think any temporaries that have DECL_INITIAL should be pushed so that 
they end up in local_decls.  set_up_extended_ref_temp already adds a 
DECL_EXPR for it, I guess we need a pushdecl as well.  Though the 
comment for get_temp_regvar suggests that this is problematic somehow.

Jason
Jakub Jelinek Dec. 5, 2018, 8:50 p.m. UTC | #4 
On Wed, Dec 05, 2018 at 03:49:26PM -0500, Jason Merrill wrote:
> On 11/28/18 3:42 AM, Jakub Jelinek wrote:
> > Whenever we need to clone a cdtor (either because the target doesn't support
> > aliases the way we need, e.g. initlist105.C testcase on darwin, where it has
> > been originally reported, or when it has virtual bases, like the made up
> > initlist106.C on x86_64-linux), we rely on DECL_INITIAL of the local
> > variables being unshared, because the tree unsharing gimplify.c performs
> > doesn't unshare DECL_INITIAL.  clone_body has some code to recurse on the
> > DECL_INITIAL, but it handles just decls FOR_EACH_LOCAL_DECL walks.  I
> > believe it is generally ok that not all temporaries are covered in local
> > decls, the gimplifier should take care of those fine if we don't need
> > debug info for them.
> 
> I think any temporaries that have DECL_INITIAL should be pushed so that they
> end up in local_decls.  set_up_extended_ref_temp already adds a DECL_EXPR
> for it, I guess we need a pushdecl as well.  Though the comment for
> get_temp_regvar suggests that this is problematic somehow.

Ok, will play with it tomorrow.

	Jakub
diff mbox series

Patch

--- gcc/cp/optimize.c.jj	2018-11-27 20:22:39.659560471 +0100
+++ gcc/cp/optimize.c	2018-11-28 09:16:40.523157389 +0100
@@ -62,6 +62,21 @@  update_cloned_parm (tree parm, tree clon
 }
 
 
+/* Helper function of clone_body.  If copy_decl_no_change creates
+   a decl local to the base ctor or dtor, remap the initializer too.  */
+
+static tree
+clone_body_copy_decl (tree decl, copy_body_data *id)
+{
+  tree copy = copy_decl_no_change (decl, id);
+  if (DECL_CONTEXT (copy) == id->dst_fn && DECL_INITIAL (copy))
+    {
+      insert_decl_map (id, decl, copy);
+      walk_tree (&DECL_INITIAL (copy), copy_tree_body_r, id, NULL);
+    }
+  return copy;
+}
+
 /* FN is a function in High GIMPLE form that has a complete body and no
    CFG.  CLONE is a function whose body is to be set to a copy of FN,
    mapping argument declarations according to the ARG_MAP splay_tree.  */
@@ -89,20 +104,26 @@  clone_body (tree clone, tree fn, void *a
   /* We're not inside any EH region.  */
   id.eh_lp_nr = 0;
 
+  /* Make sure to remap initializers of non-static variables in the
+     base variant.  */
+  if (DECL_NAME (clone) == base_dtor_identifier
+      || DECL_NAME (clone) == base_ctor_identifier)
+    id.copy_decl = clone_body_copy_decl;
+
   stmts = DECL_SAVED_TREE (fn);
   walk_tree (&stmts, copy_tree_body_r, &id, NULL);
 
   /* Also remap the initializer of any static variables so that they (in
      particular, any label addresses) correspond to the base variant rather
      than the abstract one.  */
-  if (DECL_NAME (clone) == base_dtor_identifier
-      || DECL_NAME (clone) == base_ctor_identifier)
+  if (id.copy_decl == clone_body_copy_decl)
     {
       unsigned ix;
       tree decl;
 
       FOR_EACH_LOCAL_DECL (DECL_STRUCT_FUNCTION (fn), ix, decl)
-        walk_tree (&DECL_INITIAL (decl), copy_tree_body_r, &id, NULL);
+	if (DECL_CONTEXT (decl) != clone)
+	  walk_tree (&DECL_INITIAL (decl), copy_tree_body_r, &id, NULL);
     }
 
   append_to_statement_list_force (stmts, &DECL_SAVED_TREE (clone));
--- gcc/testsuite/g++.dg/cpp0x/initlist105.C.jj	2018-11-28 08:46:36.173811505 +0100
+++ gcc/testsuite/g++.dg/cpp0x/initlist105.C	2018-11-28 08:46:36.173811505 +0100
@@ -0,0 +1,28 @@ 
+// PR c++/86669
+// { dg-do run { target c++11 } }
+
+#include <initializer_list>
+
+struct S { S (); };
+struct T : public S {};
+int cnt;
+void foo (int) { cnt++; }
+
+S::S ()
+{
+  int e = 1, f = 2, g = 3, h = 4;
+
+  for (auto k : { e, f, g, h })
+    foo (k);
+}
+
+int
+main ()
+{
+  S s;
+  if (cnt != 4)
+    __builtin_abort ();
+  T t;
+  if (cnt != 8)
+    __builtin_abort ();
+}
--- gcc/testsuite/g++.dg/cpp0x/initlist106.C.jj	2018-11-28 09:25:07.844790923 +0100
+++ gcc/testsuite/g++.dg/cpp0x/initlist106.C	2018-11-28 09:24:39.990251676 +0100
@@ -0,0 +1,29 @@ 
+// PR c++/86669
+// { dg-do run { target c++11 } }
+
+#include <initializer_list>
+
+struct A { };
+struct S : virtual public A { S (); };
+struct T : public S, virtual public A {};
+int cnt;
+void foo (int) { cnt++; }
+
+S::S ()
+{
+  int e = 1, f = 2, g = 3, h = 4;
+
+  for (auto k : { e, f, g, h })
+    foo (k);
+}
+
+int
+main ()
+{
+  S s;
+  if (cnt != 4)
+    __builtin_abort ();
+  T t;
+  if (cnt != 8)
+    __builtin_abort ();
+}
--- gcc/testsuite/g++.dg/other/pr86669.C.jj	2018-11-28 09:29:20.528593460 +0100
+++ gcc/testsuite/g++.dg/other/pr86669.C	2018-11-28 09:29:27.532475489 +0100
@@ -0,0 +1,10 @@ 
+// PR c++/86669
+// { dg-do compile }
+
+struct S { S (); };
+struct T : public S {};
+
+S::S ()
+{
+  int *p = { (int *) &p };
+}