Re: [patch] Fix PR c++/22508 ICE after invalid operator new

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

Mark Mitchell wrote:
> Volker Reichelt wrote:
> > Consider the testcase
> > 
> >   struct A
> >   {
> >     void* operator new(__SIZE_TYPE__) throw(X);
> >   };
> > 
> >   A* p = new A;
> 
> I rather think we should fix this by just ignoring the exception 
> specification on the operator when its invalid.

This is actually what happens - sort of:
The function cp_parser_exception_specification_opt calls
cp_parser_type_id_list which in turn calls cp_parser_type_id to
parse a type. Since X is not a type, this fails and we end up with
an empty exception specification (which is good). Alas the parser thinks
that the exception specification should have ended before the X - as
indicated by the following error message before the segfault:

  PR22508.cc:3: error: expected type-specifier before 'X'
  PR22508.cc:3: error: expected `)' before 'X'
  PR22508.cc:3: error: expected ';' before 'X'

We then have a superfluous "X)" before the ";" in the token stream.
And that's why the function is finally rejected. This can be demonstrated
by the following code snippet without exception specification that also
crashes:

  struct A
  {
    void* operator new(__SIZE_TYPE__) X;
  };

  A* p = new A;

PR22508A.cc:3: error: expected ';' before 'X'
PR22508A.cc:6: internal compiler error: Segmentation fault

One option would be to complain about superfluous tokens before the ";"
but otherwise ignore them. This could be done with the following patch.

===================================================================
--- gcc/gcc/cp/parser.c	2 Jul 2005 13:19:38 -0000	1.346
+++ gcc/gcc/cp/parser.c	5 Aug 2005 23:03:06 -0000
@@ -13541,8 +13541,6 @@ cp_parser_member_declaration (cp_parser*
 	      cp_parser_error (parser, "expected %<;%>");
 	      /* Skip tokens until we find a `;'.  */
 	      cp_parser_skip_to_end_of_statement (parser);
-
-	      break;
 	    }
 
 	  if (decl)
===================================================================

The patch causes two regressions in the testsuite: In one case
(g++.dg/parse/crash11.C) we'd generate less errors and in the second
case (g++.dg/tc1/dr176.C) we'd generate more errors.

One could also conditionalize the above break statement on the type of the
current declaration: A function declaration with its parentheses seems to
have more structure than a type declaration. Therefore one might be
confident that one really saw a function declaration plus garbage if there
are superfluous tokens before the ";". So 

  if (TREE_CODE (decl) != FUNCTION_DECL)
    break;

might be a sensible choice.

What do you suggest? Using the original patch, or removing the "break",
or conditionalizing the "break"? Or something completely different?

> The problem with keeping around invalid trees is that it's so easy to 
> fall over them.  I think that we should try to work in the direction 
> that whatever trees we keep are actually reasonable, rather than full of 
> error_mark_nodes.

In theory I agree, in practice there are so many ways to write invalid
C++ programs that this is a difficult task which
a) is often not worth the hassle (and the bloat)
b) can make error messages even worse - if the compiler is trying to be
   smart and starts guessing what the user intended although something
   completely different was intended. This usually srews up follow-up
   messages completely.

Regards,
Volker