Patchwork [C] Fix pr44517

login
register
mail settings
Submitter Shujing Zhao
Date June 22, 2010, 7:05 a.m.
Message ID <4C2060CC.3040401@oracle.com>
Download mbox | patch
Permalink /patch/56403/
State New
Headers show

Comments

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.
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

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));
 }