* [C++ RFC / Patch] Again about PR 70202 @ 2016-06-09 17:02 Paolo Carlini 2016-06-09 17:06 ` Jakub Jelinek 2016-06-09 21:39 ` Jason Merrill 0 siblings, 2 replies; 14+ messages in thread From: Paolo Carlini @ 2016-06-09 17:02 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill Hi again, thus today I had to revert my first try at resolving this error recovery issue because it caused c++/71465. In retrospect it was a bit heavy handed anyway, the zeroing of the type served us well for many years... Today I tried to investigate the issue a bit more. I remind you that we are crashing on the gcc_unreachable () at the end of build_simple_base_path while, during error recovery, we are processing the initialization of the final 'a' in: class A { virtual void foo () { } }; class B : public A, A { }; // { dg-error "duplicate base type" } B b1, &b2 = b1; A a = b2; Now the first comment I have about this, about which I'd like to have some feedback, is that the gcc_unreachable () at the end of build_simple_base_path without a return seems a bit weird to me, because the function is *supposed to return a tree*, not void. I guess some front-ends would even warn on this?!? Anyway, if we change: /* */ gcc_unreachable (); } to: /* */ gcc_assert (seen_error ()); return error_mark_node; } we get something which, by and large is equally safe in terms of miscompilations and helps a lot error recovery, ie, we would not have c++/70202. It's also true however that we would not make much progress in trying to understand if we could solve the specific problem at issue in a more insightful way. Then my next observation is the following: if we change the testcase to: class A { virtual void foo () { } }; class B : public A, A { }; // { dg-error "duplicate base type" } B b1, &b2 = b1; A a = b1; thus everything is the same but at the end we have 'b1', not 'b2', we also don't crash. We are saved not by something particularly smart ;) but by this check at the beginning of ocp_convert: if (error_operand_p (e) || type == error_mark_node) return error_mark_node; that is, the TREE_TYPE of 'e', the VAR_DECL for 'b1', is error_mark_node and we return early the error_mark_node, we don't reach the problematic build_base_path. With 'b2' we have a more complicated REFERENCE_REF which we don't catch, but otherwise everything is the same, we reach build_base_path and we crash on the gcc_unreachable. In practice, if I add at the beginning of ocp_convert: + if (REFERENCE_REF_P (expr) + && VAR_P (TREE_OPERAND (expr, 0)) + && DECL_INITIAL (TREE_OPERAND (expr, 0)) == error_mark_node) + return error_mark_node; it also passes testing. Then the final observation is that we also don't crash if the type of 'A' is simpler, thus we don't have the declaration of virtual void foo () { } Then I gather that when the type is "complex" enough we are not cleaning things up completely enough when we encounter it as a duplicated base. Maybe as optimal error recovery when we see a duplicated base we should clean up everything having to do with bases end up with something equivalent to an incomplete class B; or something like that?!? I don't know what we want to try short / medium /long term... Thanks! Paolo. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-09 17:02 [C++ RFC / Patch] Again about PR 70202 Paolo Carlini @ 2016-06-09 17:06 ` Jakub Jelinek 2016-06-09 21:39 ` Jason Merrill 1 sibling, 0 replies; 14+ messages in thread From: Jakub Jelinek @ 2016-06-09 17:06 UTC (permalink / raw) To: Paolo Carlini; +Cc: gcc-patches, Jason Merrill On Thu, Jun 09, 2016 at 07:01:47PM +0200, Paolo Carlini wrote: > Now the first comment I have about this, about which I'd like to have some > feedback, is that the gcc_unreachable () at the end of > build_simple_base_path without a return seems a bit weird to me, because the > function is *supposed to return a tree*, not void. I guess some front-ends > would even warn on this?!? Anyway, if we change: > > > /* */ > > gcc_unreachable (); > } I'll just comment on this. gcc_unreachable () is a noreturn function, so this is perfectly fine IMHO. Jakub ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-09 17:02 [C++ RFC / Patch] Again about PR 70202 Paolo Carlini 2016-06-09 17:06 ` Jakub Jelinek @ 2016-06-09 21:39 ` Jason Merrill 2016-06-09 23:29 ` Paolo Carlini 1 sibling, 1 reply; 14+ messages in thread From: Jason Merrill @ 2016-06-09 21:39 UTC (permalink / raw) To: Paolo Carlini; +Cc: gcc-patches I would think that when we see a duplicated base we should just drop the duplicates and continue. If the type of b1 is error_mark_node, why isn't the type of b2 also error_mark_node? Jason On Thu, Jun 9, 2016 at 1:01 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi again, > > thus today I had to revert my first try at resolving this error recovery > issue because it caused c++/71465. In retrospect it was a bit heavy handed > anyway, the zeroing of the type served us well for many years... > > Today I tried to investigate the issue a bit more. I remind you that we are > crashing on the gcc_unreachable () at the end of build_simple_base_path > while, during error recovery, we are processing the initialization of the > final 'a' in: > > class A > { > virtual void foo () { } > }; > class B : public A, A { }; // { dg-error "duplicate base type" } > > B b1, &b2 = b1; > A a = b2; > > Now the first comment I have about this, about which I'd like to have some > feedback, is that the gcc_unreachable () at the end of > build_simple_base_path without a return seems a bit weird to me, because the > function is *supposed to return a tree*, not void. I guess some front-ends > would even warn on this?!? Anyway, if we change: > > > /* */ > > gcc_unreachable (); > } > > to: > > /* */ > > gcc_assert (seen_error ()); > return error_mark_node; > } > > we get something which, by and large is equally safe in terms of > miscompilations and helps a lot error recovery, ie, we would not have > c++/70202. It's also true however that we would not make much progress in > trying to understand if we could solve the specific problem at issue in a > more insightful way. > > Then my next observation is the following: if we change the testcase to: > > class A > { > virtual void foo () { } > }; > class B : public A, A { }; // { dg-error "duplicate base type" } > > B b1, &b2 = b1; > A a = b1; > > thus everything is the same but at the end we have 'b1', not 'b2', we also > don't crash. We are saved not by something particularly smart ;) but by this > check at the beginning of ocp_convert: > > if (error_operand_p (e) || type == error_mark_node) > return error_mark_node; > > that is, the TREE_TYPE of 'e', the VAR_DECL for 'b1', is error_mark_node and > we return early the error_mark_node, we don't reach the problematic > build_base_path. With 'b2' we have a more complicated REFERENCE_REF which we > don't catch, but otherwise everything is the same, we reach build_base_path > and we crash on the gcc_unreachable. In practice, if I add at the beginning > of ocp_convert: > > + if (REFERENCE_REF_P (expr) > + && VAR_P (TREE_OPERAND (expr, 0)) > + && DECL_INITIAL (TREE_OPERAND (expr, 0)) == error_mark_node) > + return error_mark_node; > > it also passes testing. Then the final observation is that we also don't > crash if the type of 'A' is simpler, thus we don't have the declaration of > > virtual void foo () { } > > Then I gather that when the type is "complex" enough we are not cleaning > things up completely enough when we encounter it as a duplicated base. Maybe > as optimal error recovery when we see a duplicated base we should clean up > everything having to do with bases end up with something equivalent to an > incomplete class B; or something like that?!? I don't know what we want to > try short / medium /long term... > > Thanks! > Paolo. > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-09 21:39 ` Jason Merrill @ 2016-06-09 23:29 ` Paolo Carlini 2016-06-10 1:43 ` Paolo Carlini 0 siblings, 1 reply; 14+ messages in thread From: Paolo Carlini @ 2016-06-09 23:29 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 712 bytes --] Hi, On 09/06/2016 23:38, Jason Merrill wrote: > I would think that when we see a duplicated base we should just drop > the duplicates and continue. > > If the type of b1 is error_mark_node, why isn't the type of b2 also > error_mark_node? Thanks Jason. Normally check_initializer massages the decl basing on the init and so to speak transforms an error_mark_node as TREE_TYPE of the init into an error_mark_node as DECL_INITIAL of a reference. Thus we don't see error_mark_node as TREE_TYPE of b2. Now if I do something as trivial as the attached the testcase passes. Next step, analyze the huge breakage caused by that change, I don't dare running the testsuite ;) Thanks, Paolo. /////////////////// [-- Attachment #2: p --] [-- Type: text/plain, Size: 533 bytes --] Index: decl.c =================================================================== --- decl.c (revision 237280) +++ decl.c (working copy) @@ -6002,6 +6002,12 @@ check_initializer (tree decl, tree init, int flags tree init_code = NULL; tree core_type; + if (TREE_TYPE (init) == error_mark_node) + { + TREE_TYPE (decl) = error_mark_node; + return NULL_TREE; + } + /* Things that are going to be initialized need to have complete type. */ TREE_TYPE (decl) = type = complete_type (TREE_TYPE (decl)); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-09 23:29 ` Paolo Carlini @ 2016-06-10 1:43 ` Paolo Carlini 2016-06-10 20:09 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Paolo Carlini @ 2016-06-10 1:43 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 85 bytes --] ... and this version passes testing. For real. Paolo. //////////////////////////// [-- Attachment #2: patch_70202_2 --] [-- Type: text/plain, Size: 1008 bytes --] Index: cp/decl.c =================================================================== --- cp/decl.c (revision 237285) +++ cp/decl.c (working copy) @@ -6002,6 +6002,13 @@ check_initializer (tree decl, tree init, int flags tree init_code = NULL; tree core_type; + if (init && init != error_mark_node + && TREE_TYPE (init) == error_mark_node) + { + TREE_TYPE (decl) = error_mark_node; + return NULL_TREE; + } + /* Things that are going to be initialized need to have complete type. */ TREE_TYPE (decl) = type = complete_type (TREE_TYPE (decl)); 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; // { dg-error "incomplete type" } +A a = b2; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-10 1:43 ` Paolo Carlini @ 2016-06-10 20:09 ` Jason Merrill 2016-06-10 20:15 ` Paolo Carlini 2016-06-10 20:30 ` Paolo Carlini 0 siblings, 2 replies; 14+ messages in thread From: Jason Merrill @ 2016-06-10 20:09 UTC (permalink / raw) To: Paolo Carlini; +Cc: gcc-patches On Thu, Jun 9, 2016 at 9:43 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > ... and this version passes testing. For real. This seems reasonable, but I still wonder why b1 has error_mark_node type, since it has no initializer at all. Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-10 20:09 ` Jason Merrill @ 2016-06-10 20:15 ` Paolo Carlini 2016-06-10 20:30 ` Paolo Carlini 1 sibling, 0 replies; 14+ messages in thread From: Paolo Carlini @ 2016-06-10 20:15 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches Hi, On 10/06/2016 22:08, Jason Merrill wrote: > On Thu, Jun 9, 2016 at 9:43 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote: >> ... and this version passes testing. For real. > This seems reasonable, but I still wonder why b1 has error_mark_node > type, since it has no initializer at all. Well, we are now pondering very basic things we do, which normally work.. The type of b1 is B, which *is* an erroneous type, I'm not surprised by that. I'm not sure where we are going with this... if we don't have an error_mark_node as type, again ocp_convert will not catch it, and we are going to crash in build_simple_base_path.... ?? Thanks, Paolo. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-10 20:09 ` Jason Merrill 2016-06-10 20:15 ` Paolo Carlini @ 2016-06-10 20:30 ` Paolo Carlini 2016-06-10 20:34 ` Paolo Carlini 1 sibling, 1 reply; 14+ messages in thread From: Paolo Carlini @ 2016-06-10 20:30 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches .. the type of b1 becomes error_mark_node at the end of start_decl as an effect of start_decl_1 (which issues the incomplete type error). Paolo. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-10 20:30 ` Paolo Carlini @ 2016-06-10 20:34 ` Paolo Carlini 2016-06-10 20:48 ` Paolo Carlini 0 siblings, 1 reply; 14+ messages in thread From: Paolo Carlini @ 2016-06-10 20:34 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches . in particular there is this comment in start_decl_1, where the type becomes error_mark_node: 5102 if (type_uses_auto (type)) 5103 error ("declaration of %q#D has no initializer", decl); 5104 else 5105 error ("aggregate %q#D has incomplete type and cannot be defined", 5106 decl); 5107 /* Change the type so that assemble_variable will give 5108 DECL an rtl we can live with: (mem (const_int 0)). */ 5109 type = TREE_TYPE (decl) = error_mark_node; I'd like to have some help about that... Thanks, Paolo. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ RFC / Patch] Again about PR 70202 2016-06-10 20:34 ` Paolo Carlini @ 2016-06-10 20:48 ` Paolo Carlini 2016-06-11 16:06 ` [C++ Patch] PR 70202: avoid creating incomplete types (was: "[C++ RFC / Patch] Again about PR 70202") Paolo Carlini 0 siblings, 1 reply; 14+ messages in thread From: Paolo Carlini @ 2016-06-10 20:48 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches .. if I disregard that comment, the incomplete type B gets through, gets to check_initializer (which is involved anyway even if there is no initializer in this case for b1) and somewhat comically gets to: else if (!COMPLETE_TYPE_P (type)) { error_at (DECL_SOURCE_LOCATION (decl), "%q#D has incomplete type", decl); TREE_TYPE (decl) = error_mark_node; return NULL_TREE; } thus we get a duplicate diagnostic about incompleteness and the type is turned into error_mark_node anyway ;) Paolo. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [C++ Patch] PR 70202: avoid creating incomplete types (was: "[C++ RFC / Patch] Again about PR 70202") 2016-06-10 20:48 ` Paolo Carlini @ 2016-06-11 16:06 ` Paolo Carlini 2016-06-14 13:06 ` [C++ Patch] PR 70202: avoid creating incomplete types Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Paolo Carlini @ 2016-06-11 16:06 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 2575 bytes --] 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. //////////////////////////// [-- Attachment #2: patch_70202_3 --] [-- Type: text/plain, Size: 6124 bytes --] 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; } \f 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; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ Patch] PR 70202: avoid creating incomplete types 2016-06-11 16:06 ` [C++ Patch] PR 70202: avoid creating incomplete types (was: "[C++ RFC / Patch] Again about PR 70202") Paolo Carlini @ 2016-06-14 13:06 ` Jason Merrill 2016-06-14 18:09 ` Paolo Carlini 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2016-06-14 13:06 UTC (permalink / raw) To: Paolo Carlini; +Cc: gcc-patches On 06/11/2016 12:06 PM, Paolo Carlini wrote: > 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? Why is vtt null? Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ Patch] PR 70202: avoid creating incomplete types 2016-06-14 13:06 ` [C++ Patch] PR 70202: avoid creating incomplete types Jason Merrill @ 2016-06-14 18:09 ` Paolo Carlini 2016-06-14 19:29 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Paolo Carlini @ 2016-06-14 18:09 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1772 bytes --] HI, On 14/06/2016 15:06, Jason Merrill wrote: > On 06/11/2016 12:06 PM, Paolo Carlini wrote: >> 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? > > Why is vtt null? It's a mismatch caused by the structure of xref_basetypes, which first computes the number of virtual bases, etc, and then possibly errors out and then drops the duplicate bases (or other invalid bases, see the additional testcases below involving unions). Thus for: struct B : A, virtual A { } when the second A is dropped the work done in the first part of the function, thus, in particular: vec_alloc (CLASSTYPE_VBASECLASSES (ref), max_vbases); with max_vbases == 1 becomes inconsistent. As an a obvious check, this: struct B : virtual A, A { } does not cause problems. The below goes the most obvious way: keeps track of the number of dropped virtual bases (direct and indirect, that's important for eg, unions too) and if so-to-speak nothing virtual remains at the end of the xref_basetypes loop, does a vec_free which undoes the vec_alloc above. I considered moving the new code to a separate function, but frankly I'm not sure it would add much clarity in this case. Also note, vs the patch I posted the last time, there are a few minor changes before the xref_basetypes, essentially reverting that 2006 commit more precisely (ie, no early returns at all). What do you think? Thanks, Paolo. ///////////// [-- Attachment #2: patch_70202_5 --] [-- Type: text/plain, Size: 7800 bytes --] Index: cp/cp-tree.h =================================================================== --- cp/cp-tree.h (revision 237443) +++ 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 237443) +++ 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,19 +12950,13 @@ 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) { if (TYPE_FOR_JAVA (ref)) - { - error ("Java class %qT cannot have multiple bases", ref); - return false; - } + error ("Java class %qT cannot have multiple bases", ref); else warning (OPT_Wmultiple_inheritance, "%qT defined with multiple direct bases", ref); @@ -12980,10 +12967,7 @@ xref_basetypes (tree ref, tree base_list) vec_alloc (CLASSTYPE_VBASECLASSES (ref), max_vbases); if (TYPE_FOR_JAVA (ref)) - { - error ("Java class %qT cannot have virtual bases", ref); - return false; - } + error ("Java class %qT cannot have virtual bases", ref); else if (max_dvbases) warning (OPT_Wvirtual_inheritance, "%qT defined with direct virtual base", ref); @@ -13006,7 +12990,7 @@ xref_basetypes (tree ref, tree base_list) { error ("base type %qT fails to be a struct or class type", basetype); - return false; + goto dropped_base; } if (TYPE_FOR_JAVA (basetype) && (current_lang_depth () == 0)) @@ -13040,7 +13024,7 @@ xref_basetypes (tree ref, tree base_list) error ("recursive type %qT undefined", basetype); else error ("duplicate base type %qT invalid", basetype); - return false; + goto dropped_base; } if (PACK_EXPANSION_P (TREE_VALUE (base_list))) @@ -13056,8 +13040,25 @@ xref_basetypes (tree ref, tree base_list) BINFO_BASE_APPEND (binfo, base_binfo); BINFO_BASE_ACCESS_APPEND (binfo, access); + continue; + + dropped_base: + /* Update max_vbases to reflect the reality that we are dropping + this base: if it reaches zero we want to undo the vec_alloc + above to avoid inconsistencies during error-recovery: eg, in + build_special_member_call, CLASSTYPE_VBASECLASSES non null + and vtt null (c++/27952). */ + if (via_virtual) + max_vbases--; + if (CLASS_TYPE_P (basetype)) + max_vbases + -= vec_safe_length (CLASSTYPE_VBASECLASSES (basetype)); } + if (CLASSTYPE_VBASECLASSES (ref) + && max_vbases == 0) + vec_free (CLASSTYPE_VBASECLASSES (ref)); + if (vec_safe_length (CLASSTYPE_VBASECLASSES (ref)) < max_vbases) /* If we didn't get max_vbases vbases, we must have shared at least one of them, and are therefore diamond shaped. */ @@ -13088,8 +13089,6 @@ xref_basetypes (tree ref, tree base_list) else break; } - - return true; } \f Index: cp/parser.c =================================================================== --- cp/parser.c (revision 237443) +++ 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 237443) +++ testsuite/g++.dg/inherit/virtual1.C (working copy) @@ -1,4 +1,4 @@ -//PR c++/27952 +// PR c++/27952 struct A { @@ -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; Index: testsuite/g++.dg/inherit/virtual12.C =================================================================== --- testsuite/g++.dg/inherit/virtual12.C (revision 0) +++ testsuite/g++.dg/inherit/virtual12.C (working copy) @@ -0,0 +1,14 @@ +// PR c++/70202 + +union U { }; + +struct A +{ + virtual ~A() {} +}; + +struct B : A, virtual U { }; // { dg-error "base type 'U' fails" } + +struct C : A, B {}; // { dg-message "direct base 'A' inaccessible" } + +C c; Index: testsuite/g++.dg/inherit/virtual13.C =================================================================== --- testsuite/g++.dg/inherit/virtual13.C (revision 0) +++ testsuite/g++.dg/inherit/virtual13.C (working copy) @@ -0,0 +1,16 @@ +// PR c++/70202 + +struct D { }; + +union U : virtual D { }; // { dg-error "derived union" } + +struct A +{ + virtual ~A() {} +}; + +struct B : A, virtual U { }; // { dg-error "base type 'U' fails" } + +struct C : A, B {}; // { dg-message "direct base 'A' inaccessible" } + +C c; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [C++ Patch] PR 70202: avoid creating incomplete types 2016-06-14 18:09 ` Paolo Carlini @ 2016-06-14 19:29 ` Jason Merrill 0 siblings, 0 replies; 14+ messages in thread From: Jason Merrill @ 2016-06-14 19:29 UTC (permalink / raw) To: Paolo Carlini; +Cc: gcc-patches OK. Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-06-14 19:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-09 17:02 [C++ RFC / Patch] Again about PR 70202 Paolo Carlini 2016-06-09 17:06 ` Jakub Jelinek 2016-06-09 21:39 ` Jason Merrill 2016-06-09 23:29 ` Paolo Carlini 2016-06-10 1:43 ` Paolo Carlini 2016-06-10 20:09 ` Jason Merrill 2016-06-10 20:15 ` Paolo Carlini 2016-06-10 20:30 ` Paolo Carlini 2016-06-10 20:34 ` Paolo Carlini 2016-06-10 20:48 ` Paolo Carlini 2016-06-11 16:06 ` [C++ Patch] PR 70202: avoid creating incomplete types (was: "[C++ RFC / Patch] Again about PR 70202") Paolo Carlini 2016-06-14 13:06 ` [C++ Patch] PR 70202: avoid creating incomplete types Jason Merrill 2016-06-14 18:09 ` Paolo Carlini 2016-06-14 19:29 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).