[PATCH] PR c++/PR48035

[PATCH] PR c++/PR48035

[Dodji Seketeli]

Hello,

In the example of the patch below, the zero-initialization of the
instance of E runs past the size of the object.

That's because build_zero_init recursively tries to initialize all the
sub-objects of 'e' without handling cases where 'e' could have
sub-objects for virtual direct or indirect primary bases of E, that
would come after a sub-object for the primary base of E.

More specifically, the layout of 'e' is (I left the vptrs out
for clarity):

+subobject<B>  <-- comes first b/c B is the primary base of E
  +subobject<A>
+subobject<Implementation> <-- this one doesn't include any
  +subobject<C>                subjobject of B b/c B is already
                               included above.

And the code currently generated tries to zero-initialize
subobject<Implementation>.subobject<B> even though it is not present.

The patch below teaches build_zero_init to consider that after a
subobject for a primary base the object has no subobject for virtual
bases that are direct or indirect primary bases.

Tested on x86_64-unknown-linux-gnu and i686-unknown-linux-gnu against
trunk.

PS: Thanks to Jakub for coming up with the placement new idea that eases
the writing of a deja-gnu test for this PR, and for bootstrapping the
patch on his fast iron on i686 and x86_64 for all languages.

-- 
		Dodji

From b52987810a313657202fc7ecae6b503311146302 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Thu, 10 Mar 2011 14:10:05 +0100
Subject: [PATCH] PR c++/PR48035

gcc/cp/

	* cp-tree.h (is_primary_base_of, is_virtual_base_of): Declare new
	functions.
	* class.c (is_base_of, is_virtual_base_of, is_primary_base_of):
	Define new functions.
	* init.c (build_zero_init_1):  Extract from from build_zero_init.
	Handle sub-objects for virtual primary bases allocated after a
	sub-object of a primary base.
	(build_zero_init_1):  Use build_zero_init_1.

gcc/testsuite/

	* g++.dg/inherit/virtual8.C: New test.
---
 gcc/cp/class.c                          |   74 ++++++++++++++++++++++++++
 gcc/cp/cp-tree.h                        |    2 +
 gcc/cp/init.c                           |   86 +++++++++++++++++++++++-------
 gcc/testsuite/g++.dg/inherit/virtual8.C |   52 +++++++++++++++++++
 4 files changed, 194 insertions(+), 20 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/inherit/virtual8.C

diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 1325260..b811e8f 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -7008,6 +7008,80 @@ get_primary_binfo (tree binfo)
   return copied_binfo (primary_base, binfo);
 }
 
+/* This is a subroutine of is_virtual_base_of.
+
+   Returns TRUE if BASE is a direct or indirect base class of TYPE,
+   FALSE otherwise.  */
+
+static bool
+is_base_of (tree base, tree type)
+{
+  int i;
+  tree binfo;
+
+  for (i = 0; BINFO_BASE_ITERATE (TYPE_BINFO (type), i, binfo); ++i)
+    {
+      if (same_type_p (BINFO_TYPE (binfo), base)
+	  || is_base_of (base, BINFO_TYPE (binfo)))
+	return true;
+    }
+  return false;
+}
+
+/* Returns TRUE if BASE is a direct or indirect virtual base class of
+   TYPE, FALSE otherwise.  */
+
+bool
+is_virtual_base_of (tree base, tree type)
+{
+  int i;
+  tree binfo;
+
+  for (i = 0; BINFO_BASE_ITERATE (TYPE_BINFO (type), i, binfo); ++i)
+    {
+      if (!BINFO_VIRTUAL_P (binfo))
+	continue;
+
+      if (same_type_p (BINFO_TYPE (binfo), base))
+	return true;
+
+      if (is_base_of (base, BINFO_TYPE (binfo)))
+	return true;
+    }
+  return false;
+}
+
+/* Returns TRUE if BASE is a direct primary base class of TYPE.  If
+   INDIRECT_P is TRUE, then the function returns TRUE if BASE is a
+   direct or indirect base class of TYPE.  Returns FALSE
+   otherwise.  */
+
+bool
+is_primary_base_of (tree base, tree type, bool indirect_p)
+{
+  int i;
+  tree binfo;
+
+  if (!CLASS_TYPE_P (type)
+      || !CLASS_TYPE_P (base))
+    return false;
+
+  if (CLASSTYPE_HAS_PRIMARY_BASE_P (type)
+      && same_type_p (base,
+		      BINFO_TYPE (get_primary_binfo (TYPE_BINFO (type)))))
+    return true;
+
+  if (!indirect_p)
+    return false;
+
+  for (i = 0; BINFO_BASE_ITERATE (TYPE_BINFO (type), i, binfo); ++i)
+    {
+      if (is_primary_base_of (base, BINFO_TYPE (binfo), true))
+	return true;
+    }
+  return false;
+}
+
 /* If INDENTED_P is zero, indent to INDENT. Return nonzero.  */
 
 static int
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 4b49046..cba5ddb 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -4727,6 +4727,8 @@ extern void fixup_attribute_variants		(tree);
 extern tree* decl_cloned_function_p		(const_tree, bool);
 extern void clone_function_decl			(tree, int);
 extern void adjust_clone_args			(tree);
