avoid warning for memset writing over multiple members (PR 95667)

Message ID e773e765-814a-a599-de9f-0ef3cbe477e2@gmail.com
State New
Headers show
Series
  • avoid warning for memset writing over multiple members (PR 95667)
Related show

Commit Message

Christophe Lyon via Gcc-patches June 18, 2020, 2:56 p.m.
In the recent fix to avoid false positives due to compute_objsize
(PR 95353)‚Äč where I removed the call to compute_builtin_object_size
when computing object sizes for calls to string functions like
strcpy, I kept it out of a misplaced abundance of caution when
doing the same for the more permissive memcpy and memset.  It
turned out to be a mistake because it triggered another false
positive warning in a case when the call wasn't made due to
the new structure.  Rather than continuing to rely on the function
(and continue to try to cope with its limitations) the attached
patch replaces the call with its own computation.  That keeps all
the logic in one place and avoids the limitations.

Like the patch for PR 95353, I tested this fix by building Glibc,
Binutils/GDB, and the kernel on x86_64-linux with no new warnings
(of course, that didn't prevent the false positive from turning
up in a Glibc build on nios2-linux-gnu).

Martin

PS This isn't meant to resolve the underlying problem with the PRE
transformation substituting one member for another with the same
offset.

PPS I have WIP patches for GCC 11 tweaking this area in a number
of ways, so this is just a narrow fix for the expected permissive
treatment of memset and other memory functions writing across
subobject boundaries.

Comments

Christophe Lyon via Gcc-patches June 18, 2020, 3:26 p.m. | #1
On Thu, 2020-06-18 at 08:56 -0600, Martin Sebor via Gcc-patches wrote:
> In the recent fix to avoid false positives due to compute_objsize

> (PR 95353) where I removed the call to compute_builtin_object_size

> when computing object sizes for calls to string functions like

> strcpy, I kept it out of a misplaced abundance of caution when

> doing the same for the more permissive memcpy and memset.  It

> turned out to be a mistake because it triggered another false

> positive warning in a case when the call wasn't made due to

> the new structure.  Rather than continuing to rely on the function

> (and continue to try to cope with its limitations) the attached

> patch replaces the call with its own computation.  That keeps all

> the logic in one place and avoids the limitations.

> 

> Like the patch for PR 95353, I tested this fix by building Glibc,

> Binutils/GDB, and the kernel on x86_64-linux with no new warnings

> (of course, that didn't prevent the false positive from turning

> up in a Glibc build on nios2-linux-gnu).

> 

> Martin

> 

> PS This isn't meant to resolve the underlying problem with the PRE

> transformation substituting one member for another with the same

> offset.

> 

> PPS I have WIP patches for GCC 11 tweaking this area in a number

> of ways, so this is just a narrow fix for the expected permissive

> treatment of memset and other memory functions writing across

> subobject boundaries.

OK
jeff

Patch

PR middle-end/95667 - unintended warning for memset writing across multiple members
PR middle-end/92814 - missing -Wstringop-overflow writing into a dynamically allocated flexible array member

gcc/ChangeLog:

	PR middle-end/95667
	PR middle-end/92814
	* builtins.c (compute_objsize): Remove call to
	compute_builtin_object_size and instead compute conservative sizes
	directly here.

