[C++ RFC / Patch] Again about PR 70202

[C++ RFC / Patch] Again about PR 70202

[Paolo Carlini]

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.

Re: [C++ RFC / Patch] Again about PR 70202

[Jakub Jelinek]

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

Re: [C++ RFC / Patch] Again about PR 70202

[Jason Merrill]

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.
>
>

Re: [C++ RFC / Patch] Again about PR 70202

[Paolo Carlini]

[-- 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));

Re: [C++ RFC / Patch] Again about PR 70202

[Paolo Carlini]

[-- 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;

Re: [C++ RFC / Patch] Again about PR 70202

[Jason Merrill]

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

Re: [C++ RFC / Patch] Again about PR 70202

[Paolo Carlini]

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.

Re: [C++ RFC / Patch] Again about PR 70202

[Paolo Carlini]

.. 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.

Re: [C++ RFC / Patch] Again about PR 70202

[Paolo Carlini]

. 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.

Re: [C++ RFC / Patch] Again about PR 70202

[Paolo Carlini]

.. 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.

[C++ Patch] PR 70202: avoid creating incomplete types (was: "[C++ RFC / Patch] Again about PR 70202")

[Paolo Carlini]

[-- 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;

Re: [C++ Patch] PR 70202: avoid creating incomplete types

[Jason Merrill]

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

Re: [C++ Patch] PR 70202: avoid creating incomplete types

[Paolo Carlini]

[-- 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;

Re: [C++ Patch] PR 70202: avoid creating incomplete types

[Jason Merrill]

OK.

Jason