Re: [PATCH C] Fix pr44517

[Shujing Zhao <pearly.zhao@oracle.com>]

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

On 06/22/2010 08:55 PM, Joseph S. Myers wrote:
> On Tue, 22 Jun 2010, Shujing Zhao wrote:
> 
>> Hi,
>>
>> This patch tries to improve diagnostic for wrong type name in function
>> declaration. It also changes the algorithm of function
>> c_parser_parms_list_declarator. If one of parameter declaration is wrong,
>> c_parser_parms_list_declarator would return NULL, not an empty parameter
>> information struct. A test case is added to test this change.
> 
> You are changing the semantics of the variable good_parm from meaning 
> "there was at least one good parameter" to "there were no bad parameters".  
> Now, such a change should be accompanied by a change of name (e.g. to 
> bad_parm, also reversing the sense of the variable).
> 
> There was a previous invariant that get_pending_sizes would be called 
> after any parameters were parsed, either directly or via get_parm_info 
> because good_parm would be set, and with this patch, this invariant is no 
> longer maintained.  This is unsafe; you need to run this cleanup, whatever 
> you then return from c_parser_parms_list_declarator.  Consider for example 
> the testcase:
> 
> void foo(int n, int a[n], pid_t x);
> void bar() {}
> 
> (related to gcc.dg/noncompile/pr35444-*.c).  With your patch, this 
> testcase (which should be added to the next revision of the patch) gets an 
> ICE, because you lost the call to get_pending_sizes via get_parm_info.
> 
You are right! Yes, If append the above test case to the end of pr44517.c, it 
gets an ICE.
The above problem is fixed at the updated patch and the test case is moved to 
the directory gcc.dg/noncompile/.

>> +      error ("unknown type name %qE", token->value);
> 
> I don't think %qE is appropriate here if the token is not an identifier.
> 
Yes. But the problem is that token->id_kind is C_ID_ID and token->value is 
IDENTIFIER_NODE. At function c_lex_one_token, the token->id_kind is always be 
set to C_ID_ID if it is not the other identifier. Look at enum c_id_kind, 
C_ID_ID is "an ordinary identifier" and there is an "not an identifier" 
C_ID_NONE, but it never be really set. If token is CPP_NAME, and it was not 
declared as some type name, the token->id_kind should be set C_ID_NONE. But 
where should C_ID_ID be set?
I think that is the problem of c_lex_one_token, not the message format. I can't 
give a solution for that issue now. Does this patch can be committed firstly?

Retested on i686-pc-linux-gnu. Is it ok?

Thanks
Pearly

