[C] Fix pr44517

Submitted by Shujing Zhao on June 22, 2010, 7:05 a.m.

Details

Message ID 4C2060CC.3040401@oracle.com
State New
Headers show

Commit Message

Shujing Zhao June 22, 2010, 7:05 a.m.
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.

Bootstrapped and tested on i686-pc-linux-gnu with enabled c, objc and c++.
Is it ok?

Thanks
Pearly
gcc/
2010-06-21  Shujing Zhao  <pearly.zhao@oracle.com>

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

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

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

Comments

Manuel López-Ibáñez June 22, 2010, 7:40 a.m.
On 22 June 2010 09:05, Shujing Zhao <pearly.zhao@oracle.com> 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.
>

Great! One minor nit:

-		}
+		return false;
 	    }

This should be return NULL.

Don't need to send a new patch for this. Just wait for approval.

Manuel.
Shujing Zhao June 22, 2010, 9:04 a.m.
On 06/22/2010 03:40 PM, Manuel López-Ibáñez wrote:
> On 22 June 2010 09:05, Shujing Zhao <pearly.zhao@oracle.com> 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.
>>
> 
> Great! One minor nit:
> 
> -		}
> +		return false;
>  	    }
> 
> This should be return NULL.
> 
> Don't need to send a new patch for this. Just wait for approval.

Oops, sorry. Gcc didn't detect it, is it because both 'false' and 'NULL' are 0? 
is it a bug?

Thanks
Pearly
Manuel López-Ibáñez June 22, 2010, 9:07 a.m.
On 22 June 2010 11:04, Shujing Zhao <pearly.zhao@oracle.com> wrote:
> On 06/22/2010 03:40 PM, Manuel López-Ibáńez wrote:
>>
>> On 22 June 2010 09:05, Shujing Zhao <pearly.zhao@oracle.com> 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.
>>>
>>
>> Great! One minor nit:
>>
>> -               }
>> +               return false;
>>            }
>>
>> This should be return NULL.
>>
>> Don't need to send a new patch for this. Just wait for approval.
>
> Oops, sorry. Gcc didn't detect it, is it because both 'false' and 'NULL' are
> 0? is it a bug?

It isn't a bug. The behaviour should be the same. But for the sake of
clarity, we should return NULL for pointers and false only for bool.

Cheers,

Manuel.
Joseph S. Myers June 22, 2010, 12:55 p.m.
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.

> +      error ("unknown type name %qE", token->value);

I don't think %qE is appropriate here if the token is not an identifier.

Patch hide | download patch | download mbox

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 good_parm = true;
   /* ??? 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)
+	good_parm = false;
+      else
+	push_parm_decl (parm);
       if (c_parser_next_token_is (parser, CPP_SEMICOLON))
 	{
 	  tree new_attrs;
@@ -2777,17 +2776,7 @@  c_parser_parms_list_declarator (c_parser
 	  if (good_parm)
 	    return get_parm_info (false);
 	  else
-	    {
-	      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;
-	    }
+	    return NULL;
 	}
       if (!c_parser_require (parser, CPP_COMMA,
 			     "expected %<;%>, %<,%> or %<)%>"))
@@ -2805,17 +2794,7 @@  c_parser_parms_list_declarator (c_parser
 	      if (good_parm)
 		return get_parm_info (true);
 	      else
-		{
-		  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;
-		}
+		return false;
 	    }
 	  else
 	    {
@@ -2843,8 +2822,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/pr44517.c
===================================================================
--- testsuite/gcc.dg/pr44517.c	(revision 0)
+++ testsuite/gcc.dg/pr44517.c	(revision 0)
@@ -0,0 +1,10 @@ 
+/* PR c/44517 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+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" } */
+  return;
+}
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));
 }