gcc/testsuite/ChangeLog:

	PR middle-end/95667
	PR middle-end/92814
	* gcc.dg/Wstringop-overflow-25.c: Remove xfails.
	* gcc.dg/Wstringop-overflow-39.c: New test.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index caab188e81c..4754602e0ec 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -4009,31 +4009,6 @@  static bool
 compute_objsize (tree ptr, int ostype, access_ref *pref,
 		 bitmap *visited, const vr_values *rvals /* = NULL */)
 {
-  if (ostype == 0)
-    {
-      /* Use BOS only for raw memory functions like memcpy to get
-	 the size of the largest enclosing object.  */
-      tree off = NULL_TREE;
-      unsigned HOST_WIDE_INT size;
-      if (compute_builtin_object_size (ptr, ostype, &size, &pref->ref, &off))
-	{
-	  if (off)
-	    {
-	      offset_int offset = wi::to_offset (off);
-	      pref->offrng[0] += offset;
-	      pref->offrng[1] += offset;
-
-	      /* compute_builtin_object_size() returns the remaining
-		 size in PTR.  Add the offset to it to get the full
-		 size.  */
-	      pref->sizrng[0] = pref->sizrng[1] = size + offset;
-	    }
-	  else
-	    pref->sizrng[0] = pref->sizrng[1] = size;
-	  return true;
-	}
-    }
-
   const bool addr = TREE_CODE (ptr) == ADDR_EXPR;
   if (addr)
     ptr = TREE_OPERAND (ptr, 0);
@@ -4058,18 +4033,28 @@  compute_objsize (tree ptr, int ostype, access_ref *pref,
 
   if (code == COMPONENT_REF)
     {
+      tree field = TREE_OPERAND (ptr, 1);
+
       if (ostype == 0)
 	{
 	  /* For raw memory functions like memcpy bail if the size
 	     of the enclosing object cannot be determined.  */
-	  access_ref tmpref;
 	  tree ref = TREE_OPERAND (ptr, 0);
-	  if (!compute_objsize (ref, ostype, &tmpref, visited, rvals)
-	      || !tmpref.ref)
+	  if (!compute_objsize (ref, ostype, pref, visited, rvals)
+	      || !pref->ref)
 	    return false;
+
+	  /* Otherwise, use the size of the enclosing object and add
+	     the offset of the member to the offset computed so far.  */
+	  tree offset = byte_position (field);
+	  if (TREE_CODE (offset) != INTEGER_CST)
+	    return false;
+	  offset_int off = wi::to_offset (offset);
+	  pref->offrng[0] += off;
+	  pref->offrng[1] += off;
+	  return true;
 	}
 
-      tree field = TREE_OPERAND (ptr, 1);
       /* Bail if the reference is to the pointer itself (as opposed
 	 to what it points to).  */
       if (!addr && POINTER_TYPE_P (TREE_TYPE (field)))
@@ -4147,8 +4132,11 @@  compute_objsize (tree ptr, int ostype, access_ref *pref,
 	  orng[0] *= sz;
 	  orng[1] *= sz;
 
-	  if (TREE_CODE (eltype) == ARRAY_TYPE)
+	  if (ostype && TREE_CODE (eltype) == ARRAY_TYPE)
 	    {
+	      /* Execpt for the permissive raw memory functions which
+		 use the size of the whole object determined above,
+		 use the size of the referenced array.  */
 	      pref->sizrng[0] = pref->offrng[0] + orng[0] + sz;
 	      pref->sizrng[1] = pref->offrng[1] + orng[1] + sz;
 	    }
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-25.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-25.c
index 01807207acc..109a1dd9127 100644
--- a/gcc/testsuite/gcc.dg/Wstringop-overflow-25.c
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-25.c
@@ -370,14 +370,14 @@  NOIPA void test_strcpy_malloc_flexarray (void)
   size_t r_2_3 = UR (2, 3);
 
   T (char, S (0), r_0_1);
-  T (char, S (1), r_0_1);       // { dg-warning "\\\[-Wstringop-overflow" "pr92814" { xfail *-*-* } }
+  T (char, S (1), r_0_1);       // { dg-warning "\\\[-Wstringop-overflow" "pr92814" }
 
   T (char, S (0), r_1_2);
   T (char, S (1), r_1_2);
-  T (char, S (2), r_1_2);       // { dg-warning "\\\[-Wstringop-overflow" "pr92814" { xfail *-*-* } }
+  T (char, S (2), r_1_2);       // { dg-warning "\\\[-Wstringop-overflow" "pr92814" }
 
   T (char, S (0), r_2_3);
   T (char, S (2), r_2_3);
-  T (char, S (3), r_2_3);       // { dg-warning "\\\[-Wstringop-overflow" "pr92814" { xfail *-*-* } }
-  T (char, S (9), r_2_3);       // { dg-warning "\\\[-Wstringop-overflow" "pr92814" { xfail *-*-* } }
+  T (char, S (3), r_2_3);       // { dg-warning "\\\[-Wstringop-overflow" "pr92814" }
+  T (char, S (9), r_2_3);       // { dg-warning "\\\[-Wstringop-overflow" "pr92814" }
 }
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-39.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-39.c
new file mode 100644
index 00000000000..f83646a3721
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-39.c
@@ -0,0 +1,118 @@ 
+/* PR middle-end/95667 - unintended warning for memset writing across multiple
+   members
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+extern void sink (void*);
+
+struct S1 { char a[3], b[5]; };
+
+void warn_strcpy_s1 (void)
+{
+  struct S1 *p = __builtin_malloc (sizeof *p);
+  char s[] = "1234567";
+  __builtin_strcpy (p->a, s);         // { dg-warning "\\\[-Wstringop-overflow" }
+  sink (p);
+}
+
+void nowarn_memset_s1 (void)
+{
+  struct S1 *p = __builtin_malloc (sizeof *p);
+  __builtin_memset (p->a, 0, 8);      // { dg-bogus "\\\[-Wstringop-overflow" }
+  sink (p);
+}
+
+struct S2 { char a[2], b[2][2], c[3]; };
+
+void nowarn_memset_s2 (void)
+{
+  struct S2 *p = __builtin_malloc (sizeof *p);
+
+  __builtin_memset (p->a, 0, sizeof *p);
+  sink (p);
+
+  __builtin_memset (p->b, 0, 7);
+  sink (p);
+
+  __builtin_memset (&p->b[0], 0, 7);
+  sink (p);
+
+  __builtin_memset (&p->b[1], 0, 5);
+  sink (p);
+
+  __builtin_memset (&p->b[0][0], 0, 7);
+  sink (p);
+
+  __builtin_memset (&p->b[0][1], 0, 6);
+  sink (p);
+
+  __builtin_memset (&p->b[1][0], 0, 5);
+  sink (p);
+
+  __builtin_memset (&p->b[1][1], 0, 4);
+  sink (p);
+}
+
+void warn_memset_s2 (void)
+{
+  const unsigned n = sizeof (struct S2);
+  struct S2 *p = __builtin_malloc (n);
+
+  /* These should trigger -Wstringop-overflow rather than -Warray-bounds
+     but the main purpose of the test is to verify the absence of warnings
+     above so the exact warning for these overflwing calls isn't important
+     here.  */
+
+  __builtin_memset (p->a, 0, n + 1);  // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+  sink (p);
+
+  __builtin_memset (p->b, 0, 8);      // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+  sink (p);
+
+  __builtin_memset (&p->b[0], 0, 8);  // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+  sink (p);
+
+  __builtin_memset (&p->b[0][0], 0, 8); // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+  sink (p);
+
+  __builtin_memset (&p->b[1], 0, 6);  // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+  sink (p);
+
+  __builtin_memset (&p->b[0][1], 0, 7); // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+  sink (p);
+}
+
+void nowarn_vl_struct (unsigned n)
+{
+  if (n < 3 || 5 < n)
+    n = 3;
+
+  struct V { char a[3], b[n], c[7]; } v;
+
+  __builtin_memset (v.a, 0, 15);
+  sink (&v);
+
+  __builtin_memset (v.b, 0, 12);
+  sink (&v);
+
+  __builtin_memset (v.c, 0, 7);
+  sink (&v);
+
+  __builtin_memset (v.a, 0, 16);      // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+  sink (&v);
+
+  __builtin_memset (v.b, 0, 13);      // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+  sink (&v);
+
+  /* The &V.C argument is represented as a variable offset from
+     the beginning of the allocated object and there's no good
+     way to figure out from its variable offset that it's base
+     is the C member:
+       s.1_12 = __builtin_alloca_with_align (prephitmp_24, 8);
+       _9 = s.1_12 + prephitmp_27;
+       __builtin_memset (_9, 0, 2);
+  */
+
+  __builtin_memset (v.c, 0, 8);       // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" "pr?????" { xfail *-*-* } }
+  sink (&v);
+}