diff mbox series

[committed] fix another ICE in MEM_REF formatting (PR 98578)

Message ID 12069778-e6af-6fb9-13d4-a3c555c70a29@gmail.com
State New
Headers show
Series [committed] fix another ICE in MEM_REF formatting (PR 98578) | expand

Commit Message

Martin Sebor Jan. 7, 2021, 9:29 p.m. UTC 
Fixing the ICE in MEM_REF formatting (or the enhancements that
came along with the fix) introduced another, ICE plus a plugin
test failure.  I have committed the attached simple patch to
fix both.

Martin

PS There are outstanding bugs to fix/improvements to be made
to the MEM_REF formatting (though hopefully no more ICEs) that
I found while testing the attached fix.  I xfailed the tests
and opened the referenced bugs to keep track of them.

Comments

Jakub Jelinek Jan. 7, 2021, 9:37 p.m. UTC | #1 
On Thu, Jan 07, 2021 at 02:29:50PM -0700, Martin Sebor via Gcc-patches wrote:
> --- a/gcc/c-family/c-pretty-print.c
> +++ b/gcc/c-family/c-pretty-print.c
> @@ -1844,22 +1844,25 @@ print_mem_ref (c_pretty_printer *pp, tree e)
>  	}
>      }
>  
> -  const tree access_type = TREE_TYPE (e);
> +  tree access_type = TREE_TYPE (e);
> +  if (TREE_CODE (access_type) == ARRAY_TYPE)
> +    access_type = TREE_TYPE (access_type);
>    tree arg_type = TREE_TYPE (TREE_TYPE (arg));
>    if (TREE_CODE (arg_type) == ARRAY_TYPE)
>      arg_type = TREE_TYPE (arg_type);

The array types can be multidimensional, are you sure you don't want
to use strip_array_types instead?

	Jakub
Martin Sebor Jan. 7, 2021, 11:24 p.m. UTC | #2 
On 1/7/21 2:37 PM, Jakub Jelinek wrote:
> On Thu, Jan 07, 2021 at 02:29:50PM -0700, Martin Sebor via Gcc-patches wrote:
>> --- a/gcc/c-family/c-pretty-print.c
>> +++ b/gcc/c-family/c-pretty-print.c
>> @@ -1844,22 +1844,25 @@ print_mem_ref (c_pretty_printer *pp, tree e)
>>   	}
>>       }
>>   
>> -  const tree access_type = TREE_TYPE (e);
>> +  tree access_type = TREE_TYPE (e);
>> +  if (TREE_CODE (access_type) == ARRAY_TYPE)
>> +    access_type = TREE_TYPE (access_type);
>>     tree arg_type = TREE_TYPE (TREE_TYPE (arg));
>>     if (TREE_CODE (arg_type) == ARRAY_TYPE)
>>       arg_type = TREE_TYPE (arg_type);
> 
> The array types can be multidimensional, are you sure you don't want
> to use strip_array_types instead?

Pretty sure.

access_type is used to figure out the element index and residual
byte offset into the argument, so that needs the size of the array.

Both access_type and arg_type are then checked for compatibility,
to decide if the type of the access needs to be included as a cast.
So there again I think the outer array bounds need to be preserved.

There are a few simple tests involving multidimensional VLAs but
a more involved example I just tried triggers another ICE, this
time in gimple_canonical_types_compatible_p.  Apparently
the default invocation of the function doesn't like mixed arrays
and scalars.  So clearly there's a whole bounty of ICEs here and
more to do.

Martin
diff mbox series

Patch

commit 178f0afce3611282170de380fcea9db9d6e3ff0c
Author: Martin Sebor <msebor@redhat.com>
Date:   Thu Jan 7 14:20:39 2021 -0700

    PR middle-end/98578 - ICE warning on uninitialized VLA access
    
    gcc/c-family/ChangeLog:
    
            PR middle-end/98578
            * c-pretty-print.c (print_mem_ref): Strip array from access type.
            Avoid assuming acces type's size is constant.  Correct condition
            guarding the printing of a parenthesis.
    
    gcc/testsuite/ChangeLog:
    
            PR middle-end/98578
            * gcc.dg/plugin/gil-1.c: Adjust expected output.
            * gcc.dg/uninit-pr98578.c: New test.

diff --git a/gcc/c-family/c-pretty-print.c b/gcc/c-family/c-pretty-print.c
index e963cf53091..87301a2091c 100644
--- a/gcc/c-family/c-pretty-print.c
+++ b/gcc/c-family/c-pretty-print.c
@@ -1844,22 +1844,25 @@  print_mem_ref (c_pretty_printer *pp, tree e)
 	}
     }
 