+extern bool is_primary_base_of                  (tree, tree, bool);
+extern bool is_virtual_base_of                  (tree, tree);
 
 /* in cvt.c */
 extern tree convert_to_reference		(tree, tree, int, int, tree);
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 56f66fa..7beb94f 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -130,20 +130,22 @@ initialize_vtbl_ptrs (tree addr)
   dfs_walk_once (TYPE_BINFO (type), dfs_initialize_vtbl_ptrs, NULL, list);
 }
 
-/* Return an expression for the zero-initialization of an object with
-   type T.  This expression will either be a constant (in the case
-   that T is a scalar), or a CONSTRUCTOR (in the case that T is an
-   aggregate), or NULL (in the case that T does not require
-   initialization).  In either case, the value can be used as
-   DECL_INITIAL for a decl of the indicated TYPE; it is a valid static
-   initializer. If NELTS is non-NULL, and TYPE is an ARRAY_TYPE, NELTS
-   is the number of elements in the array.  If STATIC_STORAGE_P is
-   TRUE, initializers are only generated for entities for which
-   zero-initialization does not simply mean filling the storage with
-   zero bytes.  */
+/* A subroutine of build_zero_init.
 
-tree
-build_zero_init (tree type, tree nelts, bool static_storage_p)
+   The parameters are the same as for build_zero_init.  If
+   CURRENT_OBJECT_TYPE is different from NULL_TREE, it means that we
+   are currently initializing sub-objects of an object of type
+   CURRENT_OBJECT_TYPE and we have already initialized the sub-object
+   for the primary base of CURRENT_OBJECT_TYPE.  In that case, this
+   function will avoid initializing sub-objects for virtual direct or
+   indirect primary bases as per the C++ ABI specficication
+   [2.4.III/"Virtual Base Allocation"].  */
+
+static tree
+build_zero_init_1 (tree type,
+		   tree nelts,
+		   bool static_storage_p,
+		   tree current_object_type)
 {
   tree init = NULL_TREE;
 
@@ -188,17 +190,42 @@ build_zero_init (tree type, tree nelts, bool static_storage_p)
 	  if (TREE_CODE (field) != FIELD_DECL)
 	    continue;
 
+	  /* If we are initializing a sub-object of
+	     CURRENT_OBJECT_TYPE [for which a primary base class
+	     sub-object has already been initialized] then we must NOT
+	     initialize any sub-object from a virtual base that is a
+	     direct or indirect primary base of
+	     CURRENT_OBJECT_TYPE.  */
+	  if (current_object_type
+	      && is_virtual_base_of (TREE_TYPE (field), current_object_type)
+	      && is_primary_base_of (TREE_TYPE (field), current_object_type,
+				     /*indirect_p=*/true))
+	    continue;
+
 	  /* Note that for class types there will be FIELD_DECLs
 	     corresponding to base classes as well.  Thus, iterating
 	     over TYPE_FIELDs will result in correct initialization of
 	     all of the subobjects.  */
 	  if (!static_storage_p || !zero_init_p (TREE_TYPE (field)))
 	    {
-	      tree value = build_zero_init (TREE_TYPE (field),
-					    /*nelts=*/NULL_TREE,
-					    static_storage_p);
+	      tree value = build_zero_init_1 (TREE_TYPE (field),
+					      /*nelts=*/NULL_TREE,
+					      static_storage_p,
+					      current_object_type);
 	      if (value)
 		CONSTRUCTOR_APPEND_ELT(v, field, value);
+	      
+	      /* Dectect the case where, while initializing an object
+		 of type TYPE, we have just initialized a sub-object
+		 of TYPE for a primary base.  That sub-object would
+		 then be FIELD.  In that case, we must be remember
+		 what object we are initializing so that we can apply
+		 the rules of sub-objects allocation for virtual
+		 bases.  */
+	      if (current_object_type == NULL_TREE
+		  && is_primary_base_of (TREE_TYPE (field), type,
+					 /*indirect_p=*/false))
+		current_object_type = type;
 	    }
 
 	  /* For unions, only the first field is initialized.  */
@@ -244,9 +271,10 @@ build_zero_init (tree type, tree nelts, bool static_storage_p)
 	    ce->index = build2 (RANGE_EXPR, sizetype, size_zero_node,
 				max_index);
 
-	  ce->value = build_zero_init (TREE_TYPE (type),
-				       /*nelts=*/NULL_TREE,
-				       static_storage_p);
+	  ce->value = build_zero_init_1 (TREE_TYPE (type),
+					 /*nelts=*/NULL_TREE,
+					 static_storage_p,
+					 NULL_TREE);
 	}
 
       /* Build a constructor to contain the initializations.  */
@@ -261,7 +289,25 @@ build_zero_init (tree type, tree nelts, bool static_storage_p)
   if (init)
     TREE_CONSTANT (init) = 1;
 
