From patchwork Sat Jun 11 16:06:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Carlini X-Patchwork-Id: 634081 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rRkSl6ZGNz9spN for ; Sun, 12 Jun 2016 02:07:01 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=yp3e55SU; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=ghHOs9s8sWmDfrE+W B/1a/xACXVm5eg5K+YZeXSMWk4ShgxKHz4uQyaDLDUiXhS0Z026qNsiryyqrBFam eImFXQUiYxEExH10N6cKdMmesMnCR7kIbgA3vSA4Ued1mWyAN4EmdZrU1rFoDmWh I7IXdAYXnleOHtMHmDCark8oAw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=EpiLbiS58JVTdsnyfAidraY gYWo=; b=yp3e55SUBTdOKWBv1WZzKM1TgNzzNEldCDAq2VYNwSmSuTGt2CFpclU 90JWnsTeWlnlHS4UtJW4q1/B1OQDxa2cearXtLOMbP7Jh8Zn2/h6rxKe2oS7Kkw1 JreGazxRFzbRnJbCpQGuLCjyxWsYfcvP/eyu/9gx7jAMeuduT96w= Received: (qmail 109940 invoked by alias); 11 Jun 2016 16:06:54 -0000 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 Received: (qmail 109930 invoked by uid 89); 11 Jun 2016 16:06:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=BAYES_00, KAM_ASCII_DIVIDERS, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=UD:call.c, call.c, callc, lighter X-HELO: aserp1040.oracle.com Received: from aserp1040.oracle.com (HELO aserp1040.oracle.com) (141.146.126.69) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sat, 11 Jun 2016 16:06:43 +0000 Received: from aserv0022.oracle.com (aserv0022.oracle.com [141.146.126.234]) by aserp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u5BG6eYs020136 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 11 Jun 2016 16:06:41 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserv0022.oracle.com (8.13.8/8.13.8) with ESMTP id u5BG6dOZ005976 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 11 Jun 2016 16:06:40 GMT Received: from abhmp0018.oracle.com (abhmp0018.oracle.com [141.146.116.24]) by userv0121.oracle.com (8.13.8/8.13.8) with ESMTP id u5BG6cpW020658; Sat, 11 Jun 2016 16:06:39 GMT Received: from [192.168.1.4] (/79.23.238.165) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Sat, 11 Jun 2016 09:06:38 -0700 Subject: [C++ Patch] PR 70202: avoid creating incomplete types (was: "[C++ RFC / Patch] Again about PR 70202") To: Jason Merrill References: <5759A0FB.2050703@oracle.com> <5759FBC7.5080503@oracle.com> <575A1B49.1040702@oracle.com> <575B2372.6020806@oracle.com> <575B2431.4030904@oracle.com> <575B277C.7020806@oracle.com> Cc: "gcc-patches@gcc.gnu.org" From: Paolo Carlini Message-ID: <575C370C.30307@oracle.com> Date: Sat, 11 Jun 2016 18:06:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.0 MIME-Version: 1.0 In-Reply-To: <575B277C.7020806@oracle.com> X-IsSubscribed: yes Hi again, hi Jason, I can't help further investigating this issue and I have a completely different proposal which pursues a different hint of Jason: why the TREE_TYPE of such DECL is error_mark_node in the first place? As we understand now that is just something that start_decl_1 does when it sees an incomplete type. Then why we have that kind of type? The answer is this change, back in 2006 (!), for c++/27952: https://gcc.gnu.org/ml/gcc-patches/2006-10/msg00862.html which changed the loop in xref_basetypes to early return false after error. That implies that the caller skips the body, the type remains incomplete. That has non trivial implications: we avoid ICEs but we also end up with redundant error messages about incomplete types during error recovery. And the inconsistencies shown by the present issue. Thus my idea: let's fix that old bug in a different, lighter way and revert the bulk of the old fix, just disregard and continue on the erroneous duplicate bases in the main xref_basetpes loop. I think at some point Jason hinted at that as a general error recovery strategy: we have been doing exactly that until 2006!! In a sense we are also back to my original idea of not zeroing by hand the type in cp_parser_class_head. Anyway, things appear to work fine: no regressions, more terse diagnostic, see inherit/crash6.C but also the tweaked g++.dg/inherit/virtual1.C (now the diagnostic is *very* similar to that produced by clang, by the way, eh, eh) and the new g++.dg/inherit/union2.C, no redundant error messages on the declaration of u during error recovery. The remaining issue to be discussed is what to do about that old bug, c++/27952, a crash in decay_conversion during error recovery, when vtt is found null. I propose to simply check for it in build_special_member_call, avoid passing an unusable null argument to decay_conversion and return back an error_mark_node. We could also do gcc_assert (seen_error()) before returning error_mark_node? Ah, there is also another detail: around line 12930 of the decl.c diff, I'm also reverting my own 2010 changes to fix c++/30298 (r159637) which really had to do with the 2006 change as well: we can have the gcc_assert back and still handle correctly inherit/crash1 and crash2 which I added back then. This is nice, I think. In conclusion, IMHO this approach is way better than all the other I tried so far, modulo the possible need of additional / better error recovery measures in build_special_member_call, etc. Thanks, Paolo. //////////////////////////// Index: cp/call.c =================================================================== --- cp/call.c (revision 237318) +++ cp/call.c (working copy) @@ -8022,6 +8022,8 @@ build_special_member_call (tree instance, tree nam or destructor, then we fetch the VTT directly. Otherwise, we look it up using the VTT we were given. */ vtt = DECL_CHAIN (CLASSTYPE_VTABLES (current_class_type)); + if (!vtt) + return error_mark_node; vtt = decay_conversion (vtt, complain); if (vtt == error_mark_node) return error_mark_node; Index: cp/cp-tree.h =================================================================== --- cp/cp-tree.h (revision 237318) +++ cp/cp-tree.h (working copy) @@ -5771,7 +5771,7 @@ extern int grok_ctor_properties (const_tree, con extern bool grok_op_properties (tree, bool); extern tree xref_tag (enum tag_types, tree, tag_scope, bool); extern tree xref_tag_from_type (tree, tree, tag_scope); -extern bool xref_basetypes (tree, tree); +extern void xref_basetypes (tree, tree); extern tree start_enum (tree, tree, tree, tree, bool, bool *); extern void finish_enum_value_list (tree); extern void finish_enum (tree); Index: cp/decl.c =================================================================== --- cp/decl.c (revision 237318) +++ cp/decl.c (working copy) @@ -12871,12 +12871,9 @@ xref_tag_from_type (tree old, tree id, tag_scope s /* Create the binfo hierarchy for REF with (possibly NULL) base list BASE_LIST. For each element on BASE_LIST the TREE_PURPOSE is an access_* node, and the TREE_VALUE is the type of the base-class. - Non-NULL TREE_TYPE indicates virtual inheritance. - - Returns true if the binfo hierarchy was successfully created, - false if an error was detected. */ + Non-NULL TREE_TYPE indicates virtual inheritance. */ -bool +void xref_basetypes (tree ref, tree base_list) { tree *basep; @@ -12889,7 +12886,7 @@ xref_basetypes (tree ref, tree base_list) tree igo_prev; /* Track Inheritance Graph Order. */ if (ref == error_mark_node) - return false; + return; /* The base of a derived class is private by default, all others are public. */ @@ -12933,11 +12930,7 @@ xref_basetypes (tree ref, tree base_list) /* The binfo slot should be empty, unless this is an (ill-formed) redefinition. */ - if (TYPE_BINFO (ref) && !TYPE_SIZE (ref)) - { - error ("redefinition of %q#T", ref); - return false; - } + gcc_assert (!TYPE_BINFO (ref) || TYPE_SIZE (ref)); gcc_assert (TYPE_MAIN_VARIANT (ref) == ref); @@ -12957,10 +12950,7 @@ xref_basetypes (tree ref, tree base_list) CLASSTYPE_NON_AGGREGATE (ref) = 1; if (TREE_CODE (ref) == UNION_TYPE) - { - error ("derived union %qT invalid", ref); - return false; - } + error ("derived union %qT invalid", ref); } if (max_bases > 1) @@ -12968,7 +12958,7 @@ xref_basetypes (tree ref, tree base_list) if (TYPE_FOR_JAVA (ref)) { error ("Java class %qT cannot have multiple bases", ref); - return false; + return; } else warning (OPT_Wmultiple_inheritance, @@ -12982,7 +12972,7 @@ xref_basetypes (tree ref, tree base_list) if (TYPE_FOR_JAVA (ref)) { error ("Java class %qT cannot have virtual bases", ref); - return false; + return; } else if (max_dvbases) warning (OPT_Wvirtual_inheritance, @@ -13006,7 +12996,7 @@ xref_basetypes (tree ref, tree base_list) { error ("base type %qT fails to be a struct or class type", basetype); - return false; + continue; } if (TYPE_FOR_JAVA (basetype) && (current_lang_depth () == 0)) @@ -13040,7 +13030,7 @@ xref_basetypes (tree ref, tree base_list) error ("recursive type %qT undefined", basetype); else error ("duplicate base type %qT invalid", basetype); - return false; + continue; } if (PACK_EXPANSION_P (TREE_VALUE (base_list))) @@ -13088,8 +13078,6 @@ xref_basetypes (tree ref, tree base_list) else break; } - - return true; } Index: cp/parser.c =================================================================== --- cp/parser.c (revision 237318) +++ cp/parser.c (working copy) @@ -22050,9 +22050,8 @@ cp_parser_class_head (cp_parser* parser, /* If we're really defining a class, process the base classes. If they're invalid, fail. */ - if (type && cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE) - && !xref_basetypes (type, bases)) - type = NULL_TREE; + if (type && cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE)) + xref_basetypes (type, bases); done: /* Leave the scope given by the nested-name-specifier. We will Index: testsuite/g++.dg/inherit/crash6.C =================================================================== --- testsuite/g++.dg/inherit/crash6.C (revision 0) +++ testsuite/g++.dg/inherit/crash6.C (working copy) @@ -0,0 +1,10 @@ +// PR c++/70202 + +class A +{ + virtual void foo () { } +}; +class B : public A, A { }; // { dg-error "duplicate base type" } + +B b1, &b2 = b1; +A a = b2; Index: testsuite/g++.dg/inherit/union2.C =================================================================== --- testsuite/g++.dg/inherit/union2.C (revision 0) +++ testsuite/g++.dg/inherit/union2.C (working copy) @@ -0,0 +1,3 @@ +struct A { }; +union U : A { }; // { dg-error "derived union 'U' invalid" } +U u; Index: testsuite/g++.dg/inherit/virtual1.C =================================================================== --- testsuite/g++.dg/inherit/virtual1.C (revision 237318) +++ testsuite/g++.dg/inherit/virtual1.C (working copy) @@ -5,8 +5,8 @@ struct A virtual ~A() {} }; -struct B : A, virtual A {}; // { dg-error "duplicate base|forward declaration" } +struct B : A, virtual A {}; // { dg-error "duplicate base" } -struct C : A, B {}; // { dg-error "duplicate base|invalid use" } +struct C : A, B {}; // { dg-message "direct base 'A' inaccessible" } -C c; // { dg-error "aggregate" } +C c;