diff mbox

PR c++/PR48035

Message ID m3r5afuf57.fsf@redhat.com
State New
Headers show

Commit Message

Dodji Seketeli March 10, 2011, 4:14 p.m. UTC 
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.

Comments

Jakub Jelinek March 10, 2011, 6:22 p.m. UTC | #1 
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
Mike Stump March 10, 2011, 8:31 p.m. UTC | #2 
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.
Jakub Jelinek March 10, 2011, 8:45 p.m. UTC | #3 
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
Dodji Seketeli March 10, 2011, 8:52 p.m. UTC | #4 
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.
Jason Merrill March 10, 2011, 10:50 p.m. UTC | #5 
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
Jason Merrill March 10, 2011, 11:02 p.m. UTC | #6 
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
diff mbox

Patch

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