-  return init;
+  return init;  
+}
+
+/* Return an expression for the zero-initialization of an object with
+   type T.  This expression will either be a constant (in the case
+   that T is a scalar), or a CONSTRUCTOR (in the case that T is an
+   aggregate), or NULL (in the case that T does not require
+   initialization).  In either case, the value can be used as
+   DECL_INITIAL for a decl of the indicated TYPE; it is a valid static
+   initializer. If NELTS is non-NULL, and TYPE is an ARRAY_TYPE, NELTS
+   is the number of elements in the array.  If STATIC_STORAGE_P is
+   TRUE, initializers are only generated for entities for which
+   zero-initialization does not simply mean filling the storage with
+   zero bytes.  */
+
+tree
+build_zero_init (tree type, tree nelts, bool static_storage_p)
+{
+  return build_zero_init_1 (type, nelts, static_storage_p, NULL_TREE);
 }
 
 /* Return a suitable initializer for value-initializing an object of type
diff --git a/gcc/testsuite/g++.dg/inherit/virtual8.C b/gcc/testsuite/g++.dg/inherit/virtual8.C
new file mode 100644
index 0000000..28626f4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/inherit/virtual8.C
@@ -0,0 +1,52 @@
+// Origin PR c++/PR48035
+// { dg-do run }
+
+#include <new>
+#include <cstring>
+
+struct A
+{
+    virtual void foo (void) {}
+    virtual ~A () {}
+};
+
+struct B : public A
+{
+    virtual ~B () {}
+};
+
+struct C
+{
+    virtual ~C ()
+    {
+    }
+    int c;
+};
+
+struct D : public virtual B, public C
+{
+    virtual ~D () {}
+};
+
+struct E : public virtual D
+{
+    virtual ~E ()
+    {
+    }
+};
+
+int
+main ()
+{
+    char *v = new char[sizeof (E) + 16];
+    memset (v, 0x55, sizeof (E) + 16);
+    E *e = new (v) E ();
+    e->~E ();
+
+    for (unsigned i = sizeof (E); i < sizeof (E) + 16; ++i)
+        if (v[i] != 0x55)
+            return 1;
+
+    delete[] v;
+    return 0;
+}
-- 
1.7.3.4

Re: [PATCH] PR c++/PR48035

[Jakub Jelinek]

On Thu, Mar 10, 2011 at 05:14:12PM +0100, Dodji Seketeli wrote:
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/inherit/virtual8.C
...
> +
> +int
> +main ()
> +{
> +    char *v = new char[sizeof (E) + 16];
> +    memset (v, 0x55, sizeof (E) + 16);
> +    E *e = new (v) E ();
> +    e->~E ();
> +
> +    for (unsigned i = sizeof (E); i < sizeof (E) + 16; ++i)
> +        if (v[i] != 0x55)
> +            return 1;
> +
> +    delete[] v;
> +    return 0;
> +}

The standard way of signalizing a test failure is calling abort (),
not returning 1, see http://gcc.gnu.org/codingconventions.html
(Testsuite Conventions).  While exit (0); isn't widely used
for testcase success (and return 0; can be omitted at the end of
main in C++ or C99), abort as the method of signalizing failure
is the norm.

	Jakub

Re: [PATCH] PR c++/PR48035

[Mike Stump]

On Mar 10, 2011, at 10:22 AM, Jakub Jelinek wrote:
> The standard way of signalizing a test failure is calling abort (),
> not returning 1, see http://gcc.gnu.org/codingconventions.html

Actually, returning 0 or 1 is a perfectly fine way to signal pass/fail in the testsuites, as it exit.

Re: [PATCH] PR c++/PR48035

[Jakub Jelinek]

On Thu, Mar 10, 2011 at 12:31:57PM -0800, Mike Stump wrote:
> On Mar 10, 2011, at 10:22 AM, Jakub Jelinek wrote:
> > The standard way of signalizing a test failure is calling abort (),
> > not returning 1, see http://gcc.gnu.org/codingconventions.html
> 
> Actually, returning 0 or 1 is a perfectly fine way to signal pass/fail in the testsuites, as it exit.

E.g.
http://gcc.gnu.org/wiki/HowToPrepareATestcase
recommends abort instead and it is existing practice too.

	Jakub

Re: [PATCH] PR c++/PR48035

[Dodji Seketeli]

Jakub Jelinek <jakub@redhat.com> writes:

> http://gcc.gnu.org/wiki/HowToPrepareATestcase
> recommends abort instead and it is existing practice too.

Sure.  I am going to update my local copy of the patch and re-post.  I
used return as I noticed it was working.  I didn't realize the
abort/exit was the recommended custom.  Thanks for the notice.

-- 
		Dodji

Re: [PATCH] PR c++/PR48035

[Jason Merrill]

On 03/10/2011 03:45 PM, Jakub Jelinek wrote:
> E.g.
> http://gcc.gnu.org/wiki/HowToPrepareATestcase
> recommends abort instead and it is existing practice too.

In the C++ testcase most testcases return non-zero to indicate failure. 
  The main reason for this is to avoid having to deal with declaring abort.

Jason

Re: [PATCH] PR c++/PR48035

[Jason Merrill]

On 03/10/2011 11:14 AM, Dodji Seketeli wrote:
> +	  /* If we are initializing a sub-object of
> +	     CURRENT_OBJECT_TYPE [for which a primary base class
> +	     sub-object has already been initialized] then we must NOT
> +	     initialize any sub-object from a virtual base that is a
> +	     direct or indirect primary base of
> +	     CURRENT_OBJECT_TYPE.  */
> +	  if (current_object_type
> +	      && is_virtual_base_of (TREE_TYPE (field), current_object_type)
> +	      && is_primary_base_of (TREE_TYPE (field), current_object_type,
> +				     /*indirect_p=*/true))
> +	    continue;

