On Wed, 27 Apr 2022, Richard Biener wrote:
> On Wed, 27 Apr 2022, Jakub Jelinek wrote:
>
> > On Wed, Apr 27, 2022 at 09:43:59AM +0200, Richard Biener wrote:
> > > but not any operations on the pointer value (compare, pointer-plus,
> > > difference, masking, etc.). A simple-minded implementation would
> > > then be
> > >
> > > if ((!gimple_vuse (use_stmt) && warn_dangling_pointer < 3)
> > > || (maybe && ...
> >
> > That would consider as memory uses both cases where we actually dereference
> > the pointer and where we store the pointer in some memory.
> > But perhaps that would be the goal.
>
> I think that was the intention of the diagnostic, yes. I don't think
> it's very useful to diagnose that you add 1 to a dangling pointer,
> instead I hope the code will diagnose the dereference of the offsetted
> dangling pointer instead (I think it does, though it might miss _1 =
> &MEM[_2 + 4]; from a quick look, likewise _2 & ~3 (aligning a pointer)
> and any tricks via uintptr casting).
>
> There's some testsuite fallout with using !gimple_vuse () because
> when a diagnostic is suppressed we are _not_ tracking derived pointers.
>
> For c-c++-common/Wdangling-pointer.c at -O0 we diagnose
>
> void* warn_return_local_addr (void)
> {
> int *p = 0;
> {
> int a[] = { 1, 2, 3 };
> p = a;
> }
>
> /* Unlike the above case, here the pointer is dangling when it's
> used. */
> return p; // { dg-warning "using dangling pointer 'p'
> to 'a'" "array" }
> }
>
> _8 = p_6; // <-- here
>
> <bb3>:
> return _8;
>
> but if we suppress that we do not add _8 to the pointers to check.
> Even if we do add it we get a maybe diagnostic because the
> post-dominator query uses reversed arguments (oops, we should probably
> fix that independently). Fixing that results in
> 'using a dangling pointer to 'a'' (instead of naming 'p') since _8
> is anonymous.
>
> When optimizing we fail to diagnose this case - we skip return stmts
> because of -Wreturn-local-addr but when we do not warn about the
> artificial SSA copy the diagnostic is lost. So that's the only
> "intentional" diagnostic that's skipped by !gimple_vuse ().
>
> Still it requires too much changes to code I'm not familiar with
> at this point.
>
> > > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> > > index 879dbcc1e52..80b5119da7d 100644
> > > --- a/gcc/gimple-ssa-warn-access.cc
> > > +++ b/gcc/gimple-ssa-warn-access.cc
> > > @@ -3923,7 +3923,8 @@ pass_waccess::warn_invalid_pointer (tree ref,
> > > gimple
> > > *use_
> > > stmt,
> > > return;
> > > }
> > >
> > > - if ((maybe && warn_dangling_pointer < 2)
> > > + if (equality
> > > + || (maybe && warn_dangling_pointer < 2)
> > > || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_))
> > > return;
> > >
> >
> > This is fine too with the invoke.texi change and the testcases.
>
> So I'm leaning towards this to fix the P1 bug and see what other
> cases people come up with in the real world.
>
> As said if not introducing =3 I'd not document this implementation
> detail (as said above we miss some cases because we fail to follow
> all pointer adjustments as well).
>
> I'm re-testing the variant below, not documenting the implementation
> detail but fixing the post-dominator queries.
Bootstrapped & tested on x86_64-unknown-linux-gnu.
OK?
Thanks,
Richard.
> Richard.
>
> From 098ab3298b273a56bafe978facdc512789a7628a Mon Sep 17 00:00:00 2001
> From: Richard Biener <[email protected]>
> Date: Mon, 25 Apr 2022 10:46:16 +0200
> Subject: [PATCH] middle-end/104492 - avoid all equality compare dangling
> pointer diags
> To: [email protected]
>
> The following extends the equality compare dangling pointer diagnostics
> suppression for uses following free or realloc to also cover those
> following invalidation of auto variables via CLOBBERs. That avoids
> diagnosing idioms like
>
> return std::find(std::begin(candidates), std::end(candidates), s)
> != std::end(candidates);
>
> for auto candidates which are prone to forwarding of the final
> comparison across the storage invalidation as then seen by the
> late run access warning pass.
>
> 2022-04-25 Richard Biener <[email protected]>
>
> PR middle-end/104492
> * gimple-ssa-warn-access.cc
> (pass_waccess::warn_invalid_pointer): Exclude equality compare
> diagnostics for all kind of invalidations.
> (pass_waccess::check_dangling_uses): Fix post-dominator query.
> (pass_waccess::check_pointer_uses): Likewise.
> ---
> gcc/gimple-ssa-warn-access.cc | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> index 879dbcc1e52..39aa8186de6 100644
> --- a/gcc/gimple-ssa-warn-access.cc
> +++ b/gcc/gimple-ssa-warn-access.cc
> @@ -3923,7 +3923,8 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple
> *use_stmt,
> return;
> }
>
> - if ((maybe && warn_dangling_pointer < 2)
> + if (equality
> + || (maybe && warn_dangling_pointer < 2)
> || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_))
> return;
>
> @@ -4241,7 +4242,7 @@ pass_waccess::check_pointer_uses (gimple *stmt, tree
> ptr,
> basic_block use_bb = gimple_bb (use_stmt);
> bool this_maybe
> = (maybe
> - || !dominated_by_p (CDI_POST_DOMINATORS, use_bb, stmt_bb));
> + || !dominated_by_p (CDI_POST_DOMINATORS, stmt_bb, use_bb));
> warn_invalid_pointer (*use_p->use, use_stmt, stmt, var,
> this_maybe, equality);
> continue;
> @@ -4486,7 +4487,7 @@ pass_waccess::check_dangling_uses (tree var, tree decl,
> bool maybe /* = false */
>
> basic_block use_bb = gimple_bb (use_stmt);
> basic_block clob_bb = gimple_bb (*pclob);
> - maybe = maybe || !dominated_by_p (CDI_POST_DOMINATORS, use_bb, clob_bb);
> + maybe = maybe || !dominated_by_p (CDI_POST_DOMINATORS, clob_bb, use_bb);
> warn_invalid_pointer (var, use_stmt, *pclob, decl, maybe, false);
> }
>
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
