public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] adjust object size computation for union accesses and PHIs (PR 92765)
@ 2020-01-15 13:25 Martin Sebor
  2020-01-15 21:15 ` Jeff Law
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Martin Sebor @ 2020-01-15 13:25 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

The strcmp optimization newly introduced in GCC 10 relies on
the size of the smallest referenced array object to determine
whether the function can return zero.  When the size of
the object is smaller than the length of the other string
argument the optimization folds the equality to false.

The bug report has identified a couple of problems here:
1) when the access to the array object is via a pointer to
a (possibly indirect) member of a union, in GIMPLE the pointer
may actually point to a different member than the one in
the original source code.  Thus the size of the array may
appear to be smaller than in the source code which can then
result in the optimization being invalid.
2) when the pointer in the access may point to two or more
arrays of different size (i.e., it's the result of a PHI),
assuming it points to the smallest of them can also lead
to an incorrect result when the optimization is applied.

The attached patch adjusts the optimization to 1) avoid making
any assumptions about the sizes of objects accessed via union
types, and b) use the size of the largest object in PHI nodes.

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-92765.diff --]
[-- Type: text/x-patch, Size: 5119 bytes --]

PR tree-optimization/92765 - wrong code for strcmp of a union member

gcc/testsuite/ChangeLog:

	PR tree-optimization/92765
	* gcc.dg/strlenopt-92.c: New test.

gcc/ChangeLog:

	PR tree-optimization/92765
	* tree-ssa-strlen.c (component_ref_via_union_p): New function.
	(determine_min_objsize): Call it.  Use the maximum object size
	for PHI arguments.

diff --git a/gcc/testsuite/gcc.dg/strlenopt-92.c b/gcc/testsuite/gcc.dg/strlenopt-92.c
new file mode 100644
index 00000000000..44ad5b88854
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/strlenopt-92.c
@@ -0,0 +1,94 @@
+/* PR tree-optimization/92765 - wrong code for strcmp of a union member
+   { dg-do run }
+   { dg-options "-O2 -Wall" } */
+
+#include "strlenopt.h"
+
+extern void free (void*);
+extern void* calloc (size_t, size_t);
+extern void* malloc (size_t);
+
+
+/* Test from comment #0.  */
+struct S0 {
+  char a[2];
+};
+
+union U0 {
+  char b[4];
+  struct S0 s;
+};
+
+union U0 u0;
+union U0 *p0 = &u0;
+
+int test_0 ()
+{
+  u0.b[0] = 'a';
+  u0.b[1] = 'b';
+  u0.b[2] = '\0';
+
+  int x = memcmp (p0->s.a, "x", 2);
+
+  if (strcmp (p0->b, "ab"))
+    abort ();
+
+  return x;
+}
+
+
+/* Test from comment #6.  */
+union U1 { struct S1 { char a[2]; char b[2]; char c[2]; } s; char d[6]; } u1;
+
+__attribute__((noipa)) void
+barrier (char *p)
+{
+  asm volatile ("" : : "g" (p) : "memory");
+}
+
+__attribute__((noipa)) void
+test_1 (union U1 *x)
+{
+  char *p = (char *) &x->s.b;
+  barrier (p);
+  if (strcmp (&x->d[2], "cde"))
+    abort ();
+}
+
+/* Test from comment #7.  */
+
+__attribute__((noipa)) int
+barrier_copy (char *x, int y)
+{
+  asm volatile ("" : : "g" (x), "g" (y) : "memory");
+  if (y == 0)
+    strcpy (x, "abcd");
+  return y;
+}
+
+__attribute__((noipa)) char *
+test_2 (int x)
+{
+  char *p;
+  if (x)
+    p = malloc (2);
+  else
+    p = calloc (16, 1);
+  if (barrier_copy (p, x))
+    return p;
+  if (strcmp (p, "abcd") != 0)
+    abort ();
+  return p;
+}
+
+
+int main (void)
+{
+  test_0 ();
+
+  strcpy (u1.d, "abcde");
+  test_1 (&u1);
+
+  free (test_2 (0));
+  free (test_2 (1));
+}
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index ad9e98973b1..f4b6aadae47 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -4087,6 +4087,29 @@ compute_string_length (int idx)
   return string_leni;
 }
 
+/* Returns true if REF is a reference to a member of a union type,
+   or a member of such a type (traversing all references along
+   the path).  Used to avoid making assumptions about accesses
+   to members that could also be accessed by other members of
+   incompatible types.  */
+
+static bool
+component_ref_via_union_p (tree ref)
+{
+  if (TREE_CODE (ref) == ADDR_EXPR)
+    ref = TREE_OPERAND (ref, 0);
+
+  while (TREE_CODE (ref) == MEM_REF || handled_component_p (ref))
+    {
+      tree type = TREE_TYPE (ref);
+      if (TREE_CODE (type) == UNION_TYPE)
+	return true;
+      ref = TREE_OPERAND (ref, 0);
+    }
+
+  return false;
+}
+
 /* Determine the minimum size of the object referenced by DEST expression
    which must have a pointer type.
    Return the minimum size of the object if successful or HWI_M1U when
@@ -4099,14 +4122,18 @@ determine_min_objsize (tree dest)
 
   init_object_sizes ();
 
-  if (compute_builtin_object_size (dest, 2, &size))
-    return size;
-
   /* Try to determine the size of the object through the RHS
      of the assign statement.  */
   if (TREE_CODE (dest) == SSA_NAME)
     {
       gimple *stmt = SSA_NAME_DEF_STMT (dest);
+
+      /* Determine the size of the largest object when DEST refers
+	 to two or more via a PHI, otherwise the smallest.  */
+      int ostype = gimple_code (stmt) == GIMPLE_PHI ? 0 : 2;
+      if (compute_builtin_object_size (dest, ostype, &size))
+	return size;
+
       if (!is_gimple_assign (stmt))
 	return HOST_WIDE_INT_M1U;
 
@@ -4118,6 +4145,10 @@ determine_min_objsize (tree dest)
       return determine_min_objsize (dest);
     }
 
+  /* Try to determine the size of the referenced object itself.  */
+  if (compute_builtin_object_size (dest, 2, &size))
+    return size;
+
   /* Try to determine the size of the object from its type.  */
   if (TREE_CODE (dest) != ADDR_EXPR)
     return HOST_WIDE_INT_M1U;
@@ -4129,11 +4160,13 @@ determine_min_objsize (tree dest)
   type = TYPE_MAIN_VARIANT (type);
 
   /* The size of a flexible array cannot be determined.  Otherwise,
-     for arrays with more than one element, return the size of its
-     type.  GCC itself misuses arrays of both zero and one elements
-     as flexible array members so they are excluded as well.  */
+     unless the reference involves a union, for arrays with more than
+     one element, return the size of its type.  GCC itself misuses
+     arrays of both zero and one elements as flexible array members
+     so they are excluded as well.  */
   if (TREE_CODE (type) != ARRAY_TYPE
-      || !array_at_struct_end_p (dest))
+      || (!component_ref_via_union_p (dest)
+	  && !array_at_struct_end_p (dest)))
     {
       tree type_size = TYPE_SIZE_UNIT (type);
       if (type_size && TREE_CODE (type_size) == INTEGER_CST

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-02-06 13:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 13:25 [PATCH] adjust object size computation for union accesses and PHIs (PR 92765) Martin Sebor
2020-01-15 21:15 ` Jeff Law
2020-01-15 21:49 ` Jakub Jelinek
2020-01-16 13:26   ` Jakub Jelinek
2020-01-31 20:17 ` Martin Sebor
2020-02-03 18:44   ` Jeff Law
2020-02-04 14:35     ` Richard Biener
2020-02-05 23:58       ` Martin Sebor
2020-02-05 23:58     ` Martin Sebor
2020-02-06 13:01       ` Jeff Law

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).