This seems like the wrong test.  If we're currently initializing a 
subobject, then we don't want to initialize any of our virtual base 
fields unless they are primary to the current type.  We don't need to 
consider the complete object type at all.

I'm also rather nervous about using is_*_base_of tests given that a 
class can have multiple indirect bases of a particular type.  Whether or 
not there is a virtual base T of U isn't important; what is important is 
whether the current field represents a virtual base.

Jason

Re: [PATCH] PR c++/PR48035

[Jakub Jelinek]

On Thu, Mar 10, 2011 at 06:02:50PM -0500, Jason Merrill wrote:
> On 03/10/2011 11:14 AM, Dodji Seketeli wrote:
> >+	  /* If we are initializing a sub-object of
> >+	     CURRENT_OBJECT_TYPE [for which a primary base class
> >+	     sub-object has already been initialized] then we must NOT
> >+	     initialize any sub-object from a virtual base that is a
> >+	     direct or indirect primary base of
> >+	     CURRENT_OBJECT_TYPE.  */
> >+	  if (current_object_type
> >+	      && is_virtual_base_of (TREE_TYPE (field), current_object_type)
> >+	      && is_primary_base_of (TREE_TYPE (field), current_object_type,
> >+				     /*indirect_p=*/true))
> >+	    continue;
> 
> This seems like the wrong test.  If we're currently initializing a
> subobject, then we don't want to initialize any of our virtual base
> fields unless they are primary to the current type.  We don't need
> to consider the complete object type at all.
> 
> I'm also rather nervous about using is_*_base_of tests given that a
> class can have multiple indirect bases of a particular type.
> Whether or not there is a virtual base T of U isn't important; what
> is important is whether the current field represents a virtual base.

Do we need to redo parts of class.c anyway?  From what I understand, the
problematic vtbl pointers are at the end of the base types and DECL_SIZE
is set to the CLASSTYPE_AS_BASE type size, thus those pointers ought to
be at or beyond the containing field's size.
So, can't we simply skip subfields whose bit_position is equal or larger
to containing field's size?  Something like (works on the testcase from this
PR, otherwise untested):

--- gcc/cp/init.c.jj	2011-03-11 08:06:36.000000000 +0100
+++ gcc/cp/init.c	2011-03-11 08:40:29.321401994 +0100
@@ -142,8 +142,9 @@ initialize_vtbl_ptrs (tree addr)
    zero-initialization does not simply mean filling the storage with
    zero bytes.  */
 