[-- Attachment #2: ChangeLog --]
[-- Type: text/plain, Size: 452 bytes --]

gcc/
2010-06-23  Shujing Zhao  <pearly.zhao@oracle.com>

	PR c/44517
	* c-parser.c (c_parser_parms_list_declarator): Return NULL if one of
	parameters are not good.
	(c_parser_parameter_declaration): Error unknown type name if the
	parameter can't start declaration specifiers.

gcc/testsuite/
2010-06-23  Shujing Zhao  <pearly.zhao@oracle.com>

	PR c/44517
	* gcc.dg/noncompile/pr44517.c: New.
	* gcc.dg/noncompile/990416-1.c: Adjust expected error.


[-- Attachment #3: pr44517.patch --]
[-- Type: text/x-patch, Size: 4674 bytes --]

Index: c-parser.c
===================================================================
--- c-parser.c	(revision 160889)
+++ c-parser.c	(working copy)
@@ -2706,7 +2706,7 @@ c_parser_parms_declarator (c_parser *par
 static struct c_arg_info *
 c_parser_parms_list_declarator (c_parser *parser, tree attrs)
 {
-  bool good_parm = false;
+  bool bad_parm = false;
   /* ??? Following the old parser, forward parameter declarations may
      use abstract declarators, and if no real parameter declarations
      follow the forward declarations then this is not diagnosed.  Also
@@ -2758,11 +2758,10 @@ c_parser_parms_list_declarator (c_parser
       /* Parse a parameter.  */
       struct c_parm *parm = c_parser_parameter_declaration (parser, attrs);
       attrs = NULL_TREE;
-      if (parm != NULL)
-	{
-	  good_parm = true;
-	  push_parm_decl (parm);
-	}
+      if (parm == NULL)
+	bad_parm = true;
+      else
+	push_parm_decl (parm);
       if (c_parser_next_token_is (parser, CPP_SEMICOLON))
 	{
 	  tree new_attrs;
@@ -2774,20 +2773,13 @@ c_parser_parms_list_declarator (c_parser
       if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN))
 	{
 	  c_parser_consume_token (parser);
-	  if (good_parm)
-	    return get_parm_info (false);
-	  else
+	  if (bad_parm)
 	    {
-	      struct c_arg_info *ret
-		= XOBNEW (&parser_obstack, struct c_arg_info);
-	      ret->parms = 0;
-	      ret->tags = 0;
-	      ret->types = 0;
-	      ret->others = 0;
-	      ret->pending_sizes = 0;
-	      ret->had_vla_unspec = 0;
-	      return ret;
+	      get_pending_sizes ();
+	      return NULL;
 	    }
+	  else
+	    return get_parm_info (false);
 	}
       if (!c_parser_require (parser, CPP_COMMA,
 			     "expected %<;%>, %<,%> or %<)%>"))
@@ -2802,20 +2794,13 @@ c_parser_parms_list_declarator (c_parser
 	  if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN))
 	    {
 	      c_parser_consume_token (parser);
-	      if (good_parm)
-		return get_parm_info (true);
-	      else
+	      if (bad_parm)
 		{
-		  struct c_arg_info *ret
-		    = XOBNEW (&parser_obstack, struct c_arg_info);
-		  ret->parms = 0;
-		  ret->tags = 0;
-		  ret->types = 0;
-		  ret->others = 0;
-		  ret->pending_sizes = 0;
-		  ret->had_vla_unspec = 0;
-		  return ret;
+		  get_pending_sizes ();
+		  return NULL;
 		}
+	      else
+		return get_parm_info (true);
 	    }
 	  else
 	    {
@@ -2843,8 +2828,12 @@ c_parser_parameter_declaration (c_parser
     {
       /* ??? In some Objective-C cases '...' isn't applicable so there
 	 should be a different message.  */
-      c_parser_error (parser,
-		      "expected declaration specifiers or %<...%>");
+      c_token *token = c_parser_peek_token (parser);
+      if (parser->error)
+	return NULL;
+      parser->error = true;
+      c_parser_set_source_position_from_token (token);
+      error ("unknown type name %qE", token->value);
       c_parser_skip_to_end_of_parameter (parser);
       return NULL;
     }
Index: testsuite/gcc.dg/noncompile/pr44517.c
===================================================================
--- testsuite/gcc.dg/noncompile/pr44517.c	(revision 0)
+++ testsuite/gcc.dg/noncompile/pr44517.c	(revision 0)
@@ -0,0 +1,11 @@
+/* PR c/44517: Improve diagnostic for misspelled typename in function declaration. */
+int foo(int x, pid_t y, long z, in t) {  /* { dg-error "unknown type name.*pid_t|unknown type name.*in" } */
+  return x + y + z + t;
+}
+
+int bar(int x, lon y, long z, ...){ /* { dg-error "unknown type name" { target *-*-* } 8 } */
+  return;
+}
+
+void foo(int n, int a[n], pid_t x); /* { dg-error "unknown type name" { target *-*-* } 12 } */
+void bar() {};
Index: testsuite/gcc.dg/noncompile/990416-1.c
===================================================================
--- testsuite/gcc.dg/noncompile/990416-1.c	(revision 160889)
+++ testsuite/gcc.dg/noncompile/990416-1.c	(working copy)
@@ -2,11 +2,11 @@ extern void *memcpy (void *, const void 
 typedef int word_type;
    
 static void
-copy_reg (unsigned int reg, frame_state *udata,	/* { dg-error "parse|syntax|expected" } */
-	  frame_state *target_udata)	/* { dg-error "expected" } */
+copy_reg (unsigned int reg, frame_state *udata,	/* { dg-error "unknown type name" } */
+	  frame_state *target_udata)	/* { dg-error "unknown type name" } */
 {  
-  word_type *preg = get_reg_addr (reg, udata, 0);	/* { dg-error "undeclared|function|without a cast" } */
-  word_type *ptreg = get_reg_addr (reg, target_udata, 0); /* { dg-error "undeclared|without a cast" } */
+  word_type *preg = ge_reg_addr (reg, udata, 0);
+  word_type *ptreg = ge_reg_addr (reg, target_udata, 0);
    
   memcpy (ptreg, preg, __builtin_dwarf_reg_size (reg));
 }