Re: [PATCH] -Wdangling-pointer: don't mark SSA lhs sets as stores

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Feb 17, 2023 at 8:09 AM Alexandre Oliva via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> check_dangling_stores has some weirdnesses that causes its behavior to
> change when the target ABI requires C++ ctors to return this: while
> scanning stmts backwards in e.g. the AS ctor on a target that returns
> this in ctors, the scan first encounters a copy of this to the SSA
> name used to hold the return value.  m_ptr_query.get_ref resolves lhs
> (the return SSA name) to the rhs (the default SSA name for this), does
> not skip it because auto_var_p is false for SSA_NAMEs, and proceeds to
> add it to stores, which seems to prevent later attempts to add stores
> into *this from succeeding, which disables warnings that should have
> triggered.
>
> This is also the case when the backwards search finds unrelated stores
> to other fields of *this before it reaches stores that IMHO should be
> warned about.  The store found first disables checking of other
> stores, as if the store appearing later in the code would necessarily
> overwrite the store that should be warned about.  I've added an
> xfailed variant of the existing test (struct An) that triggers this
> problem, but I'm not sure how to go about fixing it.
>
> Meanwhile, this patch prevents assignments with SSA_NAMEs in the lhs
> from being regarded as stores, which is enough to remove the
> undesirable side effect on -Wdangling-pointer of ABI-mandated ctors'
> returning this.  Another variant of the existing test (struct Al) that
> demonstrates the problem regardless of this aspect of the ABI, and
> that gets the desired warning with the proposed patch, but not
> without.
>
> Curiously, this fix exposes yet another problem in
> Wdangling-pointer-5.c: it is the return stmt of the unrelated pointer
> p, not the store into possibly-overlapping *vpp2, that caused the
> warning to not be issued for the store in *vpp1.  I'm not sure whether
> we should or should not warn in that case, but this patch adjusts the
> test to reflect the behavior change.
>
> Regstrapped on x86_64-linux-gnu.
> Tested on arm-vxworks7 (gcc-12) and arm-eabi (trunk).  Ok to install?

It seems the case should run into

      else if (TREE_CODE (lhs_ref.ref) == SSA_NAME)
        {
          gimple *def_stmt = SSA_NAME_DEF_STMT (lhs_ref.ref);
          if (!gimple_nop_p (def_stmt))
            /* Avoid looking at or before stores into unknown objects.  */
            return;

          tree var = SSA_NAME_VAR (lhs_ref.ref);
          if (TREE_CODE (var) == PARM_DECL && DECL_BY_REFERENCE (var))
            /* Avoid by-value arguments transformed into by-reference.  */
            continue;

and what your patch tried to avoid is running into

      if (stores.add (lhs_ref.ref))
        continue;

?  I wonder what the circumstances are that we want the latter to happen if
the former condition is true?

> for  gcc/ChangeLog
>
>         * gimple-ssa-warn-access.cc
>         (pass_waccess::check_dangling_stores): Skip non-stores.
>
> for  gcc/testsuite/ChangeLog
>
>         * g++.dg/warn/Wdangling-pointer.C (warn_init_ref_member): Add
>         two new variants, one fixed, one xfailed.
>         * c-c++-common/Wdangling-pointer-5.c
>         (nowarn_store_arg_store_arg): Add now-expected warnings.
> ---
>  gcc/gimple-ssa-warn-access.cc                    |    3 ++
>  gcc/testsuite/c-c++-common/Wdangling-pointer-5.c |    4 ++-
>  gcc/testsuite/g++.dg/warn/Wdangling-pointer.C    |   29 +++++++++++++++++++++-
>  3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> index 2eab1d59abd05..c0efb3fdb4e52 100644
> --- a/gcc/gimple-ssa-warn-access.cc
> +++ b/gcc/gimple-ssa-warn-access.cc
> @@ -4511,7 +4511,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
>            use the escaped locals.  */
>         return;
>
> -      if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt))
> +      if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt)
> +         || !gimple_store_p (stmt))
>         continue;
>
>        access_ref lhs_ref;
> diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
> index 2a165cea76768..cb6da9e86394d 100644
> --- a/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
> +++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-5.c
> @@ -75,9 +75,9 @@ void nowarn_store_arg_store (void **vpp)
>
>  void* nowarn_store_arg_store_arg (void **vpp1, void **vpp2)
>  {
> -  int x;
> +  int x;              // { dg-message "'x' declared here" }
>    void **p = (void**)sink (0);
> -  *vpp1 = &x;         // warn here?
> +  *vpp1 = &x;         // { dg-warning "storing the address of local variable 'x' in '\\*vpp1'" }
>    *vpp2 = 0;          // might overwrite *vpp1
>    return p;
>  }
> diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
> index 22c559e4adafe..a94477a647666 100644
> --- a/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
> +++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer.C
> @@ -35,7 +35,34 @@ void warn_init_ref_member ()
>      { }
>    } ai;
>
> -  sink (&as, &ai);
> +  struct Al
> +  {
> +    const S &sref;
> +    Al ():
> +      // The temporary S object is destroyed when Al::Al() returns.
> +      sref (S ())  // { dg-warning "storing the address" }
> +    {
> +      // Copying this to an SSA_NAME used to disable the warning:
> +      Al *ptr = this;
> +      asm ("" : "+r" (ptr));
> +    }
> +  } al;
> +
> +  struct An
> +  {
> +    An *next;
> +    const S &sref;
> +    An ():
> +      next (0),
> +      // The temporary S object is destroyed when An::An() returns.
> +      sref (S ())  // { dg-warning "storing the address" "" { xfail *-*-* } }
> +    {
> +      // ??? Writing to another part of *this disables the warning:
> +      next = 0;
> +    }
> +  } an;
> +
> +  sink (&as, &ai, &al, &an);
>  }
>
>
>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>