-tree
-build_zero_init (tree type, tree nelts, bool static_storage_p)
+static tree
+build_zero_init_1 (tree type, tree nelts, bool static_storage_p,
+		   tree field_size)
 {
   tree init = NULL_TREE;
 
@@ -188,15 +189,32 @@ build_zero_init (tree type, tree nelts, 
 	  if (TREE_CODE (field) != FIELD_DECL)
 	    continue;
 
+	  /* Don't add virtual bases for base classes if they are beyond
+	     the size of the current field, that means it is present
+	     somewhere else in the object.  */
+	  if (field_size)
+	    {
+	      tree bitpos = bit_position (field);
+	      if (TREE_CODE (bitpos) == INTEGER_CST
+		  && !tree_int_cst_lt (bitpos, field_size))
+		continue;
+	    }
+
 	  /* Note that for class types there will be FIELD_DECLs
 	     corresponding to base classes as well.  Thus, iterating
 	     over TYPE_FIELDs will result in correct initialization of
 	     all of the subobjects.  */
 	  if (!static_storage_p || !zero_init_p (TREE_TYPE (field)))
 	    {
-	      tree value = build_zero_init (TREE_TYPE (field),
-					    /*nelts=*/NULL_TREE,
-					    static_storage_p);
+	      tree new_field_size
+		= (DECL_FIELD_IS_BASE (field)
+		   && DECL_SIZE (field)
+		   && TREE_CODE (DECL_SIZE (field)) == INTEGER_CST)
+		  ? DECL_SIZE (field) : NULL_TREE;
+	      tree value = build_zero_init_1 (TREE_TYPE (field),
+					      /*nelts=*/NULL_TREE,
+					      static_storage_p,
+					      new_field_size);
 	      if (value)
 		CONSTRUCTOR_APPEND_ELT(v, field, value);
 	    }
@@ -244,9 +262,9 @@ build_zero_init (tree type, tree nelts, 
 	    ce->index = build2 (RANGE_EXPR, sizetype, size_zero_node,
 				max_index);
 
-	  ce->value = build_zero_init (TREE_TYPE (type),
-				       /*nelts=*/NULL_TREE,
-				       static_storage_p);
+	  ce->value = build_zero_init_1 (TREE_TYPE (type),
+					 /*nelts=*/NULL_TREE,
+					 static_storage_p, NULL_TREE);
 	}
 
       /* Build a constructor to contain the initializations.  */
@@ -264,6 +282,12 @@ build_zero_init (tree type, tree nelts, 
   return init;
 }
 
+tree
+build_zero_init (tree type, tree nelts, bool static_storage_p)
+{
+  return build_zero_init_1 (type, nelts, static_storage_p, NULL_TREE);
+}
+
 /* Return a suitable initializer for value-initializing an object of type
    TYPE, as described in [dcl.init].  */
 


	Jakub

Re: [PATCH] PR c++/PR48035

[Jakub Jelinek]

On Fri, Mar 11, 2011 at 09:01:40AM +0100, Jakub Jelinek wrote:
> Do we need to redo parts of class.c anyway?  From what I understand, the
> problematic vtbl pointers are at the end of the base types and DECL_SIZE
> is set to the CLASSTYPE_AS_BASE type size, thus those pointers ought to
> be at or beyond the containing field's size.
> So, can't we simply skip subfields whose bit_position is equal or larger
> to containing field's size?  Something like (works on the testcase from this
> PR, otherwise untested):

Or, in the simplify_aggr_init case where the created tree is handed
immediately to the gimplifier, there is IMHO no point in initializing
all fields to zero when the gimplifier throws that immediately away again.

So something like following works too.  But I have no idea about what
the C++ FE does with all other build_zero_init calls and whether just
returning empty CONSTRUCTOR would work in those cases too or not.

--- gcc/cp/semantics.c.jj	2011-03-08 11:39:32.516389675 +0100
+++ gcc/cp/semantics.c	2011-03-11 09:27:22.340671253 +0100
@@ -3379,8 +3379,18 @@ simplify_aggr_init_expr (tree *tp)
 
   if (AGGR_INIT_ZERO_FIRST (aggr_init_expr))
     {
-      tree init = build_zero_init (type, NULL_TREE,
-				   /*static_storage_p=*/false);
+      tree init;
+      /* Empty CONSTRUCTOR is the middle-end alternative of zero
+	 initialization, so avoid creating a full CONSTRUCTOR
+	 which will be thrown away immediately.  */
+      if (CLASS_TYPE_P (type) || TREE_CODE (type) == ARRAY_TYPE)
+	{
+	  init = build_constructor (type, NULL);
+	  TREE_CONSTANT (init) = 1;
+	}
+      else
+	init = build_zero_init (type, NULL_TREE,
+				/*static_storage_p=*/false);
       init = build2 (INIT_EXPR, void_type_node, slot, init);
       call_expr = build2 (COMPOUND_EXPR, TREE_TYPE (call_expr),
 			  init, call_expr);

	Jakub

Re: [PATCH] PR c++/PR48035

[Jason Merrill]

On 03/11/2011 03:01 AM, Jakub Jelinek wrote:
> Do we need to redo parts of class.c anyway?  From what I understand, the
> problematic vtbl pointers are at the end of the base types and DECL_SIZE
> is set to the CLASSTYPE_AS_BASE type size, thus those pointers ought to
> be at or beyond the containing field's size.
> So, can't we simply skip subfields whose bit_position is equal or larger
> to containing field's size?  Something like (works on the testcase from this
> PR, otherwise untested):

Sure, that should work.  I had been thinking of stopping when we run out 
of fields in CLASSTYPE_AS_BASE, but your approach seems simpler.

> Or, in the simplify_aggr_init case where the created tree is handed
> immediately to the gimplifier, there is IMHO no point in initializing
> all fields to zero when the gimplifier throws that immediately away again.

Unfortunately, pointers to data members make this not good enough: 
zero-initializing one means setting the bits to -1.  Though I suppose we 
could keep track of whether or not a class has a pointer to data member 
field somewhere (and therefore need to do this the hard way) and if not, 
just use an empty CONSTRUCTOR.

Jason

Re: [PATCH] PR c++/PR48035

[Jakub Jelinek]

On Fri, Mar 11, 2011 at 09:56:59AM -0500, Jason Merrill wrote:
> On 03/11/2011 03:01 AM, Jakub Jelinek wrote:
> >Do we need to redo parts of class.c anyway?  From what I understand, the
> >problematic vtbl pointers are at the end of the base types and DECL_SIZE
> >is set to the CLASSTYPE_AS_BASE type size, thus those pointers ought to
> >be at or beyond the containing field's size.
> >So, can't we simply skip subfields whose bit_position is equal or larger
> >to containing field's size?  Something like (works on the testcase from this
> >PR, otherwise untested):
> 
> Sure, that should work.  I had been thinking of stopping when we run
> out of fields in CLASSTYPE_AS_BASE, but your approach seems simpler.

It worked, here is what I've bootstrapped/regtested on x86_64-linux and
i686-linux.  Is that ok then?

> >Or, in the simplify_aggr_init case where the created tree is handed
> >immediately to the gimplifier, there is IMHO no point in initializing
> >all fields to zero when the gimplifier throws that immediately away again.
> 
> Unfortunately, pointers to data members make this not good enough:
> zero-initializing one means setting the bits to -1.  Though I
> suppose we could keep track of whether or not a class has a pointer
> to data member field somewhere (and therefore need to do this the
> hard way) and if not, just use an empty CONSTRUCTOR.

Np, we'd need to call a recursive function then to tell us if there
is a pointer to data member then.

