Re: [PATCH] [PR81810] unused strcpy to a local buffer not eliminated

[Jeff Law <law at redhat dot com>]

On 8/21/19 11:23 AM, kamlesh kumar wrote:
> Hi ,
> This patch include fix for PR81810
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> Thanks
> ./Kamlesh
> 
> 2019-08-21  Kamlesh Kumar   <kamleshbhalui@gmail.com>
> 
>        PR tree-optimization/81810
>        * tree-ssa-dse.c (dse_dom_walker::dse_optimize_stmt): Added
>        BUILT_IN_STRCPY to consider for dse.
>        (maybe_trim_memstar_call): Likewise.
>        (initialize_ao_ref_for_dse): Likewise.
>        * gcc.dg/tree-ssa/pr81810.c: New testcase.
This doesn't look right to me.


> 
> diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c
> index ba67884a825..dc4da4c9730 100644
> --- a/gcc/tree-ssa-dse.c
> +++ b/gcc/tree-ssa-dse.c
> @@ -115,8 +115,11 @@ initialize_ao_ref_for_dse (gimple *stmt, ao_ref *write)
>         case BUILT_IN_MEMSET_CHK:
>         case BUILT_IN_STRNCPY:
>         case BUILT_IN_STRNCPY_CHK:
> +       case BUILT_IN_STRCPY:
>           {
> -           tree size = gimple_call_arg (stmt, 2);
> +           tree size = NULL;
> +           if (gimple_call_num_args (stmt) > 2)
> +               size = gimple_call_arg (stmt, 2);
>             tree ptr = gimple_call_arg (stmt, 0);
>             ao_ref_init_from_ptr_and_size (write, ptr, size);
>             return true;
This routine serves two purposes.

It can be called for a statement that is potentially dead.  In that case
it has to set the size of the ao_ref to cover all the data potentially
written by the strcpy.

It can also be called for a statement that makes an earlier statement
dead.  In this case you have to set the size for the ao_ref to the
number of bytes that you know have to be written.

In both cases the size is a function of the source string.  You can't
just leave the size NULL and expect everything to continue to work.






> @@ -470,6 +473,7 @@ maybe_trim_memstar_call (ao_ref *ref, sbitmap live,
> gimple *stmt)
>      case BUILT_IN_MEMCPY:
>      case BUILT_IN_MEMMOVE:
>      case BUILT_IN_STRNCPY:
> +    case BUILT_IN_STRCPY:
>      case BUILT_IN_MEMCPY_CHK:
>      case BUILT_IN_MEMMOVE_CHK:
>      case BUILT_IN_STRNCPY_CHK:
How do you expect to trim a partially dead strcpy call?  The only way to
do that is to adjust the source string.  There's no mechanisms right now
to do that.  So this change can't be correct either.



> @@ -966,6 +970,7 @@ dse_dom_walker::dse_optimize_stmt (gimple_stmt_iterator
> *gsi)
>         case BUILT_IN_MEMCPY:
>         case BUILT_IN_MEMMOVE:
>         case BUILT_IN_STRNCPY:
> +       case BUILT_IN_STRCPY:
>         case BUILT_IN_MEMSET:
>         case BUILT_IN_MEMCPY_CHK:
>         case BUILT_IN_MEMMOVE_CHK:
This would be OK if all the other stuff was done right :-)


> @@ -975,11 +980,14 @@ dse_dom_walker::dse_optimize_stmt
> (gimple_stmt_iterator *gsi)
>             /* Occasionally calls with an explicit length of zero
>                show up in the IL.  It's pointless to do analysis
>                on them, they're trivially dead.  */
> -           tree size = gimple_call_arg (stmt, 2);
> -           if (integer_zerop (size))
> +           if (gimple_call_num_args (stmt) > 2)
>               {
> -               delete_dead_or_redundant_call (gsi, "dead");
> -               return;
> +               tree size = gimple_call_arg (stmt, 2);
> +               if (integer_zerop (size))
> +                 {
> +                   delete_dead_or_redundant_call (gsi, "dead");
> +                   return;
> +                 }
>               }
?!?  This looks like you're just working around the fact that the other
parts of the patch are incorrect.


> 
>             /* If this is a memset call that initializes an object
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr81810.c
> b/gcc/testsuite/gcc.dg/tree-ssa/pr81810.c
> new file mode 100644
> index 00000000000..89cf36a1367
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr81810.c
> @@ -0,0 +1,25 @@
> +/* PR tree-optimization/81810
> +   { dg-do compile }
> +   { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +void f (const void *p, unsigned n)
> +{
> +  char a[8];
> +  __builtin_memcpy (a, p, n);
> +}
> +
> +void g (const char *s)
> +{
> +  char a[8];
> +  __builtin_strcpy (a, s);
> +}
> +
> +void h (const char *s)
> +{
> +  char a[8];
> +  __builtin_strncpy (a, s, sizeof a);
> +}
So just to be clear, the only case that's not handled here is the second
one involving strcpy.  The memcpy and strncpy cases are already handled.

The second case isn't handled because DSE doesn't know about the
semantics of strcpy yet.  That's precisely what I've been working on
recently and I'm pretty sure my changes (if we can deal with the
problems regarding some low level gimple semantics and sub-object
consistency) will fix this issue.  If my changes don't, then it
something I'm pretty sure can be addressed correctly using my changes as
underlying infrastructure.

jeff