This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] avoid assuming strncat/strncpy don't change length of source string (PR 83075)
- From: Martin Sebor <msebor at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Gcc Patch List <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 20 Nov 2017 14:38:10 -0700
- Subject: Re: [PATCH] avoid assuming strncat/strncpy don't change length of source string (PR 83075)
- Authentication-results: sourceware.org; auth=none
- References: <38594ab4-39c8-5018-28de-05927cb3bb4b@gmail.com> <20171120193925.GL14653@tucnak>
On 11/20/2017 12:39 PM, Jakub Jelinek wrote:
On Mon, Nov 20, 2017 at 12:15:09PM -0700, Martin Sebor wrote:
PR tree-optimization/83075
* tree-ssa-strlen.c (handle_builtin_stxncpy): Avoid assuming
strncat/strncpy don't change length of source string.
gcc/testsuite/ChangeLog:
PR tree-optimization/83075
* gcc.dg/tree-ssa/strncat.c: New test.
* gcc.dg/tree-ssa/strncpy-2.c: Same.
Index: gcc/testsuite/gcc.dg/tree-ssa/strncat.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/strncat.c (nonexistent)
+++ gcc/testsuite/gcc.dg/tree-ssa/strncat.c (working copy)
@@ -0,0 +1,22 @@
+/* PR tree-optimization/83075 - Invalid strncpy optimization
+ { dg-do compile }
+ { dg-options "-O2 -Wno-stringop-overflow -fdump-tree-optimized" } */
+
+void test_overlapping_strncpy (void)
+{
+ char a[8] = "";
+
+ __builtin_strcpy (a, "123");
+
+ unsigned n0 = __builtin_strlen (a);
+
+ __builtin_strncat (a + 3, a, n0);
+
+ unsigned n1 = __builtin_strlen (a);
+
+ if (n1 == n0)
+ __builtin_abort ();
+}
+
+/* Verify the call to abort hasn't been eliminated.
+ { dg-final { scan-tree-dump-times "abort" 1 "optimized" } } */
Index: gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c (nonexistent)
+++ gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c (working copy)
@@ -0,0 +1,22 @@
+/* PR tree-optimization/83075 - Invalid strncpy optimization
+ { dg-do compile }
+ { dg-options "-O2 -Wno-stringop-overflow -fdump-tree-optimized" } */
+
+void test_overlapping_strncpy (void)
+{
+ char a[8] = "";
+
+ __builtin_strcpy (a, "123");
+
+ unsigned n0 = __builtin_strlen (a);
+
+ __builtin_strncpy (a + 3, a, n0);
+
+ unsigned n1 = __builtin_strlen (a);
+
+ if (n1 == n0)
+ __builtin_abort ();
+}
+
+/* Verify the call to abort hasn't been eliminated.
+ { dg-final { scan-tree-dump-times "abort" 1 "optimized" } } */
I think it is better to have a dg-do run testcase and not scan for abort.
We can be able to optimize at some point n0 to 3 and n1 to 6 and optimize
away the abort. In the testcase I've added to the PR there was a separate
function, you could add __attribute__((noipa)) to it to make sure that
it isn't IPA optimized.
Similarly for the strncat test.
Sure. Attached is an update to make tests runnable.
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c (revision 254961)
+++ gcc/tree-ssa-strlen.c (working copy)
@@ -1931,10 +1931,9 @@ handle_builtin_stxncpy (built_in_function, gimple_
int sidx = get_stridx (src);
strinfo *sisrc = sidx > 0 ? get_strinfo (sidx) : NULL;
- /* Strncpy() et al. cannot modify the source string. Prevent the rest
- of the pass from invalidating the strinfo data. */
- if (sisrc)
- sisrc->dont_invalidate = true;
+ /* Strncat() and strncpy() can modify the source string by specifying
+ as a destination SRC + strlen(SRC) and a bound <= strlen(SRC) so
+ SISRC->DONT_INVALIDATE must be left clear. */
The destination could be SRC + N and as long as LEN <= N, you shouldn't
invalidate SRC. Though, if LEN < strlen(SRC) I think you return early.
I admit I'm intrigued by the idea. At the same time, for strncpy
and strncat, these are the cases that trigger the truncation warning.
They don't seem like something worth putting effort into optimizing.
I don't expect copying into the same array to be a common use of
string functions but perhaps it's worth thinking about for memcpy
and/or memmove?
void f (void)
{
char a[10] = "";
__builtin_strcpy (a, "123");
unsigned n0 = __builtin_strlen (a);
__builtin_memcpy (a + 4, a, 4);
unsigned n1 = __builtin_strlen (a);
if (n1 != n0)
__builtin_abort ();
}
Martin
PR tree-optimization/83075 - Invalid strncpy optimization
gcc/ChangeLog:
PR tree-optimization/83075
* tree-ssa-strlen.c (handle_builtin_stxncpy): Avoid assuming
strncat/strncpy don't change length of source string.
gcc/testsuite/ChangeLog:
PR tree-optimization/83075
* gcc.dg/tree-ssa/strncat.c: New test.
* gcc.dg/tree-ssa/strncpy-2.c: Same.
Index: gcc/testsuite/gcc.dg/tree-ssa/strncat.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/strncat.c (nonexistent)
+++ gcc/testsuite/gcc.dg/tree-ssa/strncat.c (working copy)
@@ -0,0 +1,25 @@
+/* PR tree-optimization/83075 - Invalid strncpy optimization
+ { dg-do run }
+ { dg-options "-O2 -Wno-stringop-overflow" } */
+
+void __attribute__ ((noipa))
+test_overlapping_strncpy (void)
+{
+ char a[8] = "";
+
+ __builtin_strcpy (a, "123");
+
+ unsigned n0 = __builtin_strlen (a);
+
+ __builtin_strncat (a + 3, a, n0);
+
+ unsigned n1 = __builtin_strlen (a);
+
+ if (n1 == n0)
+ __builtin_abort ();
+}
+
+int main (void)
+{
+ test_overlapping_strncpy ();
+}
Index: gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c (nonexistent)
+++ gcc/testsuite/gcc.dg/tree-ssa/strncpy-2.c (working copy)
@@ -0,0 +1,25 @@
+/* PR tree-optimization/83075 - Invalid strncpy optimization
+ { dg-do run }
+ { dg-options "-O2 -Wno-stringop-overflow" } */
+
+void __attribute__ ((noipa))
+test_overlapping_strncpy (void)
+{
+ char a[8] = "";
+
+ __builtin_strcpy (a, "123");
+
+ unsigned n0 = __builtin_strlen (a);
+
+ __builtin_strncpy (a + 3, a, n0);
+
+ unsigned n1 = __builtin_strlen (a);
+
+ if (n1 == n0)
+ __builtin_abort ();
+}
+
+int main ()
+{
+ test_overlapping_strncpy ();
+}
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c (revision 254961)
+++ gcc/tree-ssa-strlen.c (working copy)
@@ -1931,10 +1931,9 @@ handle_builtin_stxncpy (built_in_function, gimple_
int sidx = get_stridx (src);
strinfo *sisrc = sidx > 0 ? get_strinfo (sidx) : NULL;
- /* Strncpy() et al. cannot modify the source string. Prevent the rest
- of the pass from invalidating the strinfo data. */
- if (sisrc)
- sisrc->dont_invalidate = true;
+ /* Strncat() and strncpy() can modify the source string by specifying
+ as a destination SRC + strlen(SRC) and a bound <= strlen(SRC) so
+ SISRC->DONT_INVALIDATE must be left clear. */
/* Retrieve the strinfo data for the string S that LEN was computed
from as some function F of strlen (S) (i.e., LEN need not be equal