* [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).