RFA: Backport fix for PR80769

Message ID 87y3r9tvek.fsf@linaro.org
State New
Headers show

Commit Message

Richard Sandiford July 27, 2017, 9:36 p.m.
This is a minimal-ish backport of the fix for PR80769.  The trunk version
also replaced open-coded instances of get_next_strinfo with calls to the
new function.  It also added asserts in various other places to try to
ensure that related strinfos were consistently delayed or not delayed.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK for gcc-7-branch?
(And OK for gcc-6-branch if the same patch passes testing there?)

Richard


2017-07-27  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	PR tree-optimization/80769
	* tree-ssa-strlen.c (strinfo): Document that "stmt" is also used
	for malloc and calloc.  Document the new invariant that all related
	strinfos have delayed lengths or none do.
	(get_next_strinfo): New function.
	(verify_related_strinfos): Move earlier in file.
	(set_endptr_and_length): New function, split out from...
	(get_string_length): ...here.  Also set the lengths of related
	strinfos.

gcc/testsuite/
	PR tree-optimization/80769
	* gcc.dg/strlenopt-31.c: New test.
	* gcc.dg/strlenopt-31g.c: Likewise.

Comments

Richard Biener Aug. 1, 2017, 11:06 a.m. | #1
On Thu, Jul 27, 2017 at 11:36 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> This is a minimal-ish backport of the fix for PR80769.  The trunk version

> also replaced open-coded instances of get_next_strinfo with calls to the

> new function.  It also added asserts in various other places to try to

> ensure that related strinfos were consistently delayed or not delayed.

>

> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK for gcc-7-branch?

> (And OK for gcc-6-branch if the same patch passes testing there?)


Ok.

Thanks,
Richard.

> Richard

>

>

> 2017-07-27  Richard Sandiford  <richard.sandiford@linaro.org>

>

> gcc/

>         PR tree-optimization/80769

>         * tree-ssa-strlen.c (strinfo): Document that "stmt" is also used

>         for malloc and calloc.  Document the new invariant that all related

>         strinfos have delayed lengths or none do.

>         (get_next_strinfo): New function.

>         (verify_related_strinfos): Move earlier in file.

>         (set_endptr_and_length): New function, split out from...

>         (get_string_length): ...here.  Also set the lengths of related

>         strinfos.

>

> gcc/testsuite/

>         PR tree-optimization/80769

>         * gcc.dg/strlenopt-31.c: New test.

>         * gcc.dg/strlenopt-31g.c: Likewise.

>

> Index: gcc/tree-ssa-strlen.c

> ===================================================================

> --- gcc/tree-ssa-strlen.c       2017-05-10 18:04:24.514775477 +0100

> +++ gcc/tree-ssa-strlen.c       2017-07-27 18:21:20.308966958 +0100

> @@ -61,7 +61,13 @@ struct strinfo

>    tree length;

>    /* Any of the corresponding pointers for querying alias oracle.  */

>    tree ptr;

> -  /* Statement for delayed length computation.  */

> +  /* This is used for two things:

> +

> +     - To record the statement that should be used for delayed length

> +       computations.  We maintain the invariant that all related strinfos

> +       have delayed lengths or none do.

> +

> +     - To record the malloc or calloc call that produced this result.  */

>    gimple *stmt;

>    /* Pointer to '\0' if known, if NULL, it can be computed as

>       ptr + length.  */

> @@ -156,6 +162,19 @@ get_strinfo (int idx)

>    return (*stridx_to_strinfo)[idx];

>  }

>

> +/* Get the next strinfo in the chain after SI, or null if none.  */

> +

> +static inline strinfo *

> +get_next_strinfo (strinfo *si)

> +{

> +  if (si->next == 0)

> +    return NULL;

> +  strinfo *nextsi = get_strinfo (si->next);

> +  if (nextsi == NULL || nextsi->first != si->first || nextsi->prev != si->idx)

> +    return NULL;

> +  return nextsi;

> +}

> +

>  /* Helper function for get_stridx.  */

>

>  static int

