diff mbox

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

Message ID 575C370C.30307@oracle.com
State New
Headers show

Commit Message

Paolo Carlini June 11, 2016, 4:06 p.m. UTC 
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.

////////////////////////////

Comments

Jason Merrill June 14, 2016, 1:06 p.m. UTC | #1 
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
diff mbox

Patch

Index: cp/call.c
===================================================================
--- cp/call.c	(revision 237318)
+++ cp/call.c	(working copy)
@@ -8022,6 +8022,8 @@  build_special_member_call (tree instance, tree nam
 	 or destructor, then we fetch the VTT directly.
 	 Otherwise, we look it up using the VTT we were given.  */
       vtt = DECL_CHAIN (CLASSTYPE_VTABLES (current_class_type));
+      if (!vtt)
+	return error_mark_node;
       vtt = decay_conversion (vtt, complain);
       if (vtt == error_mark_node)
 	return error_mark_node;
Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 237318)
+++ cp/cp-tree.h	(working copy)
@@ -5771,7 +5771,7 @@  extern int grok_ctor_properties			(const_tree, con
 extern bool grok_op_properties			(tree, bool);
 extern tree xref_tag				(enum tag_types, tree, tag_scope, bool);
 extern tree xref_tag_from_type			(tree, tree, tag_scope);
-extern bool xref_basetypes			(tree, tree);
+extern void xref_basetypes			(tree, tree);
 extern tree start_enum				(tree, tree, tree, tree, bool, bool *);
 extern void finish_enum_value_list		(tree);
 extern void finish_enum				(tree);
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 237318)
+++ cp/decl.c	(working copy)
@@ -12871,12 +12871,9 @@  xref_tag_from_type (tree old, tree id, tag_scope s
 /* Create the binfo hierarchy for REF with (possibly NULL) base list
    BASE_LIST.  For each element on BASE_LIST the TREE_PURPOSE is an
    access_* node, and the TREE_VALUE is the type of the base-class.
-   Non-NULL TREE_TYPE indicates virtual inheritance.  
- 
-   Returns true if the binfo hierarchy was successfully created,
-   false if an error was detected. */
+   Non-NULL TREE_TYPE indicates virtual inheritance.  */
 
-bool
+void
 xref_basetypes (tree ref, tree base_list)
 {
   tree *basep;
@@ -12889,7 +12886,7 @@  xref_basetypes (tree ref, tree base_list)
   tree igo_prev; /* Track Inheritance Graph Order.  */
 
   if (ref == error_mark_node)
-    return false;
+    return;
 
   /* The base of a derived class is private by default, all others are
      public.  */
@@ -12933,11 +12930,7 @@  xref_basetypes (tree ref, tree base_list)
 
   /* The binfo slot should be empty, unless this is an (ill-formed)
      redefinition.  */
-  if (TYPE_BINFO (ref) && !TYPE_SIZE (ref))
-    {
-      error ("redefinition of %q#T", ref);
-      return false;
-    }
+  gcc_assert (!TYPE_BINFO (ref) || TYPE_SIZE (ref));
 
   gcc_assert (TYPE_MAIN_VARIANT (ref) == ref);
 
@@ -12957,10 +12950,7 @@  xref_basetypes (tree ref, tree base_list)
       CLASSTYPE_NON_AGGREGATE (ref) = 1;
 
       if (TREE_CODE (ref) == UNION_TYPE)
-        {
-	  error ("derived union %qT invalid", ref);
-          return false;
-        }
+	error ("derived union %qT invalid", ref);
     }
 
   if (max_bases > 1)
@@ -12968,7 +12958,7 @@  xref_basetypes (tree ref, tree base_list)
       if (TYPE_FOR_JAVA (ref))
         {
 	  error ("Java class %qT cannot have multiple bases", ref);
-          return false;
+          return;
         }
       else
 	warning (OPT_Wmultiple_inheritance,
@@ -12982,7 +12972,7 @@  xref_basetypes (tree ref, tree base_list)
       if (TYPE_FOR_JAVA (ref))
         {
 	  error ("Java class %qT cannot have virtual bases", ref);
-          return false;
+          return;
         }
       else if (max_dvbases)
 	warning (OPT_Wvirtual_inheritance,
@@ -13006,7 +12996,7 @@  xref_basetypes (tree ref, tree base_list)
 	{
 	  error ("base type %qT fails to be a struct or class type",
 		 basetype);
-	  return false;
+	  continue;
 	}
 
       if (TYPE_FOR_JAVA (basetype) && (current_lang_depth () == 0))
@@ -13040,7 +13030,7 @@  xref_basetypes (tree ref, tree base_list)
 	    error ("recursive type %qT undefined", basetype);
 	  else
 	    error ("duplicate base type %qT invalid", basetype);
-	  return false;
+	  continue;
 	}
 
       if (PACK_EXPANSION_P (TREE_VALUE (base_list)))
@@ -13088,8 +13078,6 @@  xref_basetypes (tree ref, tree base_list)
 	else
 	  break;
     }
-
-  return true;
 }
 
 
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 237318)
+++ cp/parser.c	(working copy)
@@ -22050,9 +22050,8 @@  cp_parser_class_head (cp_parser* parser,
 
   /* If we're really defining a class, process the base classes.
      If they're invalid, fail.  */
-  if (type && cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE)
-      && !xref_basetypes (type, bases))
-    type = NULL_TREE;
+  if (type && cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE))
+    xref_basetypes (type, bases);
 
  done:
   /* Leave the scope given by the nested-name-specifier.  We will
Index: testsuite/g++.dg/inherit/crash6.C
===================================================================
--- testsuite/g++.dg/inherit/crash6.C	(revision 0)
+++ testsuite/g++.dg/inherit/crash6.C	(working copy)
@@ -0,0 +1,10 @@ 
+// PR c++/70202
+
+class A
+{
+  virtual void foo () { }
+};
+class B : public A, A { };  // { dg-error "duplicate base type" }
+
+B b1, &b2 = b1;
+A a = b2;
Index: testsuite/g++.dg/inherit/union2.C
===================================================================
--- testsuite/g++.dg/inherit/union2.C	(revision 0)
+++ testsuite/g++.dg/inherit/union2.C	(working copy)
@@ -0,0 +1,3 @@ 
+struct A { };
+union U : A { };  // { dg-error "derived union 'U' invalid" }
+U u;
Index: testsuite/g++.dg/inherit/virtual1.C
===================================================================
--- testsuite/g++.dg/inherit/virtual1.C	(revision 237318)
+++ testsuite/g++.dg/inherit/virtual1.C	(working copy)
@@ -5,8 +5,8 @@  struct A
     virtual ~A() {}
 };
 
-struct B : A, virtual A {};     // { dg-error "duplicate base|forward declaration" }
+struct B : A, virtual A {};     // { dg-error "duplicate base" }
 
-struct C : A, B {};             // { dg-error "duplicate base|invalid use" }
+struct C : A, B {};             // { dg-message "direct base 'A' inaccessible" }
 
-C c;                            // { dg-error "aggregate" }
+C c;