Patchwork [C++] Fix constexpr related wrong-code (PR c++/46626)

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 23, 2010, 8:49 p.m.
Message ID <20101223204951.GJ16156@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/76554/
State New
Headers show

Comments

Jakub Jelinek - Dec. 23, 2010, 8:49 p.m.
Hi!

The testcase in the patch is miscompiled.  The problem seems to be
that the B::B() ctor contains
{
  <<cleanup_point <<< Unknown tree: expr_stmt
  A::A (&((struct B *) this)->D.1396) >>>>>;
  <<< Unknown tree: cleanup_stmt
  <<cleanup_point <<< Unknown tree: expr_stmt
  (void) (((struct B *) this)->D.1396._vptr.A = &_ZTV1B + 16) >>>>>;
  A::~A ((struct A *) this)
   >>>;
}
and build_data_member_initialization ignores CLEANUP_STMT, which means
the constexpr ctor is expanded as setting _vptr.A field to &_ZTV1A + 16
instead of &_ZTV1B + 16.  The following patch ignores the cleanup (A::~A
dtor in this case), but handles CLEANUP_BODY.

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

2010-12-23  Jakub Jelinek  <jakub@redhat.com>

	PR c++/46626
	* semantics.c (build_data_member_initialization): For CLEANUP_STMT
	recurse into CLEANUP_BODY.

	* g++.dg/cpp0x/constexpr-base4.C: New test.


	Jakub
Gabriel Dos Reis - Dec. 23, 2010, 9:42 p.m.
On Thu, Dec 23, 2010 at 2:49 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The testcase in the patch is miscompiled.  The problem seems to be
> that the B::B() ctor contains
> {
>  <<cleanup_point <<< Unknown tree: expr_stmt
>  A::A (&((struct B *) this)->D.1396) >>>>>;
>  <<< Unknown tree: cleanup_stmt
>  <<cleanup_point <<< Unknown tree: expr_stmt
>  (void) (((struct B *) this)->D.1396._vptr.A = &_ZTV1B + 16) >>>>>;
>  A::~A ((struct A *) this)
>   >>>;
> }
> and build_data_member_initialization ignores CLEANUP_STMT, which means
> the constexpr ctor is expanded as setting _vptr.A field to &_ZTV1A + 16
> instead of &_ZTV1B + 16.  The following patch ignores the cleanup (A::~A
> dtor in this case), but handles CLEANUP_BODY.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Jason had worked several times and made many improvements
in this particular area, so I'll him approve (or disapprove.)

One thing that `constexpr' has  shown is our lack of systematic
documentation of what kind of trees can appear where, and definite
unambiguous mapping of C++ souce-level constructs to internal
GCC trees... :-/
Jason Merrill - Dec. 24, 2010, 7:28 p.m.
OK.

Jason

Patch

--- gcc/cp/semantics.c.jj	2010-12-16 10:55:31.000000000 +0100
+++ gcc/cp/semantics.c	2010-12-23 13:02:11.000000000 +0100
@@ -5440,11 +5440,25 @@  build_data_member_initialization (tree t
   if (t == error_mark_node)
     return false;
   if (TREE_CODE (t) == CLEANUP_STMT)
-    /* We can't see a CLEANUP_STMT in a constructor for a literal class,
-       but we can in a constexpr constructor for a non-literal class.  Just
-       ignore it; either all the initialization will be constant, in which
-       case the cleanup can't run, or it can't be constexpr.  */
-    return true;
+    {
+      /* We can't see a CLEANUP_STMT in a constructor for a literal class,
+	 but we can in a constexpr constructor for a non-literal class.  Just
+	 ignore it; either all the initialization will be constant, in which
+	 case the cleanup can't run, or it can't be constexpr.
+	 Still recurse into CLEANUP_BODY.  */
+      t = CLEANUP_BODY (t);
+      if (TREE_CODE (t) == STATEMENT_LIST)
+	{
+	  tree_stmt_iterator i;
+	  for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i))
+	    {
+	      if (! build_data_member_initialization (tsi_stmt (i), vec))
+		return false;
+	    }
+	  return true;
+	}
+      return build_data_member_initialization (t, vec);
+    }
   if (TREE_CODE (t) == CONVERT_EXPR)
     t = TREE_OPERAND (t, 0);
   if (TREE_CODE (t) == INIT_EXPR
--- gcc/testsuite/g++.dg/cpp0x/constexpr-base4.C.jj	2010-12-23 13:04:46.000000000 +0100
+++ gcc/testsuite/g++.dg/cpp0x/constexpr-base4.C	2010-12-23 13:03:48.000000000 +0100
@@ -0,0 +1,28 @@ 
+// PR c++/46626
+// { dg-do run }
+// { dg-options "-std=c++0x" }
+
+struct A
+{
+  virtual void f () = 0;
+  virtual ~A () { }
+};
+
+struct B : A
+{
+  virtual void f () { }
+};
+
+static void
+foo (A *a)
+{
+  a->f ();
+}
+
+int
+main ()
+{
+  B b;
+  foo (&b);
+  return 0;
+}