Re: [patch] Fix PR c++/27508: ICE on invalid destructor name

[Volker Reichelt <reichelt at igpm dot rwth-aachen dot de>]

On  3 Jun, Mark Mitchell wrote:
> Volker Reichelt wrote:
> 
>> 2006-06-03  Volker Reichelt  <reichelt@igpm.rwth-aachen.de>
>> 
>> 	PR c++/27508
>> 	* parser.c (cp_parser_using_declaration): Check for destructor names.
> 
> That's a little brutal, even for me; I might have applied the check
> after the call to parse the unqualified-id returned a destructor name,

That would be to late, the ICE happens in check_dtor_name which is
called by cp_parser_unqualified_id.

> but your approach should be fine.

Well. Actually not. :-(
See below.

> Please apply the patch.

I rather withdraw the patch and suggest a third version of the patch:
The C++ frontend ICEs on the following code snippet:

  struct A;
  using ::~A;

bug.cc:2: internal compiler error: tree check: expected class 'type', have 'declaration' (namespace_decl) in check_dtor_name, at cp/call.c:239
Please submit a full bug report, [etc.]

The first version fixed the problem by rejecting NAMESPACE_DECLs as
basetypes in check_dtor_name.

Mark wrote:
> In general, if an error can be caught in the parser, we want to catch it
> there.  We should try to make other routines "type-safe" in the sense
> that if a parameter is supposed to be a _TYPE, then, the only values it
> actually has are a _TYPE node or error_mark_node.  (This has been a
> gradual change over the past several years; with the YACC parser, we had
> no choice but to try to sort things out in the semantic analysis routines.)
>
> So, here, we should never be passing a NAMESPACE_DECL as a type.
> Instead, if SCOPE is a NAMESPACE_DECL, we should just issue an error in
> the parser.

Although I agree that we shouldn't be passing garbage to routines which
we could have caught before, I'm not quite convinced that we shouldn't
do the test in check_dtor_name. This routine is called directly from the
parser and is just a helper routine for error catching (it should probably
be moved out of call.c).

Anyways, the second version tried to fix the PR in
cp_parser_using_declaration right before the call to
cp_parser_unqualified_id (which in turn calls check_dtor_name).
I'm withdrawing this patch, because it fails to fix the second
testcase added below where cp_parser_unqualified_id is called
from a different routine.

So here's a third version which checks for the invalid scope
in cp_parser_unqualified_id. It checks for NAMESPACE_DECLs and
error_mark_nodes and adds a gcc_assert to make sure that we only
have scopes with valid types.

One problem remains, though: We call check_dtor_name not only from
the parser, but also from typeck.c. I'm not sure whether it is possible
to pass a NAMESPACE_DECL via this path. The first patch would deal
with that situation, the patch below would not.

Bootstrapped and regtested on x86_64-unknown-linux-gnu.
Ok for mainline, 4.1 branch, and 4.0 branch?
Or is the first patch better?

Regards,
Volker

:ADDPATCH C++:


2006-06-07  Volker Reichelt  <reichelt@igpm.rwth-aachen.de>

	PR c++/27508
	* parser.c (cp_parser_unqualified_id): Check for invalid scopes
	when parsing destructor names.

===================================================================
--- gcc/gcc/cp/parser.c	(revision 114427)
+++ gcc/gcc/cp/parser.c	(working copy)
@@ -3374,8 +3374,23 @@ cp_parser_unqualified_id (cp_parser* parser,
 	object_scope = parser->object_scope;
 	qualifying_scope = parser->qualifying_scope;
 
+	/* Check for invalid scopes.  */
+	if (scope == error_mark_node)
+	  {
+	    cp_parser_skip_to_end_of_statement (parser);
+	    return error_mark_node;
+	  }
+	if (scope && TREE_CODE (scope) == NAMESPACE_DECL)
+	  {
+	    if (!cp_parser_uncommitted_to_tentative_parse_p (parser))
+	      error ("scope %qT before %<~%> is not a class-name", scope);
+	    cp_parser_skip_to_end_of_statement (parser);
+	    return error_mark_node;
+	  }
+	gcc_assert (!scope || TYPE_P (scope));
+
 	/* If the name is of the form "X::~X" it's OK.  */
-	if (scope && TYPE_P (scope)
+	if (scope
 	    && cp_lexer_next_token_is (parser->lexer, CPP_NAME)
 	    && (cp_lexer_peek_nth_token (parser->lexer, 2)->type
 		== CPP_OPEN_PAREN)
@@ -3458,7 +3473,7 @@ cp_parser_unqualified_id (cp_parser* parser,
 	   destructor is the same as the name of the qualifying
 	   class.  That allows us to keep parsing after running
 	   into ill-formed destructor names.  */
-	if (type_decl == error_mark_node && scope && TYPE_P (scope))
+	if (type_decl == error_mark_node && scope)
 	  return build_nt (BIT_NOT_EXPR, scope);
 	else if (type_decl == error_mark_node)
 	  return error_mark_node;
===================================================================

2006-06-07  Volker Reichelt  <reichelt@igpm.rwth-aachen.de>

	PR c++/27508
	* g++.dg/parse/dtor9.C: New test.
	* g++.dg/parse/dtor10.C: New test.
	* g++.dg/other/error7.C: Adjust error-marker.

===================================================================
--- gcc/gcc/testsuite/g++.dg/parse/dtor9.C	2003-09-23 19:59:22 +0200
+++ gcc/gcc/testsuite/g++.dg/parse/dtor9.C	2006-05-08 22:11:44 +0200
@@ -0,0 +1,5 @@
+// PR c++/27508
+// { dg-do compile }
+
+struct A;
+using ::~A;  // { dg-error "not a class-name" }
===================================================================
--- gcc/gcc/testsuite/g++.dg/parse/dtor10.C	2005-08-29 00:25:44 +0200
+++ gcc/gcc/testsuite/g++.dg/parse/dtor10.C	2006-06-07 01:08:01 +0200
@@ -0,0 +1,9 @@
+// PR c++/27508
+// { dg-do compile }
+
+namespace N
+{
+    struct A { ~A(); };
+}
+
+N::~A () {}  // { dg-error "not a class-name" }
===================================================================
--- gcc/gcc/testsuite/g++.dg/other/error7.C	(revision 114459)
+++ gcc/gcc/testsuite/g++.dg/other/error7.C	(working copy)
@@ -8,5 +8,5 @@
 
 void foo(void)
 {
-  N::~A();    // { dg-error "not a member" }
+  N::~A();    // { dg-error "not a class-name" }
 }

===================================================================