2011-03-11  Jakub Jelinek  <jakub@redhat.com>

	PR c++/48035
	* init.c (build_zero_init_1): Extracted from build_zero_init.
	Add FIELD_SIZE argument, if non-NULL and field bit_position
	as not smaller than that, don't add that field's initializer.
	Pass DECL_SIZE as last argument to build_zero_init_1
	for DECL_FIELD_IS_BASE fields.
	(build_zero_init): Use build_zero_init_1.

	* g++.dg/inherit/virtual8.C: New test.

--- gcc/cp/init.c.jj	2011-03-11 08:06:36.000000000 +0100
+++ gcc/cp/init.c	2011-03-11 08:40:29.321401994 +0100
@@ -140,10 +140,13 @@ initialize_vtbl_ptrs (tree addr)
    is the number of elements in the array.  If STATIC_STORAGE_P is
    TRUE, initializers are only generated for entities for which
    zero-initialization does not simply mean filling the storage with
-   zero bytes.  */
+   zero bytes.  FIELD_SIZE, if non-NULL, is the bit size of the field,
+   subfields with bit positions at or above that bit size shouldn't
+   be added.  */
 
-tree
-build_zero_init (tree type, tree nelts, bool static_storage_p)
+static tree
+build_zero_init_1 (tree type, tree nelts, bool static_storage_p,
+		   tree field_size)
 {
   tree init = NULL_TREE;
 
@@ -188,15 +191,32 @@ build_zero_init (tree type, tree nelts, 
 	  if (TREE_CODE (field) != FIELD_DECL)
 	    continue;
 
+	  /* Don't add virtual bases for base classes if they are beyond
+	     the size of the current field, that means it is present
+	     somewhere else in the object.  */
+	  if (field_size)
+	    {
+	      tree bitpos = bit_position (field);
+	      if (TREE_CODE (bitpos) == INTEGER_CST
+		  && !tree_int_cst_lt (bitpos, field_size))
+		continue;
+	    }
+
 	  /* Note that for class types there will be FIELD_DECLs
 	     corresponding to base classes as well.  Thus, iterating
 	     over TYPE_FIELDs will result in correct initialization of
 	     all of the subobjects.  */
 	  if (!static_storage_p || !zero_init_p (TREE_TYPE (field)))
 	    {
-	      tree value = build_zero_init (TREE_TYPE (field),
-					    /*nelts=*/NULL_TREE,
-					    static_storage_p);
+	      tree new_field_size
+		= (DECL_FIELD_IS_BASE (field)
+		   && DECL_SIZE (field)
+		   && TREE_CODE (DECL_SIZE (field)) == INTEGER_CST)
+		  ? DECL_SIZE (field) : NULL_TREE;
+	      tree value = build_zero_init_1 (TREE_TYPE (field),
+					      /*nelts=*/NULL_TREE,
+					      static_storage_p,
+					      new_field_size);
 	      if (value)
 		CONSTRUCTOR_APPEND_ELT(v, field, value);
 	    }
@@ -244,9 +264,9 @@ build_zero_init (tree type, tree nelts, 
 	    ce->index = build2 (RANGE_EXPR, sizetype, size_zero_node,
 				max_index);
 
-	  ce->value = build_zero_init (TREE_TYPE (type),
-				       /*nelts=*/NULL_TREE,
-				       static_storage_p);
+	  ce->value = build_zero_init_1 (TREE_TYPE (type),
+					 /*nelts=*/NULL_TREE,
+					 static_storage_p, NULL_TREE);
 	}
 
       /* Build a constructor to contain the initializations.  */
@@ -264,6 +284,24 @@ build_zero_init (tree type, tree nelts, 
   return init;
 }
 
+/* Return an expression for the zero-initialization of an object with
+   type T.  This expression will either be a constant (in the case
+   that T is a scalar), or a CONSTRUCTOR (in the case that T is an
+   aggregate), or NULL (in the case that T does not require
+   initialization).  In either case, the value can be used as
+   DECL_INITIAL for a decl of the indicated TYPE; it is a valid static
+   initializer. If NELTS is non-NULL, and TYPE is an ARRAY_TYPE, NELTS
+   is the number of elements in the array.  If STATIC_STORAGE_P is
+   TRUE, initializers are only generated for entities for which
+   zero-initialization does not simply mean filling the storage with
+   zero bytes.  */
+
+tree
+build_zero_init (tree type, tree nelts, bool static_storage_p)
+{
+  return build_zero_init_1 (type, nelts, static_storage_p, NULL_TREE);
+}
+
 /* Return a suitable initializer for value-initializing an object of type
    TYPE, as described in [dcl.init].  */
 
--- gcc/testsuite/g++.dg/inherit/virtual8.C
+++ gcc/testsuite/g++.dg/inherit/virtual8.C
@@ -0,0 +1,48 @@
+// PR c++/48035
+// { dg-do run }
+
+#include <new>
+#include <cstring>
+#include <cstdlib>
+
+struct A
+{
+  virtual void foo (void) {}
+  virtual ~A () {}
+};
+
+struct B : public A
+{
+  virtual ~B () {}
+};
+
+struct C
+{
+  virtual ~C () {}
+  int c;
+};
+
+struct D : public virtual B, public C
+{
+  virtual ~D () {}
+};
+
+struct E : public virtual D
+{
+  virtual ~E () {}
+};
+
+int
+main ()
+{
+  char *v = new char[sizeof (E) + 16];
+  memset (v, 0x55, sizeof (E) + 16);
+  E *e = new (v) E ();
+  e->~E ();
+
+  for (unsigned i = sizeof (E); i < sizeof (E) + 16; ++i)
+    if (v[i] != 0x55)
+      abort ();
+
+  delete[] v;
+}


	Jakub

Re: [PATCH] PR c++/PR48035

[Dodji Seketeli]

Jason Merrill <jason@redhat.com> writes:

> If we're currently initializing a subobject, then we don't want to
> initialize any of our virtual base fields unless they are primary to
> the current type.

Indeed.  I think I understand better now.

> I'm also rather nervous about using is_*_base_of tests given that a
> class can have multiple indirect bases of a particular type.  Whether
> or not there is a virtual base T of U isn't important; what is
> important is whether the current field represents a virtual base.

I have updated is_virtual_base_of to make it test if the current field
represents a virtual base.  Is there a simpler way to detect that?

> In the C++ testcase most testcases return non-zero to indicate
> failure. The main reason for this is to avoid having to deal with
> declaring abort.

Ah okay.  I did notice that C++ tests were using the non-zero return
convention but I didn't know why.  In the patch below I took the liberty
to use abort() nonetheless because as we need to include <new> for the
placement new operator, I thought just adding another header would be
understandable.

Jakub Jelinek <jakub@redhat.com> writes:

> Or, in the simplify_aggr_init case where the created tree is handed
> immediately to the gimplifier, there is IMHO no point in initializing
> all fields to zero when the gimplifier throws that immediately away
> again.

From what I understand, zero-initializing doesn't necessarily mean
setting all fields to zero, because, e.g, for member pointers fields the
ABI states that a NULL pointer is represented as -1.

So this is the patch I am currently testing.

-- 
		Dodji

From 6a757e998cb6b09883ec366c8c8939a70a215600 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Fri, 11 Mar 2011 15:24:00 +0100
Subject: [PATCH] [PATCH] PR c++/PR48035

gcc/cp/

	* cp-tree.h (is_virtual_base_of): Declare new function.
	* class.c (is_virtual_base_of): Define it.
	* init.c (build_zero_init_1): Extract from from build_zero_init.
	Don't initialize non-primary virtual bases of sub-objects.
	(build_zero_init_1): Use build_zero_init_1.

gcc/testsuite/

	* g++.dg/inherit/virtual8.C: New test.
---
 gcc/cp/class.c                          |   21 ++++++++++
 gcc/cp/cp-tree.h                        |    1 +
 gcc/cp/init.c                           |   66 ++++++++++++++++++++++---------
 gcc/testsuite/g++.dg/inherit/virtual8.C |   52 ++++++++++++++++++++++++
 4 files changed, 121 insertions(+), 19 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/inherit/virtual8.C

diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 1325260..4c6c9d5 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -7008,6 +7008,27 @@ get_primary_binfo (tree binfo)
   return copied_binfo (primary_base, binfo);
 }
 
+/* Returns TRUE if BASE is a direct virtual base of TYPE, FALSE
+   otherwise.  */
+
+bool
+is_virtual_base_of (tree base, tree type)
+{
+  tree binfo;
+
+  for (binfo = TREE_CHAIN (TYPE_BINFO (type));
+       binfo;
+       binfo = TREE_CHAIN (binfo))
+    {
+      if (!BINFO_VIRTUAL_P (binfo))
+	continue;
+
+      if (same_type_p (BINFO_TYPE (binfo), base))
+	return binfo;
+    }
+  return NULL_TREE;
+}
+
 /* If INDENTED_P is zero, indent to INDENT. Return nonzero.  */
 
 static int
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 4b49046..558c38b 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -4727,6 +4727,7 @@ extern void fixup_attribute_variants		(tree);
 extern tree* decl_cloned_function_p		(const_tree, bool);
 extern void clone_function_decl			(tree, int);
 extern void adjust_clone_args			(tree);
+extern bool is_virtual_base_of                  (tree, tree);
 
 /* in cvt.c */
 extern tree convert_to_reference		(tree, tree, int, int, tree);
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 56f66fa..440db79 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -130,20 +130,17 @@ initialize_vtbl_ptrs (tree addr)
   dfs_walk_once (TYPE_BINFO (type), dfs_initialize_vtbl_ptrs, NULL, list);
 }
 
