Patchwork detect unknown type names in declaration

login
register
mail settings
Submitter H.J. Lu
Date Nov. 13, 2010, 5:21 p.m.
Message ID <AANLkTim95pxeQ5B3ERo+0JXE_ZRD7wGKxErpNLeMz_=i@mail.gmail.com>
Download mbox | patch
Permalink /patch/71060/
State New
Headers show

Comments

H.J. Lu - Nov. 13, 2010, 5:21 p.m.
On Sat, Nov 13, 2010 at 8:51 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
> On 11/13/2010 05:25 PM, H.J. Lu wrote:
>>
>> There is one ICE:
>>
>> FAIL: gcc.dg/pr14963.c (internal compiler error)
>> FAIL: gcc.dg/pr14963.c (test for excess errors)
>>
>> It looks like a real bug.
>
> It is PR45062, it's just exposed by this patch because the problematic
> statement is parsed instead of skipped.  So the ICE is "correct", it's an
> improvement in coverage.
>


Like this?
Paolo Bonzini - Nov. 13, 2010, 5:22 p.m.
On 11/13/2010 06:21 PM, H.J. Lu wrote:
> On Sat, Nov 13, 2010 at 8:51 AM, Paolo Bonzini<bonzini@gnu.org>  wrote:
>> On 11/13/2010 05:25 PM, H.J. Lu wrote:
>>>
>>> There is one ICE:
>>>
>>> FAIL: gcc.dg/pr14963.c (internal compiler error)
>>> FAIL: gcc.dg/pr14963.c (test for excess errors)
>>>
>>> It looks like a real bug.
>>
>> It is PR45062, it's just exposed by this patch because the problematic
>> statement is parsed instead of skipped.  So the ICE is "correct", it's an
>> improvement in coverage.
>
> Like this?

I have no idea, I didn't investigate it much.

Maybe it's possible to avoid the TREE_LIST altogether, or use a VEC 
somewhere.  I'd leave it to Nathan, who said he'd fix it at the 
beginning of stage3. :)

Paolo
Nathan Froyd - Nov. 13, 2010, 5:34 p.m.
On Sat, Nov 13, 2010 at 09:21:15AM -0800, H.J. Lu wrote:
> On Sat, Nov 13, 2010 at 8:51 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
> > It is PR45062, it's just exposed by this patch because the problematic
> > statement is parsed instead of skipped.  So the ICE is "correct", it's an
> > improvement in coverage.
> >
> 
> 
> Like this?
> 
> -- 
> H.J.
> ---
> diff --git a/gcc/c-decl.c b/gcc/c-decl.c
> index c0d5a49..2b0c0e4 100644
> --- a/gcc/c-decl.c
> +++ b/gcc/c-decl.c
> @@ -4046,7 +4046,11 @@ start_decl (struct c_declarator *declarator,
> struct c_declspecs *declspecs,
>        if (ce->kind == cdk_function)
>  	{
>  	  tree args = ce->u.arg_info->parms;
> -	  for (; args; args = DECL_CHAIN (args))
> +	  /* ARGS may contain a mixture of DECLs and TREE_LISTs due to
> +	     invalid input, so be careful.  */
> +	  for (; args; args = (DECL_P (args)
> +			       ? DECL_CHAIN (args)
> +			       : TREE_CHAIN (args)))
>  	    {
>  	      tree type = TREE_TYPE (args);
>  	      if (type && INTEGRAL_TYPE_P (type)

I agree that this fixes the bug; I don't think it's the right fix.
Everyplace else we grovel through arg_info->parms, it's a chain of
DECLs...why not here?

The reason we're getting TREE_LISTs here is this bit of code in
grokparms:

  else if (arg_types && TREE_CODE (TREE_VALUE (arg_types)) == IDENTIFIER_NODE)
    {
      if (!funcdef_flag)
	pedwarn (input_location, 0, "parameter names (without types) in function declaration");

      arg_info->parms = arg_info->types;
      arg_info->types = 0;
      return 0;
    }

which is somewhat dubious (and would be invalid code if we had a more
statically typed IR).  I think the right fix is to zero out
arg_info->parms here if !funcdef_flag.  Like I said, I'll work on this
one, unless you beat me to it. :)

-Nathan
H.J. Lu - Nov. 13, 2010, 8:49 p.m.
On Sat, Nov 13, 2010 at 9:34 AM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Sat, Nov 13, 2010 at 09:21:15AM -0800, H.J. Lu wrote:
>> On Sat, Nov 13, 2010 at 8:51 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
>> > It is PR45062, it's just exposed by this patch because the problematic
>> > statement is parsed instead of skipped.  So the ICE is "correct", it's an
>> > improvement in coverage.
>> >
>>
>>
>> Like this?
>>
>> --
>> H.J.
>> ---
>> diff --git a/gcc/c-decl.c b/gcc/c-decl.c
>> index c0d5a49..2b0c0e4 100644
>> --- a/gcc/c-decl.c
>> +++ b/gcc/c-decl.c
>> @@ -4046,7 +4046,11 @@ start_decl (struct c_declarator *declarator,
>> struct c_declspecs *declspecs,
>>        if (ce->kind == cdk_function)
>>       {
>>         tree args = ce->u.arg_info->parms;
>> -       for (; args; args = DECL_CHAIN (args))
>> +       /* ARGS may contain a mixture of DECLs and TREE_LISTs due to
>> +          invalid input, so be careful.  */
>> +       for (; args; args = (DECL_P (args)
>> +                            ? DECL_CHAIN (args)
>> +                            : TREE_CHAIN (args)))
>>           {
>>             tree type = TREE_TYPE (args);
>>             if (type && INTEGRAL_TYPE_P (type)
>
> I agree that this fixes the bug; I don't think it's the right fix.
> Everyplace else we grovel through arg_info->parms, it's a chain of
> DECLs...why not here?
>
> The reason we're getting TREE_LISTs here is this bit of code in
> grokparms:
>
>  else if (arg_types && TREE_CODE (TREE_VALUE (arg_types)) == IDENTIFIER_NODE)
>    {
>      if (!funcdef_flag)
>        pedwarn (input_location, 0, "parameter names (without types) in function declaration");
>
>      arg_info->parms = arg_info->types;
>      arg_info->types = 0;
>      return 0;
>    }
>
> which is somewhat dubious (and would be invalid code if we had a more
> statically typed IR).  I think the right fix is to zero out
> arg_info->parms here if !funcdef_flag.  Like I said, I'll work on this
> one, unless you beat me to it. :)
>

No, I have no plan to work on it.

Patch

diff --git a/gcc/c-decl.c b/gcc/c-decl.c
index c0d5a49..2b0c0e4 100644
--- a/gcc/c-decl.c
+++ b/gcc/c-decl.c
@@ -4046,7 +4046,11 @@  start_decl (struct c_declarator *declarator,
struct c_declspecs *declspecs,
       if (ce->kind == cdk_function)
 	{
 	  tree args = ce->u.arg_info->parms;
-	  for (; args; args = DECL_CHAIN (args))
+	  /* ARGS may contain a mixture of DECLs and TREE_LISTs due to
+	     invalid input, so be careful.  */
+	  for (; args; args = (DECL_P (args)
+			       ? DECL_CHAIN (args)
+			       : TREE_CHAIN (args)))
 	    {
 	      tree type = TREE_TYPE (args);