> @@ -438,6 +457,45 @@ set_strinfo (int idx, strinfo *si)

>    (*stridx_to_strinfo)[idx] = si;

>  }

>

> +/* Return the first strinfo in the related strinfo chain

> +   if all strinfos in between belong to the chain, otherwise NULL.  */

> +

> +static strinfo *

> +verify_related_strinfos (strinfo *origsi)

> +{

> +  strinfo *si = origsi, *psi;

> +

> +  if (origsi->first == 0)

> +    return NULL;

> +  for (; si->prev; si = psi)

> +    {

> +      if (si->first != origsi->first)

> +       return NULL;

> +      psi = get_strinfo (si->prev);

> +      if (psi == NULL)

> +       return NULL;

> +      if (psi->next != si->idx)

> +       return NULL;

> +    }

> +  if (si->idx != si->first)

> +    return NULL;

> +  return si;

> +}

> +

> +/* Set SI's endptr to ENDPTR and compute its length based on SI->ptr.

> +   Use LOC for folding.  */

> +

> +static void

> +set_endptr_and_length (location_t loc, strinfo *si, tree endptr)

> +{

> +  si->endptr = endptr;

> +  si->stmt = NULL;

> +  tree start_as_size = fold_convert_loc (loc, size_type_node, si->ptr);

> +  tree end_as_size = fold_convert_loc (loc, size_type_node, endptr);

> +  si->length = fold_build2_loc (loc, MINUS_EXPR, size_type_node,

> +                               end_as_size, start_as_size);

> +}

> +

>  /* Return string length, or NULL if it can't be computed.  */

>

>  static tree

> @@ -533,12 +591,12 @@ get_string_length (strinfo *si)

>         case BUILT_IN_STPCPY_CHK_CHKP:

>           gcc_assert (lhs != NULL_TREE);

>           loc = gimple_location (stmt);

> -         si->endptr = lhs;

> -         si->stmt = NULL;

> -         lhs = fold_convert_loc (loc, size_type_node, lhs);

> -         si->length = fold_convert_loc (loc, size_type_node, si->ptr);

> -         si->length = fold_build2_loc (loc, MINUS_EXPR, size_type_node,

> -                                       lhs, si->length);

> +         set_endptr_and_length (loc, si, lhs);

> +         for (strinfo *chainsi = verify_related_strinfos (si);

> +              chainsi != NULL;

> +              chainsi = get_next_strinfo (chainsi))

> +           if (chainsi->length == NULL)

> +             set_endptr_and_length (loc, chainsi, lhs);

>           break;

>         case BUILT_IN_MALLOC:

>           break;

> @@ -607,32 +665,6 @@ unshare_strinfo (strinfo *si)

>    return nsi;

>  }

>

> -/* Return first strinfo in the related strinfo chain

> -   if all strinfos in between belong to the chain, otherwise

> -   NULL.  */

> -

> -static strinfo *

> -verify_related_strinfos (strinfo *origsi)

> -{

> -  strinfo *si = origsi, *psi;

> -

> -  if (origsi->first == 0)

> -    return NULL;

> -  for (; si->prev; si = psi)

> -    {

> -      if (si->first != origsi->first)

> -       return NULL;

> -      psi = get_strinfo (si->prev);

> -      if (psi == NULL)

> -       return NULL;

> -      if (psi->next != si->idx)

> -       return NULL;

> -    }

> -  if (si->idx != si->first)

> -    return NULL;

> -  return si;

> -}

> -

>  /* Attempt to create a new strinfo for BASESI + OFF, or find existing

>     strinfo if there is any.  Return it's idx, or 0 if no strinfo has

>     been created.  */

> Index: gcc/testsuite/gcc.dg/strlenopt-31.c

> ===================================================================

> --- /dev/null   2017-07-27 17:06:14.642985039 +0100

> +++ gcc/testsuite/gcc.dg/strlenopt-31.c 2017-07-27 18:11:20.158804210 +0100

> @@ -0,0 +1,25 @@

> +/* { dg-do run } */

> +/* { dg-options "-O2" } */

> +

