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

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

[Jakub Jelinek]

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.

--- 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 };
+}
 
	Jakub

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

[Jakub Jelinek]

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

Patch ping (Re: [C++ PATCH] Fix clone_body (PR c++/86669))

[Jakub Jelinek]

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

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

[Jason Merrill]

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

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

[Jakub Jelinek]

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

[C++ PATCH] Fix make_temporary_var_for_ref_to_temp (PR c++/86669)

[Jakub Jelinek]

On Wed, Dec 05, 2018 at 09:50:56PM +0100, Jakub Jelinek wrote:
> 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.

The following fixes the testcase too and passed bootstrap/regtest on
x86_64-linux and i686-linux, ok for trunk?

2018-12-07  Jakub Jelinek  <jakub@redhat.com>

	PR c++/86669
	* call.c (make_temporary_var_for_ref_to_temp): Call pushdecl even for
	automatic vars.

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

--- gcc/cp/call.c.jj	2018-11-29 23:11:38.386646583 +0100
+++ gcc/cp/call.c	2018-12-06 13:50:26.766178596 +0100
@@ -11130,14 +11130,12 @@ make_temporary_var_for_ref_to_temp (tree
       tree name = mangle_ref_init_variable (decl);
       DECL_NAME (var) = name;
       SET_DECL_ASSEMBLER_NAME (var, name);
-
-      var = pushdecl (var);
     }
   else
     /* Create a new cleanup level if necessary.  */
     maybe_push_cleanup_level (type);
 
-  return var;
+  return pushdecl (var);
 }
 
 /* EXPR is the initializer for a variable DECL of reference or
--- gcc/testsuite/g++.dg/cpp0x/initlist105.C.jj	2018-12-06 13:31:35.993609689 +0100
+++ gcc/testsuite/g++.dg/cpp0x/initlist105.C	2018-12-06 13:31:35.993609689 +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-12-06 13:31:35.993609689 +0100
+++ gcc/testsuite/g++.dg/cpp0x/initlist106.C	2018-12-06 13:31:35.993609689 +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-12-06 13:31:35.993609689 +0100
+++ gcc/testsuite/g++.dg/other/pr86669.C	2018-12-06 13:31:35.993609689 +0100
@@ -0,0 +1,10 @@
+// PR c++/86669
+// { dg-do compile }
+
+struct S { S (); };
+struct T : public S {};
+
+S::S ()
+{
+  int *p = { (int *) &p };
+}


	Jakub

Re: [C++ PATCH] Fix make_temporary_var_for_ref_to_temp (PR c++/86669)

[Jason Merrill]

On 12/6/18 6:26 PM, Jakub Jelinek wrote:
> On Wed, Dec 05, 2018 at 09:50:56PM +0100, Jakub Jelinek wrote:
>> 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.
> 
> The following fixes the testcase too and passed bootstrap/regtest on
> x86_64-linux and i686-linux, ok for trunk?
> 
> 2018-12-07  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/86669
> 	* call.c (make_temporary_var_for_ref_to_temp): Call pushdecl even for
> 	automatic vars.

OK, thanks.

Jason