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

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

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