-  const tree access_type = TREE_TYPE (e);
+  tree access_type = TREE_TYPE (e);
+  if (TREE_CODE (access_type) == ARRAY_TYPE)
+    access_type = TREE_TYPE (access_type);
   tree arg_type = TREE_TYPE (TREE_TYPE (arg));
   if (TREE_CODE (arg_type) == ARRAY_TYPE)
     arg_type = TREE_TYPE (arg_type);
 
   if (tree access_size = TYPE_SIZE_UNIT (access_type))
-    {
-      /* For naturally aligned accesses print the nonzero offset
-	 in units of the accessed type, in the form of an index.
-	 For unaligned accesses also print the residual byte offset.  */
-      offset_int asize = wi::to_offset (access_size);
-      offset_int szlg2 = wi::floor_log2 (asize);
-
-      elt_idx = byte_off >> szlg2;
-      byte_off = byte_off - (elt_idx << szlg2);
-    }
+    if (TREE_CODE (access_size) == INTEGER_CST)
+      {
+	/* For naturally aligned accesses print the nonzero offset
+	   in units of the accessed type, in the form of an index.
+	   For unaligned accesses also print the residual byte offset.  */
+	offset_int asize = wi::to_offset (access_size);
+	offset_int szlg2 = wi::floor_log2 (asize);
+
+	elt_idx = byte_off >> szlg2;
+	byte_off = byte_off - (elt_idx << szlg2);
+      }
 
   /* True to include a cast to the accessed type.  */
   const bool access_cast = VOID_TYPE_P (arg_type)
