From patchwork Fri Mar 11 15:15:33 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 86431 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 3D01FB6FBB for ; Sat, 12 Mar 2011 02:15:52 +1100 (EST) Received: (qmail 1762 invoked by alias); 11 Mar 2011 15:15:51 -0000 Received: (qmail 1753 invoked by uid 22791); 11 Mar 2011 15:15:49 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from seketeli.net (HELO ms.seketeli.net) (91.121.166.71) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 11 Mar 2011 15:15:38 +0000 Received: from localhost.localdomain (torimasen.com [82.237.12.13]) by ms.seketeli.net (Postfix) with ESMTP id D48E1EA040; Fri, 11 Mar 2011 16:09:06 +0100 (CET) Received: by localhost.localdomain (Postfix, from userid 500) id 7736F8E604B; Fri, 11 Mar 2011 16:15:33 +0100 (CET) From: Dodji Seketeli To: Jason Merrill Cc: GCC Patches , Jakub Jelinek Subject: Re: [PATCH] PR c++/PR48035 References: <4D79589A.1050006@redhat.com> X-URL: http://www.redhat.com Mail-Followup-To: Jason Merrill , GCC Patches , Jakub Jelinek Date: Fri, 11 Mar 2011 16:15:33 +0100 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Jason Merrill 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 for the placement new operator, I thought just adding another header would be understandable. Jakub Jelinek 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. 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 +#include +#include + +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; +} +