-/* Return an expression for the zero-initialization of an object with
-   type T.  This expression will either be a constant (in the case
-   that T is a scalar), or a CONSTRUCTOR (in the case that T is an
-   aggregate), or NULL (in the case that T does not require
-   initialization).  In either case, the value can be used as
-   DECL_INITIAL for a decl of the indicated TYPE; it is a valid static
-   initializer. If NELTS is non-NULL, and TYPE is an ARRAY_TYPE, NELTS
-   is the number of elements in the array.  If STATIC_STORAGE_P is
-   TRUE, initializers are only generated for entities for which
-   zero-initialization does not simply mean filling the storage with
-   zero bytes.  */
+/* A subroutine of build_zero_init.
+   
+   The parameters are the same as for build_zero_init.  If
+   IN_SUBOBJECT is TRUE, that means we are currently initializing a
+   subobject.  In that case, we don't initialize virtual bases unless
+   the virtual base is a primary base of TYPE.  */
 
-tree
-build_zero_init (tree type, tree nelts, bool static_storage_p)
+static tree
+build_zero_init_1 (tree type, tree nelts,
+		   bool static_storage_p,
+		   bool in_subjobject)
 {
   tree init = NULL_TREE;
 
@@ -188,15 +185,25 @@ build_zero_init (tree type, tree nelts, bool static_storage_p)
 	  if (TREE_CODE (field) != FIELD_DECL)
 	    continue;
 
+	  /* If we are initializing a subobject then skip non-primary
+	     virtual bases.  */
+	  if (in_subjobject
+	      && is_virtual_base_of (TREE_TYPE (field), type)
+	      && CLASSTYPE_PRIMARY_BINFO (type)
+	      && !same_type_p (BINFO_TYPE (CLASSTYPE_PRIMARY_BINFO (type)),
+			       TREE_TYPE (field)))
+	    continue;
+
 	  /* Note that for class types there will be FIELD_DECLs
 	     corresponding to base classes as well.  Thus, iterating
 	     over TYPE_FIELDs will result in correct initialization of
 	     all of the subobjects.  */
 	  if (!static_storage_p || !zero_init_p (TREE_TYPE (field)))
 	    {
-	      tree value = build_zero_init (TREE_TYPE (field),
-					    /*nelts=*/NULL_TREE,
-					    static_storage_p);
+	      tree value = build_zero_init_1 (TREE_TYPE (field),
+					      /*nelts=*/NULL_TREE,
+					      static_storage_p,
+					      /*in_subobject=*/true);
 	      if (value)
 		CONSTRUCTOR_APPEND_ELT(v, field, value);
 	    }
@@ -244,9 +251,10 @@ build_zero_init (tree type, tree nelts, bool static_storage_p)
 	    ce->index = build2 (RANGE_EXPR, sizetype, size_zero_node,
 				max_index);
 
-	  ce->value = build_zero_init (TREE_TYPE (type),
-				       /*nelts=*/NULL_TREE,
-				       static_storage_p);
+	  ce->value = build_zero_init_1 (TREE_TYPE (type),
+					 /*nelts=*/NULL_TREE,
+					 static_storage_p,
+					 false);
 	}
 
       /* Build a constructor to contain the initializations.  */
