tweak component_ref_size to extend -Wzero-length-array-bounds and not ICE (PR 92373)

Message ID 2b4af17c-9214-9922-3a37-6224e8f4971f@gmail.com
State New
Headers show
Series
  • tweak component_ref_size to extend -Wzero-length-array-bounds and not ICE (PR 92373)
Related show

Commit Message

Martin Sebor Nov. 6, 2019, midnight
After considering more instances of the enhanced -Warray-bounds
and the new -Wzero-length-array-bounds warnings I realized that
there are some where the former is being issued for zero-length
arrays but where the latter would be more appropriate.

The attached change tweaks the logic in component_ref_size to
have some of the latter instances replace some of the former,
effectively relaxing -Warray-bounds a bit.

-Wzero-length-array-bounds triggers for accesses to zero-length
arrays with non-negative indices, interior or otherwise, that
are within the bounds of the largest enclosing object.  All other
accesses are diagnosed via -Warray-bounds.

The change replaces all instances of -Warray-bounds in Glibc with
-Wzero-length-array-bounds.  In the kernel, it similarly turns
the vast majority of distinct -Warray-bounds warnings (56) into
-Wzero-length-array-bounds, leaving only four -Warray-bounds.

This should make adopting GCC 10 much easier for low-level code
that relies on dangerous "tricks" involving zero-length arrays
without sacrificing the ability to detect bugs in other projects.

While making the change I also removed an incorrect assumption
I introduced last week that was causing an ICE (as reported in
PR 92373).

Tested on x86_64-linux.

Martin

Comments

Jeff Law Nov. 6, 2019, 12:41 a.m. | #1
On 11/5/19 5:00 PM, Martin Sebor wrote:
> After considering more instances of the enhanced -Warray-bounds

> and the new -Wzero-length-array-bounds warnings I realized that

> there are some where the former is being issued for zero-length

> arrays but where the latter would be more appropriate.

> 

> The attached change tweaks the logic in component_ref_size to

> have some of the latter instances replace some of the former,

> effectively relaxing -Warray-bounds a bit.

> 

> -Wzero-length-array-bounds triggers for accesses to zero-length

> arrays with non-negative indices, interior or otherwise, that

> are within the bounds of the largest enclosing object.  All other

> accesses are diagnosed via -Warray-bounds.

> 

> The change replaces all instances of -Warray-bounds in Glibc with

> -Wzero-length-array-bounds.  In the kernel, it similarly turns

> the vast majority of distinct -Warray-bounds warnings (56) into

> -Wzero-length-array-bounds, leaving only four -Warray-bounds.

> 

> This should make adopting GCC 10 much easier for low-level code

> that relies on dangerous "tricks" involving zero-length arrays

> without sacrificing the ability to detect bugs in other projects.

> 

> While making the change I also removed an incorrect assumption

> I introduced last week that was causing an ICE (as reported in

> PR 92373).

> 

> Tested on x86_64-linux.

> 

> Martin

> 

> gcc-92373.diff

> 

> PR tree-optimization/92373 - ICE in -Warray-bounds on access to member array in an initialized char buffer

> 

> gcc/testsuite/ChangeLog:

> 

> 	PR tree-optimization/92373

> 	* gcc.dg/Warray-bounds-55.c: New test.

> 	* gcc.dg/Wzero-length-array-bounds-2.c: New test.

> 

> gcc/ChangeLog:

> 

> 	PR tree-optimization/92373

> 	* tree.c (component_ref_size): Only consider initializers of objects

> 	of matching struct types.

> 	Return null for for instances of interior zero-length arrays.

OK
jeff

Patch

PR tree-optimization/92373 - ICE in -Warray-bounds on access to member array in an initialized char buffer

gcc/testsuite/ChangeLog:

	PR tree-optimization/92373
	* gcc.dg/Warray-bounds-55.c: New test.
	* gcc.dg/Wzero-length-array-bounds-2.c: New test.

gcc/ChangeLog:

	PR tree-optimization/92373
	* tree.c (component_ref_size): Only consider initializers of objects
	of matching struct types.
	Return null for for instances of interior zero-length arrays.

Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 277853)
+++ gcc/tree.c	(working copy)
@@ -13635,6 +13635,8 @@  component_ref_size (tree ref, bool *interior_zero_
 	return NULL_TREE;
 
       base = TREE_OPERAND (ref, 0);
+      while (TREE_CODE (base) == COMPONENT_REF)
+	base = TREE_OPERAND (base, 0);
       baseoff = tree_to_poly_int64 (byte_position (TREE_OPERAND (ref, 1)));
     }
 
