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>
