[C++ Patch] PR 68722

[C++ Patch] PR 68722

[Paolo Carlini]

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

Hi,

I'm having a look at this ICE during error recovery regression and I 
have a couple of different proposals which both pass testing. In the 
first case, instead of reaching (in cp_parser_cache_defarg):

   default_argument = make_node (DEFAULT_ARG);
   DEFARG_TOKENS (default_argument)
     = cp_token_cache_new (first_token, token);
   DEFARG_INSTANTIATIONS (default_argument) = NULL;

   return default_argument;

we could simply return an error_mark_node right after:

error_at (token->location, "file ends in default argument");

and certainly avoid the ICE in merge_exception_specifiers much later. 
Alternately, maybe it's too heavy handed, maybe fatal_errors are really 
appropriate in a very restrict set of cases (not sure), it would work to 
simply change the error_at to fatal_error, which also cuts some 
redundant diagnostic we produce right now after the sensible one about 
file ends. The idea occurred to me in particular because the error_at 
above does *not* result (and never did to my best knowledge) in a actual 
location being printed, we issue:

cc1plus: error: file ends in default argument

which smells like a fatal-type error message?!?

What do you think?

Thanks,
Paolo.

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

[-- Attachment #2: CL_68722 --]
[-- Type: text/plain, Size: 291 bytes --]

/cp
2016-04-08  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/68722
	* parser.c (cp_parser_cache_defarg): When file ends in default
	argument simply return error_mark_node.

/testsuite
2016-04-08  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/68722
	* g++.dg/parse/pr68722.C: New.

[-- Attachment #3: patch_68722 --]
[-- Type: text/plain, Size: 959 bytes --]

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 234836)
+++ cp/parser.c	(working copy)
@@ -27472,8 +27472,7 @@ cp_parser_cache_defarg (cp_parser *parser, bool ns
 	case CPP_EOF:
 	case CPP_PRAGMA_EOL:
 	  error_at (token->location, "file ends in default argument");
-	  done = true;
-	  break;
+	  return error_mark_node;
 
 	case CPP_NAME:
 	case CPP_SCOPE:
Index: testsuite/g++.dg/parse/pr68722.C
===================================================================
--- testsuite/g++.dg/parse/pr68722.C	(revision 0)
+++ testsuite/g++.dg/parse/pr68722.C	(working copy)
@@ -0,0 +1,9 @@
+// PR c++/68722
+
+class A {
+  &__loc   // { dg-error "" }
+} class ios_base {  // { dg-error "" }
+  A _M_ios_locale ios_base(ios_base &) template <_Traits> class basic_ios {  // { dg-error "" }
+basic_ios basic_ios = operator=  // { dg-error "" }
+
+// { dg-prune-output "file ends in default argument" }

[-- Attachment #4: patch_68722_alt --]
[-- Type: text/plain, Size: 1054 bytes --]

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 234836)
+++ cp/parser.c	(working copy)
@@ -27471,7 +27471,7 @@ cp_parser_cache_defarg (cp_parser *parser, bool ns
 	  /* If we run out of tokens, issue an error message.  */
 	case CPP_EOF:
 	case CPP_PRAGMA_EOL:
-	  error_at (token->location, "file ends in default argument");
+	  fatal_error (token->location, "file ends in default argument");
 	  done = true;
 	  break;
 
Index: testsuite/g++.dg/parse/pr68722.C
===================================================================
--- testsuite/g++.dg/parse/pr68722.C	(revision 0)
+++ testsuite/g++.dg/parse/pr68722.C	(working copy)
@@ -0,0 +1,10 @@
+// PR c++/68722
+
+class A {
+  &__loc   // { dg-error "" }
+} class ios_base {  // { dg-error "" }
+  A _M_ios_locale ios_base(ios_base &) template <_Traits> class basic_ios {  // { dg-error "" }
+basic_ios basic_ios = operator=
+
+// { dg-prune-output "file ends in default argument" }
+// { dg-prune-output "compilation terminated" }

Re: [C++ Patch] PR 68722

[Jason Merrill]

Let's go with the first patch.

Jason

Re: [C++ Patch] PR 68722

[Paolo Carlini]

Hi again,

On 12/04/2016 15:53, Jason Merrill wrote:
> Let's go with the first patch.
What about this one? Today I returned to it, and technically it still 
represents a regression in gcc-4_9-branch and gcc-5-branch, but 
personally I'd rather not backport the fix: in release-mode we just emit 
an additional "confused by earlier errors, bailing out" at the end of a 
rather long series of diagnostic messages, the first ones meaningful, 
the last redundant anyway and the snippet triggering it seems 
particularly broken to me...

Thanks,
Paolo.

Re: [C++ Patch] PR 68722

[Jason Merrill]

Agreed, I tend not to backport bugs on invalid code, definitely not if
we already give a useful diagnostic.

Jason


On Wed, May 4, 2016 at 4:10 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi again,
>
> On 12/04/2016 15:53, Jason Merrill wrote:
>>
>> Let's go with the first patch.
>
> What about this one? Today I returned to it, and technically it still
> represents a regression in gcc-4_9-branch and gcc-5-branch, but personally
> I'd rather not backport the fix: in release-mode we just emit an additional
> "confused by earlier errors, bailing out" at the end of a rather long series
> of diagnostic messages, the first ones meaningful, the last redundant anyway
> and the snippet triggering it seems particularly broken to me...
>
> Thanks,
> Paolo.
>