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

Message ID CAAgBjMn42wpBL4nvrDVhCo8B0B70V4wo5BKEH6JBF9_eSPYdkw@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni Feb. 15, 2017, 12:07 p.m.
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 ?

Thanks,
Prathamesh
2017-02-15  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

c/
	* gimple-parser.c (c_parser_parse_ssa_name): Emit diagnostic if same
	ssa version is used with multiple names.

testsuite/
	* gcc.dg/gimplefe-error-4.c: New test.

Comments

Richard Biener Feb. 16, 2017, 8:31 a.m. | #1
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

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)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);
        }
     }
 

Prathamesh Kulkarni Feb. 16, 2017, 9:05 a.m. | #2
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)

Patch hide | download patch | download mbox

diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
index d959877..2e163f4 100644
--- a/gcc/c/gimple-parser.c
+++ b/gcc/c/gimple-parser.c
@@ -672,29 +672,49 @@  c_parser_parse_ssa_name (c_parser *parser,
     }
   else
     {
+      /* Separate var name from version.  */
+      char *var_name = XNEWVEC (char, ver_offset + 1);
+      memcpy (var_name, token, ver_offset);
+      var_name[ver_offset] = '\0';
+      /* lookup for parent decl.  */
+      id = get_identifier (var_name);
+      tree parent = lookup_name (id);
+      if (! parent || parent == error_mark_node)
+	{
+	  c_parser_error (parser, "base variable or SSA name undeclared");
+	  XDELETEVEC (var_name);
+	  return error_mark_node;
+	}
       if (version < num_ssa_names)
 	name = ssa_name (version);
       if (! name)
 	{
-	  /* Separate var name from version.  */
-	  char *var_name = XNEWVEC (char, ver_offset + 1);
-	  memcpy (var_name, token, ver_offset);
-	  var_name[ver_offset] = '\0';
-	  /* lookup for parent decl.  */
-	  id = get_identifier (var_name);
-	  tree parent = lookup_name (id);
-	  XDELETEVEC (var_name);
-	  if (! parent || parent == error_mark_node)
-	    {
-	      c_parser_error (parser, "base variable or SSA name undeclared"); 
-	      return error_mark_node;
-	    }
 	  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);
 	}
+      else if (!SSA_NAME_IDENTIFIER (name))
+ 	{
+	  error_at (input_location, "ssa version %<%d%> used anonymously"
+		    " and in %<%s%>", version, var_name);
+	  XDELETEVEC (var_name);
+	  return error_mark_node;
+	}
+      else
+	{
+	  const char *ssaname = IDENTIFIER_POINTER (SSA_NAME_IDENTIFIER (name));
+	  if (strcmp (ssaname, var_name))
+	    {
+	      error_at (input_location, "ssa version %<%d%> used for"
+			" multiple names %<%s%>, %<%s%>", version,
+			ssaname, var_name);
+	      XDELETEVEC (var_name);
+	      return error_mark_node;
+	    }
+	}
+      XDELETEVEC (var_name);
     }
 
   return name;
diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-4.c b/gcc/testsuite/gcc.dg/gimplefe-error-4.c
new file mode 100644
index 0000000..a3c652e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gimplefe-error-4.c
@@ -0,0 +1,20 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgimple" } */
+
+void __GIMPLE () foo (int a)
+{
+  int t0;
+  int _1;
+
+  _1 = a;
+  t0_1 = a; /* { dg-error "used anonymously and in 't0'" } */
+}
+
+void __GIMPLE () bar (int a)
+{
+  int t0;
+  int t1;
+
+  t0_1 = a;
+  t1_1 = a; /* { dg-error "multiple names 't0', 't1'" } */
+}