> +#include "strlenopt.h"

> +

> +__attribute__((noinline, noclone)) int

> +bar (char *p1, const char *q)

> +{

> +  strcpy (p1, "abcde");

> +  char *p2 = strchr (p1, '\0');

> +  strcpy (p2, q);

> +  char *p3 = strchr (p2, '\0');

> +  memcpy (p3, "x", 2);

> +  return strlen (p1);

> +}

> +

> +int

> +main (void)

> +{

> +  char buffer[10];

> +  int res = bar (buffer, "foo");

> +  if (strcmp (buffer, "abcdefoox") != 0 || res != 9)

> +    abort ();

> +  return 0;

> +}

> Index: gcc/testsuite/gcc.dg/strlenopt-31g.c

> ===================================================================

> --- /dev/null   2017-07-27 17:06:14.642985039 +0100

> +++ gcc/testsuite/gcc.dg/strlenopt-31g.c        2017-07-27 18:11:20.158804210 +0100

> @@ -0,0 +1,9 @@

> +/* { dg-do run { target *-*-linux* *-*-gnu* } } */

> +/* { dg-options "-O2 -fdump-tree-strlen" } */

> +

> +#define USE_GNU

> +#include "strlenopt-31.c"

> +

> +/* { dg-final { scan-tree-dump-times "stpcpy \\(" 1 "strlen" } } */

> +/* { dg-final { scan-tree-dump-times "memcpy \\(" 2 "strlen" } } */

> +/* { dg-final { scan-tree-dump-not "strlen \\(" "strlen" } } */

Patch

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	2017-05-10 18:04:24.514775477 +0100
+++ gcc/tree-ssa-strlen.c	2017-07-27 18:21:20.308966958 +0100
@@ -61,7 +61,13 @@  struct strinfo
   tree length;
   /* Any of the corresponding pointers for querying alias oracle.  */
   tree ptr;
-  /* Statement for delayed length computation.  */
+  /* This is used for two things:
+
+     - To record the statement that should be used for delayed length
+       computations.  We maintain the invariant that all related strinfos
+       have delayed lengths or none do.
+
+     - To record the malloc or calloc call that produced this result.  */
   gimple *stmt;
   /* Pointer to '\0' if known, if NULL, it can be computed as
      ptr + length.  */
@@ -156,6 +162,19 @@  get_strinfo (int idx)
   return (*stridx_to_strinfo)[idx];
 }
 
+/* Get the next strinfo in the chain after SI, or null if none.  */
+
+static inline strinfo *
+get_next_strinfo (strinfo *si)
+{
+  if (si->next == 0)
+    return NULL;
+  strinfo *nextsi = get_strinfo (si->next);
+  if (nextsi == NULL || nextsi->first != si->first || nextsi->prev != si->idx)
+    return NULL;
+  return nextsi;
+}
+
 /* Helper function for get_stridx.  */
 
 static int
@@ -438,6 +457,45 @@  set_strinfo (int idx, strinfo *si)
   (*stridx_to_strinfo)[idx] = si;
 }
 
+/* Return the first strinfo in the related strinfo chain
+   if all strinfos in between belong to the chain, otherwise NULL.  */
+
+static strinfo *
+verify_related_strinfos (strinfo *origsi)
+{
+  strinfo *si = origsi, *psi;
+
+  if (origsi->first == 0)
+    return NULL;
+  for (; si->prev; si = psi)
+    {
+      if (si->first != origsi->first)
+	return NULL;
+      psi = get_strinfo (si->prev);
+      if (psi == NULL)
+	return NULL;
+      if (psi->next != si->idx)
+	return NULL;
+    }
+  if (si->idx != si->first)
+    return NULL;
+  return si;
+}
+
+/* Set SI's endptr to ENDPTR and compute its length based on SI->ptr.
+   Use LOC for folding.  */
+
+static void
+set_endptr_and_length (location_t loc, strinfo *si, tree endptr)
+{
+  si->endptr = endptr;
+  si->stmt = NULL;
+  tree start_as_size = fold_convert_loc (loc, size_type_node, si->ptr);
+  tree end_as_size = fold_convert_loc (loc, size_type_node, endptr);
+  si->length = fold_build2_loc (loc, MINUS_EXPR, size_type_node,
+				end_as_size, start_as_size);
+}
+
 /* Return string length, or NULL if it can't be computed.  */
 
 static tree