@@ -1924,9 +1927,9 @@  print_mem_ref (c_pretty_printer *pp, tree e)
     }
   if (elt_idx != 0)
     {
-      if (byte_off == 0 && char_cast)
+      if (access_cast || char_cast)
 	pp_c_right_paren (pp);
-      pp_c_right_paren (pp);
+
       if (addr)
 	{
 	  pp_space (pp);
diff --git a/gcc/testsuite/gcc.dg/plugin/gil-1.c b/gcc/testsuite/gcc.dg/plugin/gil-1.c
index 4e8f535ba85..66872f07466 100644
--- a/gcc/testsuite/gcc.dg/plugin/gil-1.c
+++ b/gcc/testsuite/gcc.dg/plugin/gil-1.c
@@ -13,7 +13,7 @@  void test_2 (PyObject *obj)
 {
   Py_BEGIN_ALLOW_THREADS /* { dg-message "releasing the GIL here" } */
 
-  Py_INCREF (obj); /* { dg-warning "use of PyObject '\\*\\(obj\\)' without the GIL" } */
+  Py_INCREF (obj); /* { dg-warning "use of PyObject '\\*obj' without the GIL" } */
   Py_DECREF (obj);
 
   Py_END_ALLOW_THREADS
@@ -60,7 +60,7 @@  void test_5 (PyObject *obj)
 static void  __attribute__((noinline))
 called_by_test_6 (PyObject *obj)
 {
-  Py_INCREF (obj); /* { dg-warning "use of PyObject '\\*\\(obj\\)' without the GIL" } */
+  Py_INCREF (obj); /* { dg-warning "use of PyObject '\\*obj' without the GIL" } */
   Py_DECREF (obj);
 }
 
diff --git a/gcc/testsuite/gcc.dg/uninit-pr98578.c b/gcc/testsuite/gcc.dg/uninit-pr98578.c
new file mode 100644
index 00000000000..98d611757ab
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-pr98578.c
@@ -0,0 +1,110 @@ 
+/* PR middle-end/98578 - ICE warning on uninitialized VLA access
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+void* malloc (__SIZE_TYPE__);
+
+void T (int, ...);
+
+void vla_n (int n, int i)
+{
+  int a1[n];
+
+  /* a1[I] should be formatted as as a1[I] (or, for I == 0, perhaps
+     as *a1), but definitely not as *a1[I].  This is a bug in VLA
+     formatting.  */
+  T (a1[0]);        // { dg-warning "'a1\\\[0]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "'\\*a1\\\[0]' is used uninitialized" "spurious star" { target *-*-* } .-1 }
+  T (a1[1]);        // { dg-warning "a1\\\[1]' is used uninitialized" }
+  T (a1[i]);        // { dg-warning "a1\\\[i]' is used uninitialized" }
+}
+
+void vla_n_2 (int n, int i)
+{
+  int a2[n][2];
+
+  T (a2[0][0]);   // { dg-warning "a2\\\[0]\\\[0]' is used uninitialized" }
+  T (a2[2][1]);   // { dg-warning "a2\\\[2]\\\[1]' is used uninitialized" }
+  T (a2[3][i]);   // { dg-warning "a2\\\[3]\\\[i]' is used uninitialized" }
+  T (a2[i][0]);   // { dg-warning "a2\\\[i]\\\[0]' is used uninitialized" }
+  T (a2[i][i]);   // { dg-warning "a2\\\[i]\\\[i]' is used uninitialized" }
+}
+
+
+void vla_3_n (int n, int i)
+{
+  int a2[3][n];
+
+  T (a2[0][0]);     // { dg-warning "a2\\\[0]\\\[0]' is used uninitialized" }
+  T (a2[1][2]);     // { dg-warning "a2\\\[1]\\\[2]' is used uninitialized" }
+  T (a2[2][i]);     // { dg-warning "a2\\\[2]\\\[i]' is used uninitialized" }
+  T (a2[i][3]);     // { dg-warning "a2\\\[i]\\\[3]' is used uninitialized" }
+  T (a2[i][i]);     // { dg-warning "a2\\\[i]\\\[i]' is used uninitialized" }
+}
+
+
+void vla_n_n (int n, int i)
+{
+  int a2[n][n];
+
+  T (a2[0][0]);     // { dg-warning "a2\\\[0]\\\[0]' is used uninitialized" }
+  T (a2[4][5]);     // { dg-warning "a2\\\[4]\\\[5]' is used uninitialized" }
+  T (a2[6][i]);     // { dg-warning "a2\\\[6]\\\[i]' is used uninitialized" }
+  T (a2[i][7]);     // { dg-warning "a2\\\[i]\\\[7]' is used uninitialized" }
+  T (a2[i][i]);     // { dg-warning "a2\\\[i]\\\[i]' is used uninitialized" }
+}
+
+
+void char_ptr_n (int n, int i)
+{
+  char *p = malloc (n);
+
+  T (p[0]);         // { dg-warning "'\\\*p' is used uninitialized" }
+  T (p[1]);         // { dg-warning "'p\\\[1]' is used uninitialized" }
+  T (p[i]);         // { dg-warning "'p\\\[i]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "is used uninitialized" "POINTER_PLUS_EXPR" { target *-*-* } .-1 }
+}
+
+
+void int_ptr_n (int n, int i)
+{
+  int *p = malloc (n);
+
+  T (p[0]);         // { dg-warning "'\\\*p' is used uninitialized" }
+  T (p[1]);         // { dg-warning "'p\\\[1]' is used uninitialized" }
+  T (p[i]);         // { dg-warning "'p\\\[i]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "is used uninitialized" "POINTER_PLUS_EXPR" { target *-*-* } .-1 }
+}
+
+
+void int_arr_ptr_n (int n, int i)
+{
+  int (*p)[n] = malloc (n);
+
+  T ((*p)[0]);      // { dg-warning "\\(\\*p\\)\\\[0]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[0]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+  T ((*p)[1]);      // { dg-warning "\\(\\*p\\)\\\[1]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[1]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+  T ((*p)[i]);      // { dg-warning "\\(\\*p\\)\\\[i]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[i]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+}
+
+
+void int_arr_ptr_n_n (int n, int i)
+{
+  int (*p)[n][n] = malloc (n);
+
+  T ((*p)[0][0]);   // { dg-warning "\\(\\*p\\)\\\[0]\\\[0]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[0]\\\[0]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+  T ((*p)[1][2]);   // { dg-warning "\\(\\*p\\)\\\[1]\\\[2]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[1]\\\[2]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+  T ((*p)[0][i]);   // { dg-warning "\\(\\*p\\)\\\[0]\\\[i]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[0]\\\[i]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+  T ((*p)[3][i]);   // { dg-warning "\\(\\*p\\)\\\[3]\\\[i]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[3]\\\[i]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+  T ((*p)[i][i]);   // { dg-warning "\\(\\*p\\)\\\[i]\\\[i]' is used uninitialized" "pr98587" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[i]\\\[i]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+
+  T ((*p)[i][i + 1]); // { dg-warning "\\(\\*p\\)\\\[i]\\\[i \\+ 1]' is used uninitialized" "pr98588" { xfail *-*-* } }
+                    // { dg-warning "\\*p\\\[i]\\\[<unknown>]' is used uninitialized" "missing parens" { target *-*-* } .-1 }
+}