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

[Jakub Jelinek]

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