@@ -533,12 +591,12 @@  get_string_length (strinfo *si)
 	case BUILT_IN_STPCPY_CHK_CHKP:
 	  gcc_assert (lhs != NULL_TREE);
 	  loc = gimple_location (stmt);
-	  si->endptr = lhs;
-	  si->stmt = NULL;
-	  lhs = fold_convert_loc (loc, size_type_node, lhs);
-	  si->length = fold_convert_loc (loc, size_type_node, si->ptr);
-	  si->length = fold_build2_loc (loc, MINUS_EXPR, size_type_node,
-					lhs, si->length);
+	  set_endptr_and_length (loc, si, lhs);
+	  for (strinfo *chainsi = verify_related_strinfos (si);
+	       chainsi != NULL;
+	       chainsi = get_next_strinfo (chainsi))
+	    if (chainsi->length == NULL)
+	      set_endptr_and_length (loc, chainsi, lhs);
 	  break;
 	case BUILT_IN_MALLOC:
 	  break;
@@ -607,32 +665,6 @@  unshare_strinfo (strinfo *si)
   return nsi;
 }
 
-/* Return first strinfo in the related strinfo chain
-   if all strinfos in between belong to the chain, otherwise
-   NULL.  */
-
-static strinfo *
-verify_related_strinfos (strinfo *origsi)
-{
-  strinfo *si = origsi, *psi;
-
-  if (origsi->first == 0)
-    return NULL;
-  for (; si->prev; si = psi)
-    {
-      if (si->first != origsi->first)
-	return NULL;
-      psi = get_strinfo (si->prev);
-      if (psi == NULL)
-	return NULL;
-      if (psi->next != si->idx)
-	return NULL;
-    }
-  if (si->idx != si->first)
-    return NULL;
-  return si;
-}
-
 /* Attempt to create a new strinfo for BASESI + OFF, or find existing
    strinfo if there is any.  Return it's idx, or 0 if no strinfo has
    been created.  */
Index: gcc/testsuite/gcc.dg/strlenopt-31.c
===================================================================
--- /dev/null	2017-07-27 17:06:14.642985039 +0100
+++ gcc/testsuite/gcc.dg/strlenopt-31.c	2017-07-27 18:11:20.158804210 +0100
@@ -0,0 +1,25 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+#include "strlenopt.h"
+
+__attribute__((noinline, noclone)) int
+bar (char *p1, const char *q)
+{
+  strcpy (p1, "abcde");
+  char *p2 = strchr (p1, '\0');
+  strcpy (p2, q);
+  char *p3 = strchr (p2, '\0');
+  memcpy (p3, "x", 2);
+  return strlen (p1);
+}
+
+int
+main (void)
+{
+  char buffer[10];
+  int res = bar (buffer, "foo");
+  if (strcmp (buffer, "abcdefoox") != 0 || res != 9)
+    abort ();
+  return 0;
+}
Index: gcc/testsuite/gcc.dg/strlenopt-31g.c
===================================================================
--- /dev/null	2017-07-27 17:06:14.642985039 +0100
+++ gcc/testsuite/gcc.dg/strlenopt-31g.c	2017-07-27 18:11:20.158804210 +0100
@@ -0,0 +1,9 @@ 
+/* { dg-do run { target *-*-linux* *-*-gnu* } } */
+/* { dg-options "-O2 -fdump-tree-strlen" } */
+
+#define USE_GNU
+#include "strlenopt-31.c"
+
+/* { dg-final { scan-tree-dump-times "stpcpy \\(" 1 "strlen" } } */
+/* { dg-final { scan-tree-dump-times "memcpy \\(" 2 "strlen" } } */
+/* { dg-final { scan-tree-dump-not "strlen \\(" "strlen" } } */