@@ -264,6 +272,26 @@ build_zero_init (tree type, tree nelts, bool static_storage_p)
   return init;
 }
 
+/* Return an expression for the zero-initialization of an object with
+   type T.  This expression will either be a constant (in the case
+   that T is a scalar), or a CONSTRUCTOR (in the case that T is an
+   aggregate), or NULL (in the case that T does not require
+   initialization).  In either case, the value can be used as
+   DECL_INITIAL for a decl of the indicated TYPE; it is a valid static
+   initializer. If NELTS is non-NULL, and TYPE is an ARRAY_TYPE, NELTS
+   is the number of elements in the array.  If STATIC_STORAGE_P is
+   TRUE, initializers are only generated for entities for which
+   zero-initialization does not simply mean filling the storage with
+   zero bytes.  */
+
+tree
+build_zero_init (tree type, tree nelts, bool static_storage_p)
+{
+  return build_zero_init_1 (type, nelts,
+			    static_storage_p,
+			    /*in_subobject=*/false);
+}
+
 /* Return a suitable initializer for value-initializing an object of type
    TYPE, as described in [dcl.init].  */
 
diff --git a/gcc/testsuite/g++.dg/inherit/virtual8.C b/gcc/testsuite/g++.dg/inherit/virtual8.C
new file mode 100644
index 0000000..62bdc3b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/inherit/virtual8.C
@@ -0,0 +1,52 @@
+// Origin PR c++/PR48035
+// { dg-do run }
+
+#include <new>
+#include <cstring>
+#include <cstdlib>
+
+struct A
+{
+  virtual void foo (void) {}
+  virtual ~A () {}
+};
+
+struct B : public A
+{
+  virtual ~B () {}
+};
+
+struct C
+{
+  virtual ~C ()
+  {
+  }
+  int c;
+};
+
+struct D : public virtual B, public C
+{
+  virtual ~D () {}
+};
+
+struct E : public virtual D
+{
+  virtual ~E ()
+  {
+  }
+};
+
+int
+main ()
+{
+  char *v = new char[sizeof (E) + 16];
+  memset (v, 0x55, sizeof (E) + 16);
+  E *e = new (v) E ();
+  e->~E ();
+
+  for (unsigned i = sizeof (E); i < sizeof (E) + 16; ++i)
+    if (v[i] != 0x55)
+      abort();
+  delete[] v;
+}
+
-- 
1.7.3.4

Re: [PATCH] PR c++/PR48035

[Jason Merrill]

On 03/11/2011 10:08 AM, Jakub Jelinek wrote:
> It worked, here is what I've bootstrapped/regtested on x86_64-linux and
> i686-linux.  Is that ok then?

OK.

Jason

Re: [PATCH] PR c++/PR48035

[Jason Merrill]

On 03/11/2011 10:15 AM, Dodji Seketeli wrote:
> I have updated is_virtual_base_of to make it test if the current field
> represents a virtual base.  Is there a simpler way to detect that?

Yeah, let's go with Jakub's patch.

Jason