[PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Simon Martin]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

Hi all.

The following invalid snippet triggers an ICE with the mainline:

=== cut here ===
void foo()
{
    namespace N {
=== cut here ===

The 'namespace N {' is not allowed here ('namespace N = foo;' would be),
and the code handles it by skipping all the tokens up to the closing
brace, and then consuming the next token.

In that particular case, there is no closing brace, so we end at the
EOF, and an assertion fails when consuming the next token.

The attached patch fixes this by ensuring that we've not reached the EOF
before trying to consume the next token.

I've regtested this on i386-apple-darwin8.9.1 with no new unexpected
failures. Is it OK for mainline?

Best regards,
Simon

:ADDPATCH c++:


[-- Attachment #2: CL_31745 --]
[-- Type: text/plain, Size: 201 bytes --]

2007-05-16  Simon Martin  <simartin@users.sourceforge.net>

	PR c++/31745
	* parser.c (cp_parser_namespace_alias_definition): Don't try to
	consume the next token if the end of file has been reached.


[-- Attachment #3: pr31745.patch --]
[-- Type: text/plain, Size: 582 bytes --]

Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 124776)
+++ gcc/cp/parser.c	(working copy)
@@ -11296,7 +11296,8 @@ cp_parser_namespace_alias_definition (cp
       /* Skip the definition.  */
       cp_lexer_consume_token (parser->lexer);
       cp_parser_skip_to_closing_brace (parser);
-      cp_lexer_consume_token (parser->lexer);
+      if (cp_lexer_next_token_is_not (parser->lexer, CPP_EOF))
+	cp_lexer_consume_token (parser->lexer);
       return;
     }
   cp_parser_require (parser, CPP_EQ, "`='");


[-- Attachment #4: CL_31745_testsuite --]
[-- Type: text/plain, Size: 112 bytes --]

2007-05-16  Simon Martin  <simartin@users.sourceforge.net>

	PR c++/31745
	* g++.dg/parse/crash34.C: New test.


[-- Attachment #5: crash34.C --]
[-- Type: text/plain, Size: 129 bytes --]

/* PR c++/31745 */
/* { dg-do "compile" }  */

void foo()
{
  namespace N { /* { dg-error "is not allowed|at end of input" } */

Re: [PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Manuel López-Ibáñez]

On 17/05/07, Simon Martin <simartin@users.sourceforge.net> wrote:
===================================================================
> --- gcc/cp/parser.c     (revision 124776)
> +++ gcc/cp/parser.c     (working copy)
> @@ -11296,7 +11296,8 @@ cp_parser_namespace_alias_definition (cp
>        /* Skip the definition.  */
>        cp_lexer_consume_token (parser->lexer);
>        cp_parser_skip_to_closing_brace (parser);
> -      cp_lexer_consume_token (parser->lexer);
> +      if (cp_lexer_next_token_is_not (parser->lexer, CPP_EOF))
> +       cp_lexer_consume_token (parser->lexer);
>        return;
>      }
>    cp_parser_require (parser, CPP_EQ, "`='");


If that produces an ICE then there is another potential ICE here:

119318  lmillward   /* Process the base classes. If they're invalid, skip the
119318  lmillward      entire class body.  */
119318  lmillward   if (!xref_basetypes (type, bases))
119318  lmillward     {
119318  lmillward       cp_parser_skip_to_closing_brace (parser);
119318  lmillward
119318  lmillward       /* Consuming the closing brace yields better
error messages
119318  lmillward          later on.  */
119318  lmillward       cp_lexer_consume_token (parser->lexer);
119318  lmillward       pop_deferring_access_checks ();
119318  lmillward       return error_mark_node;
119318  lmillward     }

Also, I think that, by looking at the reasons for
cp_parser_skip_to_closing_brace stopping without finding a closing
brace, a better solution would be to require the closing brace:

  /* Skip the definition.  */
    cp_lexer_consume_token (parser->lexer);
    cp_parser_skip_to_closing_brace (parser);
+ cp_parser_require (parser, CPP_CLOSE_BRACE, "`}'");
    cp_lexer_consume_token (parser->lexer);
   return;

or to test for it before consuming it:

        /* Skip the definition.  */
        cp_lexer_consume_token (parser->lexer);
        cp_parser_skip_to_closing_brace (parser);
 -      cp_lexer_consume_token (parser->lexer);
 +      if (cp_lexer_next_token_is (parser->lexer, CPP_CLOSE_BRACE))
 +       cp_lexer_consume_token (parser->lexer);
        return;
      }

What do you think?

Cheers,

Manuel.

Re: [PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Simon Martin]

Hello Manuel.

> On 17/05/07, Simon Martin <simartin@users.sourceforge.net> wrote:
> [...]
> If that produces an ICE then there is another potential ICE here:
> [...]
Good catch. I'll address this one once it's been decided what is the 
"good" solution for this problem.

> Also, I think that, by looking at the reasons for
> cp_parser_skip_to_closing_brace stopping without finding a closing
> brace, a better solution would be to require the closing brace:
> 
>  /* Skip the definition.  */
>    cp_lexer_consume_token (parser->lexer);
>    cp_parser_skip_to_closing_brace (parser);
> + cp_parser_require (parser, CPP_CLOSE_BRACE, "`}'");
>    cp_lexer_consume_token (parser->lexer);
>   return;
This will not solve the ICE, because we'll still call 
'cp_lexer_consume_token' at the EOF, so the same assertion will fail. It 
will work if we check the value returned by 'cp_parser_require' 
though... and in that case, we will emit an extra error message 
("expected `{'..."). I don't have an opinion whether this is an 
advantage or not (we are doing error recovery, so we might already have 
"skipped" a lot of errors in the code after "namespace N {").
Of course, if you meant to remove the call to 'cp_lexer_consume_token' 
in this patch, then the ICE will indeed go away.

> or to test for it before consuming it:
> 
>        /* Skip the definition.  */
>        cp_lexer_consume_token (parser->lexer);
>        cp_parser_skip_to_closing_brace (parser);
> -      cp_lexer_consume_token (parser->lexer);
> +      if (cp_lexer_next_token_is (parser->lexer, CPP_CLOSE_BRACE))
> +       cp_lexer_consume_token (parser->lexer);
>        return;
>      }
Well, after 'cp_parser_skip_to_closing_brace', we're either at a 
CPP_CLOSE_BRACE or a CPP_EOF, so I guess that this test and the one I've 
put in the patch have the same effect.

Thanks for your feedback.

Best regards,
Simon

Re: [PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Manuel López-Ibáñez]

On 17/05/07, Simon Martin <simartin@users.sourceforge.net> wrote:
> >
> >  /* Skip the definition.  */
> >    cp_lexer_consume_token (parser->lexer);
> >    cp_parser_skip_to_closing_brace (parser);
> > + cp_parser_require (parser, CPP_CLOSE_BRACE, "`}'");
> >    cp_lexer_consume_token (parser->lexer);
> >   return;
> This will not solve the ICE, because we'll still call
> 'cp_lexer_consume_token' at the EOF, so the same assertion will fail.
[snip]
> Of course, if you meant to remove the call to 'cp_lexer_consume_token'
> in this patch, then the ICE will indeed go away.
>

Yes, I meant removing the second cp_lexer_consume_token since
cp_parser_require already consumes the closing brace.

> > or to test for it before consuming it:
> >
> >        /* Skip the definition.  */
> >        cp_lexer_consume_token (parser->lexer);
> >        cp_parser_skip_to_closing_brace (parser);
> > -      cp_lexer_consume_token (parser->lexer);
> > +      if (cp_lexer_next_token_is (parser->lexer, CPP_CLOSE_BRACE))
> > +       cp_lexer_consume_token (parser->lexer);
> >        return;
> >      }
> Well, after 'cp_parser_skip_to_closing_brace', we're either at a
> CPP_CLOSE_BRACE or a CPP_EOF, so I guess that this test and the one I've
> put in the patch have the same effect.

Can't we be at CPP_PRAGMA_EOL ? Look at the code of
cp_parser_skip_to_closing_brace. I still think that my proposal is
more robust. A more efficient version would be for
cp_parser_skip_to_closing_brace (parser) to return 1 if it found the
closing brace or 0 if it didn't. Then,

        cp_lexer_consume_token (parser->lexer);
        if (cp_parser_skip_to_closing_brace (parser))
             cp_lexer_consume_token (parser->lexer);
        return;

>
> Thanks for your feedback.

Thanks to you for considering it.

Manuel.

Re: [PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Lee Millward]

Hi Simon, Manuel,

On 5/17/07, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> If that produces an ICE then there is another potential ICE here:
>
> 119318  lmillward   /* Process the base classes. If they're invalid, skip the
> 119318  lmillward      entire class body.  */
> 119318  lmillward   if (!xref_basetypes (type, bases))
> 119318  lmillward     {
> 119318  lmillward       cp_parser_skip_to_closing_brace (parser);
> 119318  lmillward
> 119318  lmillward       /* Consuming the closing brace yields better
> error messages
> 119318  lmillward          later on.  */
> 119318  lmillward       cp_lexer_consume_token (parser->lexer);
> 119318  lmillward       pop_deferring_access_checks ();
> 119318  lmillward       return error_mark_node;
> 119318  lmillward     }
>

You can trigger an ICE here with the following testcase, perhaps this
should be added to the testsuite as well?

------------- snip --------------------
struct a {};

class foo : public a, a
{
------------- snip --------------------

Cheers,
Lee

Re: [PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Simon Martin]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

Hi all.

I attach an updated patch, that uses 'cp_lexer_next_token_is (...,
CPP_CLOSE_BRACE)' instead of 'cp_lexer_next_token_is_not (..., CPP_EOF)'
as suggested by Manuel, and that also fixes the other testcase that
triggers a similar ICE in another part of the code, contributed by Lee.

I've successfully regtested it on i386-apple-darwin8.9.1 with no new
unexpected failures. Is it OK for mainline?

Best regards,
Simon



[-- Attachment #2: CL_31745_2 --]
[-- Type: text/plain, Size: 280 bytes --]

2007-05-20  Simon Martin  <simartin@users.sourceforge.net>
	    Manuel Lopez-Ibanez  <manu@gcc.gnu.org>

	PR c++/31745
	* parser.c (cp_parser_namespace_alias_definition): Only consume the
	next token if it is a closing brace.

	* parser.c (cp_parser_class_specifier): Likewise.



[-- Attachment #3: pr31745_2.patch --]
[-- Type: text/plain, Size: 979 bytes --]

Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 124874)
+++ gcc/cp/parser.c	(working copy)
@@ -11296,7 +11296,8 @@ cp_parser_namespace_alias_definition (cp
       /* Skip the definition.  */
       cp_lexer_consume_token (parser->lexer);
       cp_parser_skip_to_closing_brace (parser);
-      cp_lexer_consume_token (parser->lexer);
+      if (cp_lexer_next_token_is (parser->lexer, CPP_CLOSE_BRACE))
+	cp_lexer_consume_token (parser->lexer);
       return;
     }
   cp_parser_require (parser, CPP_EQ, "`='");
@@ -13822,7 +13823,8 @@ cp_parser_class_specifier (cp_parser* pa
 
       /* Consuming the closing brace yields better error messages
          later on.  */
-      cp_lexer_consume_token (parser->lexer);
+      if (cp_lexer_next_token_is (parser->lexer, CPP_CLOSE_BRACE))
+	cp_lexer_consume_token (parser->lexer);
       pop_deferring_access_checks ();
       return error_mark_node;
     }



[-- Attachment #4: CL_31745_testsuite_2 --]
[-- Type: text/plain, Size: 195 bytes --]

2007-05-20  Simon Martin  <simartin@users.sourceforge.net>
	    Lee Millward  <lee.millward@gmail.com>

	PR c++/31745
	* g++.dg/parse/crash34.C: New test.

	* g++.dg/parse/crash35.C: New test.



[-- Attachment #5: crash34.C --]
[-- Type: text/plain, Size: 130 bytes --]

/* PR c++/31745 */
/* { dg-do "compile" }  */

void foo()
{
  namespace N { /* { dg-error "is not allowed|at end of input" } */



[-- Attachment #6: crash35.C --]
[-- Type: text/plain, Size: 150 bytes --]

/* This used to ICE. */
/* { dg-do "compile" } */

struct a {};

class foo : public a, a
{ /* { dg-error "duplicate base type|at end of input" } */

Re: [PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Mark Mitchell]

Simon Martin wrote:
> Hi all.
> 
> I attach an updated patch, that uses 'cp_lexer_next_token_is (...,
> CPP_CLOSE_BRACE)' instead of 'cp_lexer_next_token_is_not (..., CPP_EOF)'
> as suggested by Manuel, and that also fixes the other testcase that
> triggers a similar ICE in another part of the code, contributed by Lee.

This version is OK -- though I think that Manuel is correct that
updating cp_parser_skip_to_closing_brace to return the next token type,
or a boolean indicating whether it found a closing brace, is a nice
idea, just in making the code a little simpler.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Simon Martin]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

Hi Mark.

> This version is OK -- though I think that Manuel is correct that
> updating cp_parser_skip_to_closing_brace to return the next token type,
> or a boolean indicating whether it found a closing brace, is a nice
> idea, just in making the code a little simpler.
Ok, here's a version implementing Manuel's idea (I only attach the
updated patch and Changelog for gcc/cp; the testcases and Changelog for
gcc/testsuite are the same as previously).

I've successfully regtested it on i386-apple-darwin8.9.1 with no new
unexpected failures. Is it OK?

Best regards,
Simon


[-- Attachment #2: CL_31745_3 --]
[-- Type: text/plain, Size: 401 bytes --]

2007-05-22  Simon Martin  <simartin@users.sourceforge.net>
	    Manuel Lopez-Ibanez  <manu@gcc.gnu.org>

	PR c++/31745
	* parser.c (cp_parser_skip_to_closing_brace): Return true if the next
	token is a closing brace, false if there are no tokens left.
	(cp_parser_namespace_alias_definition): Only consume the next token if
	it is a closing brace.

	* parser.c (cp_parser_class_specifier): Likewise.


[-- Attachment #3: pr31745_3.patch --]
[-- Type: text/plain, Size: 2254 bytes --]

Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 124910)
+++ gcc/cp/parser.c	(working copy)
@@ -1999,7 +1999,7 @@ static void cp_parser_consume_semicolon_
   (cp_parser *);
 static void cp_parser_skip_to_end_of_block_or_statement
   (cp_parser *);
-static void cp_parser_skip_to_closing_brace
+static bool cp_parser_skip_to_closing_brace
   (cp_parser *);
 static void cp_parser_skip_to_end_of_template_parameter_list
   (cp_parser *);
@@ -2599,9 +2599,10 @@ cp_parser_skip_to_end_of_block_or_statem
 }
 
 /* Skip tokens until a non-nested closing curly brace is the next
-   token.  */
+   token, or there are no more tokens. Return true in the first case,
+   false otherwise.  */
 
-static void
+static bool
 cp_parser_skip_to_closing_brace (cp_parser *parser)
 {
   unsigned nesting_depth = 0;
@@ -2615,13 +2616,13 @@ cp_parser_skip_to_closing_brace (cp_pars
 	case CPP_EOF:
 	case CPP_PRAGMA_EOL:
 	  /* If we've run out of tokens, stop.  */
-	  return;
+	  return false;
 
 	case CPP_CLOSE_BRACE:
 	  /* If the next token is a non-nested `}', then we have reached
 	     the end of the current block.  */
 	  if (nesting_depth-- == 0)
-	    return;
+	    return true;
 	  break;
 
 	case CPP_OPEN_BRACE:
@@ -11295,8 +11296,8 @@ cp_parser_namespace_alias_definition (cp
       error ("%<namespace%> definition is not allowed here");
       /* Skip the definition.  */
       cp_lexer_consume_token (parser->lexer);
-      cp_parser_skip_to_closing_brace (parser);
-      cp_lexer_consume_token (parser->lexer);
+      if (cp_parser_skip_to_closing_brace (parser))
+	cp_lexer_consume_token (parser->lexer);
       return;
     }
   cp_parser_require (parser, CPP_EQ, "`='");
@@ -13818,11 +13819,10 @@ cp_parser_class_specifier (cp_parser* pa
      entire class body.  */
   if (!xref_basetypes (type, bases))
     {
-      cp_parser_skip_to_closing_brace (parser);
-
       /* Consuming the closing brace yields better error messages
          later on.  */
-      cp_lexer_consume_token (parser->lexer);
+      if (cp_parser_skip_to_closing_brace (parser))
+	cp_lexer_consume_token (parser->lexer);
       pop_deferring_access_checks ();
       return error_mark_node;
     }

Re: [PATCH] Fix PR c++/31745: ICE on invalid use of namespace

[Mark Mitchell]

Simon Martin wrote:
> Hi Mark.
> 
>> This version is OK -- though I think that Manuel is correct that
>> updating cp_parser_skip_to_closing_brace to return the next token type,
>> or a boolean indicating whether it found a closing brace, is a nice
>> idea, just in making the code a little simpler.
> Ok, here's a version implementing Manuel's idea (I only attach the
> updated patch and Changelog for gcc/cp; the testcases and Changelog for
> gcc/testsuite are the same as previously).
> 
> I've successfully regtested it on i386-apple-darwin8.9.1 with no new
> unexpected failures. Is it OK?

Yes, this is OK; thanks!

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713