[GIMPLE,FE] avoid ICE with same ssa version number for multiple names

On Wed, 15 Feb 2017, Prathamesh Kulkarni wrote:

> Hi,
> For the following (invalid) test-case:
> 
> void __GIMPLE () foo (int a)
> {
>   int t0;
>   int _1;
>   _1 = a;
>   t0_1 = a;
> }
> 
> results in following ICE:
> gimplefe-error-4.c: In function ‘foo’:
> gimplefe-error-4.c:20:1: error: SSA_NAME_DEF_STMT is wrong
>  }
>  ^
> Expected definition statement:
> _1 = a_2(D);
> 
> Actual definition statement:
> _1 = a_2(D);
> gimplefe-error-4.c:20:1: internal compiler error: verify_ssa failed
> 0xe1458b verify_ssa(bool, bool)
> ../../gcc/gcc/tree-ssa.c:1184
> 0xb0d1ed execute_function_todo
> ../../gcc/gcc/passes.c:1973
> 0xb0dad5 execute_todo
> ../../gcc/gcc/passes.c:2016
> 
> The reason for ICE is that in c_parser_parse_ssa_name, ssa_name (1)
> returns tree node for _1, and "t0_1" gets replaced by "_1"
> resulting in multiple definitions for _1.
> 
> The attached patch checks if multiple ssa names have same version
> number and emits a diagnostic in that case, for the above case:
> gimplefe-error-4.c: In function ‘foo’:
> gimplefe-error-4.c:10:3: error: ssa version ‘1’ used anonymously and in ‘t0’
>    t0_1 = a;
>    ^~~~
> 
> OK to commit after bootstrap+test ?

Hmm, I'd rather (if at all -- I consider these kind of verification errors
appropriate for valid GIMPLE FE input) do sth like


basically at the point we emit a SSA def diagnose existing ones.
Should be split out into a verify_def () function, and the diagnostic
should be more helpful of course.

Richard.

> 
> Thanks,
> Prathamesh
>

