Re: [PATCH] avoid assuming strncat/strncpy don't change length of source string (PR 83075)

[Martin Sebor <msebor at gmail dot com>]

Attached is a patch with the comment updated/simplified.
The tests do the job they need to do today so I just removed
the useless attribute but otherwise left them unchanged.  If
you would like to enhance them in some way please feel free.

Martin

On 11/20/2017 02:56 PM, Jakub Jelinek wrote:
> On Mon, Nov 20, 2017 at 02:38:10PM -0700, Martin Sebor wrote:
> > > 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?
>
> I wasn't suggesting to optimize anything, just to clarify the
> comment.  The 3rd argument is called LEN in the code, so instead
> of talking about bound it should talk about LEN.  And the destination
> can be closer to SRC than the strlen.  So I'd just change above your
> "SRC + strlen(SRC) and a bound <= strlen(SRC)" to
> "SRC + N for some N >= LEN" or similar.
>
> > --- 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)
>
> In this case the noipa attribute is pretty useless.
> What I was suggesting is that you have a noipa
> function with 2 char * arguments and the initialization and strcpy
> is done in the caller.  Then it better tests what we care about,
> it doesn't see for sure that the two pointers are related, the caller shows
> they might.  If everything is in one function, then a future version
> of the compiler might optimize everything away.
>
> 	Jakub

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/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 255369)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1941,10 +1941,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 writing
+     over the terminating nul 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
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,19 @@
+/* PR tree-optimization/83075 - Invalid strncpy optimization
+   { dg-do run }
+   { dg-options "-O2 -Wno-stringop-overflow" } */
+
+int main (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 ();
+}
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,19 @@
+/* PR tree-optimization/83075 - Invalid strncpy optimization
+   { dg-do run }
+   { dg-options "-O2 -Wno-stringop-overflow" } */
+
+int main (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 ();
+}