@@ -13656,27 +13658,28 @@  component_ref_size (tree ref, bool *interior_zero_
 
   memsize = NULL_TREE;
 
-  /* MEMBER is a true flexible array member.  Compute its size from
-     the initializer of the BASE object if it has one.  */
-  if (tree init = DECL_P (base) ? DECL_INITIAL (base) : NULL_TREE)
-    {
-      init = get_initializer_for (init, member);
-      if (init)
-	{
-	  memsize = TYPE_SIZE_UNIT (TREE_TYPE (init));
-	  if (tree refsize = TYPE_SIZE_UNIT (reftype))
-	    {
-	      /* Use the larger of the initializer size and the tail
-		 padding in the enclosing struct.  */
-	      poly_int64 rsz = tree_to_poly_int64 (refsize);
-	      rsz -= baseoff;
-	      if (known_lt (tree_to_poly_int64 (memsize), rsz))
-		memsize = wide_int_to_tree (TREE_TYPE (memsize), rsz);
-	    }
+  if (typematch)
+    /* MEMBER is a true flexible array member.  Compute its size from
+       the initializer of the BASE object if it has one.  */
+    if (tree init = DECL_P (base) ? DECL_INITIAL (base) : NULL_TREE)
+      {
+	init = get_initializer_for (init, member);
+	if (init)
+	  {
+	    memsize = TYPE_SIZE_UNIT (TREE_TYPE (init));
+	    if (tree refsize = TYPE_SIZE_UNIT (reftype))
+	      {
+		/* Use the larger of the initializer size and the tail
+		   padding in the enclosing struct.  */
+		poly_int64 rsz = tree_to_poly_int64 (refsize);
+		rsz -= baseoff;
+		if (known_lt (tree_to_poly_int64 (memsize), rsz))
+		  memsize = wide_int_to_tree (TREE_TYPE (memsize), rsz);
+	      }
 
-	  baseoff = 0;
-	}
-    }
+	    baseoff = 0;
+	  }
+      }
 
   if (!memsize)
     {
@@ -13689,7 +13692,7 @@  component_ref_size (tree ref, bool *interior_zero_
 	    /* The size of a flexible array member of an extern struct
 	       with no initializer cannot be determined (it's defined
 	       in another translation unit and can have an initializer
-	       witth an arbitrary number of elements).  */
+	       with an arbitrary number of elements).  */
 	    return NULL_TREE;
 
 	  /* Use the size of the base struct or, for interior zero-length
@@ -13696,10 +13699,12 @@  component_ref_size (tree ref, bool *interior_zero_
 	     arrays, the size of the enclosing type.  */
 	  memsize = TYPE_SIZE_UNIT (bt);
 	}
-      else
+      else if (DECL_P (base))
 	/* Use the size of the BASE object (possibly an array of some
 	   other type such as char used to store the struct).  */
 	memsize = DECL_SIZE_UNIT (base);
+      else
+	return NULL_TREE;
     }
 
   /* If the flexible array member has a known size use the greater
Index: gcc/testsuite/gcc.dg/Warray-bounds-55.c
===================================================================
--- gcc/testsuite/gcc.dg/Warray-bounds-55.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Warray-bounds-55.c	(working copy)
@@ -0,0 +1,28 @@ 
+/* PR middle-end/92373 - ICE in -Warray-bounds on access to member array
+   in an initialized char buffer
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+void sink (void*);
+
+struct S
+{
+  char data[1];
+};
+
+char a[6] = { };
+
+int f (void)
+{
+  struct S *p = (struct S*)a;
+  return p->data[4];
+
+}
+
+void g (void)
+{
+  char b[6] = { };
+  struct S *p = (struct S*)b;
+  p->data[4] = 0;
+  sink (p);
+}
Index: gcc/testsuite/gcc.dg/Wzero-length-array-bounds-2.c
===================================================================
--- gcc/testsuite/gcc.dg/Wzero-length-array-bounds-2.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Wzero-length-array-bounds-2.c	(working copy)
@@ -0,0 +1,125 @@ 
+/* Test to verify that -Wzero-length-bounds and not -Warray-bounds is
+   issued for accesses to interior zero-length array members that are
+   within the bounds of the enclosing struct.
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+void sink (void*);
+
+struct A { int i; };
+struct B { int j; struct A a[0]; };
+
+struct C
+{
+  struct B b1;
+  struct B b2;
+};
+
+
+void test_B_ref (struct B *p)
+{
+  // References with negative indices are always diagnosed by -Warray-bounds
+  // even though they could be considered the same as past the end accesses
+  // to trailing zero-length arrays.
+  p->a[-1].i = 0;       // { dg-warning "\\\[-Warray-bounds" }
+  p->a[ 0].i = 0;
+  p->a[ 1].i = 0;
+  sink (p);
+
+  p[1].a[-1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  p[1].a[ 0].i = 0;
+  p[1].a[ 1].i = 0;
+}
+
+
+void test_C_ref (struct C *p)
+{
+  p->b1.a[-1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b1.a[ 0].i = 0;     // { dg-warning "\\\[-Wzero-length-bounds" }
+  p->b1.a[ 1].i = 0;     // { dg-warning "\\\[-Wzero-length-bounds" }
+
+  // Accesses to trailing zero-length arrays are not diagnosed (should
+  // they be?)
+  p->b2.a[ 0].i = 0;
+  p->b2.a[ 9].i = 0;
+}
+
+
+void test_C_decl (void)
+{
+  struct C c, *p = &c;
+
+  p->b1.a[-1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+
+  // c.b1.a[0].i overlaps c.b2.j.
+  p->b1.a[ 0].i = 0;     // { dg-warning "\\\[-Wzero-length-bounds" }
+
+  // c.b1.a[1].i is past the end of c...
+  p->b1.a[ 1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+
+  // ...and so are references to all elements of c.b2.a
+  p->b2.a[ 0].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b2.a[ 1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+}
+
+
+char cbuf1[1 * sizeof (struct C)];
+char cbuf2[2 * sizeof (struct C)] = { };
+
+void test_C_global_buf (void)
+{
+  struct C *p = (struct C*)&cbuf1;
+
+  p->b1.a[-1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b1.a[ 0].i = 0;     // { dg-warning "\\\[-Wzero-length-bounds" }
+  p->b1.a[ 1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+
+  p->b2.a[ 0].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b2.a[ 1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+
+  p = (struct C*)&cbuf2;
+  p->b1.a[-1].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b1.a[ 0].i = 0;     // { dg-warning "\\\[-Wzero-length-bounds" }
+  p->b1.a[ 1].i = 0;     // { dg-warning "\\\[-Wzero-length-bounds" }
+  sink (p);
+
+  p->b2.a[ 0].i = 0;
+  p->b2.a[ 1].i = 0;
+  p->b2.a[ 2].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b2.a[ 3].i = 0;     // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+}
+
+
+void test_C_local_buf (void)
+{
+  char cbuf1[1 * sizeof (struct C)] = "";
+  char cbuf2[2 * sizeof (struct C)] = { };
+
+  struct C *p = (struct C*)&cbuf1;
+
+  p->b1.a[-1].i = 1;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b1.a[ 0].i = 2;     // { dg-warning "\\\[-Wzero-length-bounds" }
+  p->b1.a[ 1].i = 3;     // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+
+  p->b2.a[ 0].i = 4;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b2.a[ 1].i = 5;     // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+
+  p = (struct C*)&cbuf2;
+  p->b1.a[-1].i = 6;     // { dg-warning "\\\[-Warray-bounds" }
+  p->b1.a[ 0].i = 7;     // { dg-warning "\\\[-Wzero-length-bounds" }
+  p->b1.a[ 1].i = 8;     // { dg-warning "\\\[-Wzero-length-bounds" }
+  sink (p);
+
+  p->b2.a[ 0].i = 9;
+  p->b2.a[ 1].i = 10;
+  p->b2.a[ 2].i = 11;    // { dg-warning "\\\[-Warray-bounds" }
+  p->b2.a[ 3].i = 12;    // { dg-warning "\\\[-Warray-bounds" }
+  sink (p);
+}