On 16 February 2017 at 14:01, Richard Biener <rguenther@suse.de> wrote:
> On Wed, 15 Feb 2017, Prathamesh Kulkarni wrote:
>
>> Hi,
>> For the following (invalid) test-case:
>>
>> void __GIMPLE () foo (int a)
>> {
>>   int t0;
>>   int _1;
>>   _1 = a;
>>   t0_1 = a;
>> }
>>
>> results in following ICE:
>> gimplefe-error-4.c: In function ‘foo’:
>> gimplefe-error-4.c:20:1: error: SSA_NAME_DEF_STMT is wrong
>>  }
>>  ^
>> Expected definition statement:
>> _1 = a_2(D);
>>
>> Actual definition statement:
>> _1 = a_2(D);
>> gimplefe-error-4.c:20:1: internal compiler error: verify_ssa failed
>> 0xe1458b verify_ssa(bool, bool)
>> ../../gcc/gcc/tree-ssa.c:1184
>> 0xb0d1ed execute_function_todo
>> ../../gcc/gcc/passes.c:1973
>> 0xb0dad5 execute_todo
>> ../../gcc/gcc/passes.c:2016
>>
>> The reason for ICE is that in c_parser_parse_ssa_name, ssa_name (1)
>> returns tree node for _1, and "t0_1" gets replaced by "_1"
>> resulting in multiple definitions for _1.
>>
>> The attached patch checks if multiple ssa names have same version
>> number and emits a diagnostic in that case, for the above case:
>> gimplefe-error-4.c: In function ‘foo’:
>> gimplefe-error-4.c:10:3: error: ssa version ‘1’ used anonymously and in ‘t0’
>>    t0_1 = a;
>>    ^~~~
>>
>> OK to commit after bootstrap+test ?
>
> Hmm, I'd rather (if at all -- I consider these kind of verification errors
> appropriate for valid GIMPLE FE input) do sth like
>
> Index: gcc/c/gimple-parser.c
> ===================================================================
> --- gcc/c/gimple-parser.c       (revision 245501)
> +++ gcc/c/gimple-parser.c       (working copy)
> @@ -315,6 +315,12 @@ c_parser_gimple_statement (c_parser *par
>           else if (! FLOAT_TYPE_P (TREE_TYPE (lhs.value))
>                    && FLOAT_TYPE_P (TREE_TYPE (rhs.value)))
>             code = FIX_TRUNC_EXPR;
> +         if (TREE_CODE (lhs.value) == SSA_NAME
> +             && SSA_NAME_DEF_STMT (lhs.value))
> +           {
> +             c_parser_error (parser, "duplicate definition of SSA name");
> +             /* point at previous definition, do not emit stmt */
> +           }
>           assign = gimple_build_assign (lhs.value, code, rhs.value);
>           gimple_seq_add_stmt (seq, assign);
>           gimple_set_location (assign, loc);
> @@ -347,6 +353,13 @@ c_parser_gimple_statement (c_parser *par
>        rhs = c_parser_gimple_unary_expression (parser);
>        if (rhs.value != error_mark_node)
>         {
> +         if (TREE_CODE (lhs.value) == SSA_NAME
> +             && SSA_NAME_DEF_STMT (lhs.value))
> +           {
> +             c_parser_error (parser, "duplicate definition of SSA name");
> +             /* point at previous definition, do not emit stmt */
> +           }
> +
>           assign = gimple_build_assign (lhs.value, rhs.value);
>           gimple_set_location (assign, loc);
>           gimple_seq_add_stmt (seq, assign);
> @@ -420,6 +433,13 @@ c_parser_gimple_statement (c_parser *par
>    if (lhs.value != error_mark_node
>        && rhs.value != error_mark_node)
>      {
> +         if (TREE_CODE (lhs.value) == SSA_NAME
> +             && SSA_NAME_DEF_STMT (lhs.value))
> +           {
> +             c_parser_error (parser, "duplicate definition of SSA name");
> +             /* point at previous definition, do not emit stmt */
> +           }
> +
>        assign = gimple_build_assign (lhs.value, rhs.value);
>        gimple_seq_add_stmt (seq, assign);
>        gimple_set_location (assign, loc);
> @@ -692,8 +712,7 @@ c_parser_parse_ssa_name (c_parser *parse
>           if (VECTOR_TYPE_P (TREE_TYPE (parent))
>               || TREE_CODE (TREE_TYPE (parent)) == COMPLEX_TYPE)
>             DECL_GIMPLE_REG_P (parent) = 1;
> -         name = make_ssa_name_fn (cfun, parent,
> -                                  gimple_build_nop (), version);
> +         name = make_ssa_name_fn (cfun, parent, NULL, version);
>         }
>      }
>
>
> basically at the point we emit a SSA def diagnose existing ones.
> Should be split out into a verify_def () function, and the diagnostic
> should be more helpful of course.
Hi Richard,
Um IIUC, the issue is not about multiple definitions but when multiple names
are used for same version, we pick the first version.
For eg, the following invalid test-case is accepted by FE, and would not
get caught by gimple-verifiers because the FE generates valid gimple
but does not match the
source.

int __GIMPLE () foo (int a)
{
  int t0;
  int _1;

  _1 = a;
  return t0_1;
}

the ssa pass dump shows:
int __GIMPLE ()
foo (int a)
{
  int _1;

  bb_2:
  _1 = a_2(D);
  return _1;

}

This happens because c_parser_parse_ssa_name() calls lookup_name (1)
and since we have anonymous ssa
name associated with version number 1  that is returned, and t0_1 gets
replaced by _1 in return stmt.
The patch would reject the above test-case.

Thanks,
Prathamesh
>
> Richard.
>
>>
>> Thanks,
>> Prathamesh
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)