Re: [PATCH] warn-access: Fix up check_pointer_uses [PR104715]

[Martin Sebor <msebor@gmail.com>]

On 3/1/22 11:41, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase emits bogus -Wdangling-pointer warnings.
> The bug is that when it sees that ptr immediate use is a call that
> returns one of its arguments, it will assume that the return value
> is based on ptr, but that is the case only if ptr is passed to the
> argument that is actually returned (so e.g. for memcpy the first argument,
> etc.).  When the builtins guarantee e.g. that the result is based on the
> first argument (either ERF_RETURNS_ARG 0 in which case it will always
> just returns the first argument as is, or when it is something like
> strstr or strpbrk or mempcpy that it returns some pointer based on the
> first argument), it means the result is not based on second or following
> argument if any.  The second hunk fixes this.
> 
> The first hunk just removes an unnecessary TREE_CODE check, the code only
> pushes SSA_NAMEs into the pointers vector and if it didn't, it uses
>        FOR_EACH_IMM_USE_FAST (use_p, iter, ptr)
> a few lines below this, which of course requires that ptr is a SSA_NAME.
> Tree checking on SSA_NAME_VERSION will already ensure that if it wasn't
> a SSA_NAME, we'd ICE.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Thanks for the fix.  It makes sense to me.  Besides the test for
the false positives I would suggest to add one to verify that using
the first argument to a strstr() call is diagnosed if it's dangling
(both as is, as well as with an offset from the first element).
There are tests for memchr and strchr in the -Wdangling-pointer
test suite but none for strstr.

Martin

> 
> 2022-03-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/104715
> 	* gimple-ssa-warn-access.cc (pass_waccess::check_pointer_uses): Don't
> 	unnecessarily test if ptr is a SSA_NAME, it has to be.  Only push lhs
> 	of a call if gimple_call_return_arg is equal to ptr, not just when it
> 	is non-NULL.
> 
> 	* c-c++-common/Wdangling-pointer-7.c: New test.
> 
> --- gcc/gimple-ssa-warn-access.cc.jj	2022-02-28 16:22:40.860520930 +0100
> +++ gcc/gimple-ssa-warn-access.cc	2022-02-28 16:55:01.242272499 +0100
> @@ -4169,8 +4169,7 @@ pass_waccess::check_pointer_uses (gimple
>     for (unsigned i = 0; i != pointers.length (); ++i)
>       {
>         tree ptr = pointers[i];
> -      if (TREE_CODE (ptr) == SSA_NAME
> -	  && !bitmap_set_bit (visited, SSA_NAME_VERSION (ptr)))
> +      if (!bitmap_set_bit (visited, SSA_NAME_VERSION (ptr)))
>   	/* Avoid revisiting the same pointer.  */
>   	continue;
>   
> @@ -4267,7 +4266,7 @@ pass_waccess::check_pointer_uses (gimple
>   
>   	  if (gcall *call = dyn_cast <gcall *>(use_stmt))
>   	    {
> -	      if (gimple_call_return_arg (call))
> +	      if (gimple_call_return_arg (call) == ptr)
>   		if (tree lhs = gimple_call_lhs (call))
>   		  if (TREE_CODE (lhs) == SSA_NAME)
>   		    pointers.safe_push (lhs);
> --- gcc/testsuite/c-c++-common/Wdangling-pointer-7.c.jj	2022-02-28 17:09:09.906355082 +0100
> +++ gcc/testsuite/c-c++-common/Wdangling-pointer-7.c	2022-02-28 17:03:50.533839892 +0100
> @@ -0,0 +1,36 @@
> +/* PR tree-optimization/104715 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wdangling-pointer" } */
> +
> +char *
> +foo (char *p)
> +{
> +  {
> +    char q[61] = "012345678901234567890123456789012345678901234567890123456789";
> +    char *r = q;
> +    p = __builtin_strcat (p, r);
> +  }
> +  return p;	/* { dg-bogus "using dangling pointer" } */
> +}
> +
> +char *
> +bar (char *p)
> +{
> +  {
> +    char q[] = "0123456789";
> +    char *r = q;
> +    p = __builtin_strstr (p, r);
> +  }
> +  return p;	/* { dg-bogus "using dangling pointer" } */
> +}
> +
> +char *
> +baz (char *p)
> +{
> +  {
> +    char q[] = "0123456789";
> +    char *r = q;
> +    p = __builtin_strpbrk (p, r);
> +  }
> +  return p;	/* { dg-bogus "using dangling pointer" } */
> +}
> 
> 	Jakub
>