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

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

[Alexandre Oliva]

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?

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>

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

[Richard Biener]

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>

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

[Alexandre Oliva]

Hi, richi,

On Feb 17, 2023, Richard Biener <richard.guenther@gmail.com> wrote:

> It seems the case should run into

Yeah, but when the stmt is _7 = this_2(D), lhs is _7, whereas
lhs_ref.ref is this_2(D), a parm decl's default def, so def_stmt is a
gimple_nop, and this is not a decl_by_reference, so we don't skip
stores.add, and any subsequent stores into fields of *this fails
stores.add.  This looks so fishy that I couldn't guess what was supposed
or expected to happen there :-(

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

Note that what I'm testing for, to skip non-store stmts, is the actual
lhs, whereas lhs_ref.ref may very well have been resolved to the rhs (as
in the cases I saw), and even if it could be an SSA_NAME, whether the
stmt is a store depends on the actual lhs, not on properties of the rhs
that get_ref resolved lhs to, right?

Now, really, I did not get as far as trying to make sense of the
algorithm in there (get_ref definitely doesn't do what its name suggests
to me), I just saw a bunch of weirdnesses in blackbox testing and
failing variations, that seemed to suggest some fundamental issue, that
would hopefully be obvious to someone more familiar with that code.

-- 
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>

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

[Alexandre Oliva]

On Feb 17, 2023, Alexandre Oliva <oliva@adacore.com> wrote:

> Now, really, I did not get as far as trying to make sense of the
> algorithm in there (get_ref definitely doesn't do what its name suggests
> to me), I just saw a bunch of weirdnesses in blackbox testing and
> failing variations, that seemed to suggest some fundamental issue, that
> would hopefully be obvious to someone more familiar with that code.

Here's one more: the stores hash_set seems to be intended to help catch
overwrites, so as to detect assignments of pointers-to-auto-locals to
escaping pointers that reach exits, without flagging those that are
definitely overwritten afterwards.

In a linear backwards scan, that makes sense, but once blocks recurse
for their preds, ISTM it could suppress the desired warning in e.g.:

  if (condition)
    *incoming_ptr = &localvar;
  else
    *incoming_ptr = 0;

in case we scan the else bb before the then bb.


But how about asynchronous exceptions?  They might leave those dangling
pointers if they hit at an undesirable spot.  So should we really
suppress those warnings?

-- 
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>

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

[Alexandre Oliva]

On Feb 17, 2023, Richard Biener <richard.guenther@gmail.com> wrote:

>> * 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.

Ping?
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612186.html

-- 
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>

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

[Richard Biener]

On Fri, Mar 3, 2023 at 9:30 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Feb 17, 2023, Richard Biener <richard.guenther@gmail.com> wrote:
>
> >> * 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.
>
> Ping?
> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612186.html

I was hoping Martin would chime in, but he didn't.

So - OK.

Thanks,
Richard.

>
> --
> 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>

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

[Martin Liška]

On 3/3/23 12:12, Richard Biener via Gcc-patches wrote:
> On Fri, Mar 3, 2023 at 9:30 AM Alexandre Oliva <oliva@adacore.com> wrote:
>>
>> On Feb 17, 2023, Richard Biener <richard.guenther@gmail.com> wrote:
>>
>>>> * 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.
>>
>> Ping?
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612186.html
> 
> I was hoping Martin would chime in, but he didn't.
> 
> So - OK.
> 
> Thanks,
> Richard.
> 
>>
>> --
>> 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>

Hi.

I've just noticed this change triggered one more warning for qemu 7.1.0:

cc -m64 -mcx16 -Ilibqemuutil.a.p -I. -I.. -Isubprojects/libvhost-user -I../subprojects/libvhost-user -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/p11-kit-1 -I/usr/include/pixman-1 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -isystem /home/abuild/rpmbuild/BUILD/qemu-7.1.0/linux-headers -isystem linux-headers -iquote . -iquote /home/abuild/rpmbuild/BUILD/qemu-7.1.0 -iquote /home/abuild/rpmbuild/BUILD/qemu-7.1.0/include -iquote /home/abuild/rpmbuild/BUILD/qemu-7.1.0/tcg/i386 -pthread -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -O2 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIE -MD -MQ libqemuutil.a.p/util_async.c.o -MF libqemuutil.a.p/util_async.c.o.d -o libqemuutil.a.p/util_async.c.o -c ../util/async.c
In file included from /home/abuild/rpmbuild/BUILD/qemu-7.1.0/include/qemu/coroutine.h:18,
                 from /home/abuild/rpmbuild/BUILD/qemu-7.1.0/include/block/aio.h:20,
                 from ../util/async.c:28:
../util/async.c: In function 'aio_bh_poll':
/home/abuild/rpmbuild/BUILD/qemu-7.1.0/include/qemu/queue.h:303:22: error: storing the address of local variable 'slice' in '*ctx.bh_slice_list.sqh_last' [-Werror=dangling-pointer=]
  303 |     (head)->sqh_last = &(elm)->field.sqe_next;                          \
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
../util/async.c:161:5: note: in expansion of macro 'QSIMPLEQ_INSERT_TAIL'
  161 |     QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
      |     ^~~~~~~~~~~~~~~~~~~~
../util/async.c:156:17: note: 'slice' declared here
  156 |     BHListSlice slice;
      |                 ^~~~~
../util/async.c:156:17: note: 'ctx' declared here

which I reduced to:

$ cat util_async.i
typedef struct BHListSlice BHListSlice;
struct BHListSlice {
  struct {
    BHListSlice *sqe_next;
  } next;
} *aio_bh_poll_s;
struct AioContext {
  struct {
    BHListSlice *sqh_first;
    BHListSlice **sqh_last;
  } bh_slice_list;
} aio_bh_dequeue();
int aio_bh_poll_bh;
int aio_bh_poll(struct AioContext *ctx) {
  BHListSlice slice;
  (&ctx->bh_slice_list)->sqh_last = &(slice.next.sqe_next);
  while (aio_bh_poll_s) {
    unsigned flags;
    aio_bh_dequeue(&flags);
    if (aio_bh_poll_bh) {
      (&ctx->bh_slice_list)->sqh_last = &(&ctx->bh_slice_list)->sqh_first;
      continue;
    }
  }
  return 0;
}

$ gcc util_async.i -c -Werror=dangling-pointer
util_async.i: In function ‘aio_bh_poll’:
util_async.i:16:35: error: storing the address of local variable ‘slice’ in ‘*ctx.bh_slice_list.sqh_last’ [-Werror=dangling-pointer=]
   16 |   (&ctx->bh_slice_list)->sqh_last = &(slice.next.sqe_next);
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
util_async.i:15:15: note: ‘slice’ declared here
   15 |   BHListSlice slice;
      |               ^~~~~
util_async.i:15:15: note: ‘ctx’ declared here
cc1: some warnings being treated as errors

Is the emitted warning correct?

Thank you,
Martin

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

[Richard Biener]

On Wed, Mar 8, 2023 at 2:04 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 3/3/23 12:12, Richard Biener via Gcc-patches wrote:
> > On Fri, Mar 3, 2023 at 9:30 AM Alexandre Oliva <oliva@adacore.com> wrote:
> >>
> >> On Feb 17, 2023, Richard Biener <richard.guenther@gmail.com> wrote:
> >>
> >>>> * 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.
> >>
> >> Ping?
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612186.html
> >
> > I was hoping Martin would chime in, but he didn't.
> >
> > So - OK.
> >
> > Thanks,
> > Richard.
> >
> >>
> >> --
> >> 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>
>
> Hi.
>
> I've just noticed this change triggered one more warning for qemu 7.1.0:
>
> cc -m64 -mcx16 -Ilibqemuutil.a.p -I. -I.. -Isubprojects/libvhost-user -I../subprojects/libvhost-user -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 -I/usr/include/p11-kit-1 -I/usr/include/pixman-1 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -isystem /home/abuild/rpmbuild/BUILD/qemu-7.1.0/linux-headers -isystem linux-headers -iquote . -iquote /home/abuild/rpmbuild/BUILD/qemu-7.1.0 -iquote /home/abuild/rpmbuild/BUILD/qemu-7.1.0/include -iquote /home/abuild/rpmbuild/BUILD/qemu-7.1.0/tcg/i386 -pthread -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -O2 -Wall -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIE -MD -MQ libqemuutil.a.p/util_async.c.o -MF libqemuutil.a.p/util_async.c.o.d -o libqemuutil.a.p/util_async.c.o -c ../util/async.c
> In file included from /home/abuild/rpmbuild/BUILD/qemu-7.1.0/include/qemu/coroutine.h:18,
>                  from /home/abuild/rpmbuild/BUILD/qemu-7.1.0/include/block/aio.h:20,
>                  from ../util/async.c:28:
> ../util/async.c: In function 'aio_bh_poll':
> /home/abuild/rpmbuild/BUILD/qemu-7.1.0/include/qemu/queue.h:303:22: error: storing the address of local variable 'slice' in '*ctx.bh_slice_list.sqh_last' [-Werror=dangling-pointer=]
>   303 |     (head)->sqh_last = &(elm)->field.sqe_next;                          \
>       |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
> ../util/async.c:161:5: note: in expansion of macro 'QSIMPLEQ_INSERT_TAIL'
>   161 |     QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
>       |     ^~~~~~~~~~~~~~~~~~~~
> ../util/async.c:156:17: note: 'slice' declared here
>   156 |     BHListSlice slice;
>       |                 ^~~~~
> ../util/async.c:156:17: note: 'ctx' declared here
>
> which I reduced to:
>
> $ cat util_async.i
> typedef struct BHListSlice BHListSlice;
> struct BHListSlice {
>   struct {
>     BHListSlice *sqe_next;
>   } next;
> } *aio_bh_poll_s;
> struct AioContext {
>   struct {
>     BHListSlice *sqh_first;
>     BHListSlice **sqh_last;
>   } bh_slice_list;
> } aio_bh_dequeue();
> int aio_bh_poll_bh;
> int aio_bh_poll(struct AioContext *ctx) {
>   BHListSlice slice;
>   (&ctx->bh_slice_list)->sqh_last = &(slice.next.sqe_next);
>   while (aio_bh_poll_s) {
>     unsigned flags;
>     aio_bh_dequeue(&flags);
>     if (aio_bh_poll_bh) {
>       (&ctx->bh_slice_list)->sqh_last = &(&ctx->bh_slice_list)->sqh_first;
>       continue;
>     }
>   }
>   return 0;
> }
>
> $ gcc util_async.i -c -Werror=dangling-pointer
> util_async.i: In function ‘aio_bh_poll’:
> util_async.i:16:35: error: storing the address of local variable ‘slice’ in ‘*ctx.bh_slice_list.sqh_last’ [-Werror=dangling-pointer=]
>    16 |   (&ctx->bh_slice_list)->sqh_last = &(slice.next.sqe_next);
>       |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
> util_async.i:15:15: note: ‘slice’ declared here
>    15 |   BHListSlice slice;
>       |               ^~~~~
> util_async.i:15:15: note: ‘ctx’ declared here
> cc1: some warnings being treated as errors
>
> Is the emitted warning correct?

For the reduced testcase yes, if !aio_bh_poll_s (or !aio_bh_poll_bh)
the stored pointer
remains local.  But I can imagine this to be known (to the programmer)
to not happen
for the original code and eventually GCC jump-threading some never
reachable path
(we've been there before).

Richard.

> Thank you,
> Martin

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

[Alexandre Oliva]

On Mar  8, 2023, Richard Biener <richard.guenther@gmail.com> wrote:

> On Wed, Mar 8, 2023 at 2:04 PM Martin Liška <mliska@suse.cz> wrote:

>> Is the emitted warning correct?

> For the reduced testcase yes, if !aio_bh_poll_s (or !aio_bh_poll_bh)
> the stored pointer remains local.

*nod*, before the recent patch, it would have failed to issue the
warning in somewhat unpredictable circumstances.

-- 
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>