middle-end/104492 - avoid all equality compare dangling pointer diags

Message ID 20220425095435.226C013AE1@imap2.suse-dmz.suse.de
State New
Headers
Series middle-end/104492 - avoid all equality compare dangling pointer diags |

Commit Message

Richard Biener April 25, 2022, 9:54 a.m. UTC
  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.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

OK for trunk?

Thanks,
Richard.

2022-04-25  Richard Biener  <rguenther@suse.de>

	PR middle-end/104492
	* gimple-ssa-warn-access.cc
	(pass_waccess::warn_invalid_pointer): Exclude equality compare
	diagnostics for all kind of invalidations.

	* c-c++-common/Wdangling-pointer.c: Adjust for changed
	suppression.
	* c-c++-common/Wdangling-pointer-2.c: Likewise.
---
 gcc/gimple-ssa-warn-access.cc                    | 14 +++++---------
 gcc/testsuite/c-c++-common/Wdangling-pointer-2.c |  4 ++--
 gcc/testsuite/c-c++-common/Wdangling-pointer.c   |  4 ++--
 3 files changed, 9 insertions(+), 13 deletions(-)
  

Comments

Jakub Jelinek April 27, 2022, 6:42 a.m. UTC | #1
On Mon, Apr 25, 2022 at 11:54:34AM +0200, Richard Biener wrote:
> 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.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> OK for trunk?
> 
> Thanks,
> Richard.
> 
> 2022-04-25  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/104492
> 	* gimple-ssa-warn-access.cc
> 	(pass_waccess::warn_invalid_pointer): Exclude equality compare
> 	diagnostics for all kind of invalidations.
> 
> 	* c-c++-common/Wdangling-pointer.c: Adjust for changed
> 	suppression.
> 	* c-c++-common/Wdangling-pointer-2.c: Likewise.

I spoke with Martin on IRC and his comment was that this is ok
but should be accompanied with a doc/invoke.texi change that clarifies
that behavior in the documentation.
I think that is a reasonable request.

	Jakub
  
Richard Biener April 27, 2022, 7 a.m. UTC | #2
On Wed, 27 Apr 2022, Jakub Jelinek wrote:

> On Mon, Apr 25, 2022 at 11:54:34AM +0200, Richard Biener wrote:
> > 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.
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > 
> > OK for trunk?
> > 
> > Thanks,
> > Richard.
> > 
> > 2022-04-25  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR middle-end/104492
> > 	* gimple-ssa-warn-access.cc
> > 	(pass_waccess::warn_invalid_pointer): Exclude equality compare
> > 	diagnostics for all kind of invalidations.
> > 
> > 	* c-c++-common/Wdangling-pointer.c: Adjust for changed
> > 	suppression.
> > 	* c-c++-common/Wdangling-pointer-2.c: Likewise.
> 
> I spoke with Martin on IRC and his comment was that this is ok
> but should be accompanied with a doc/invoke.texi change that clarifies
> that behavior in the documentation.
> I think that is a reasonable request.

Like so?

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 07b440190c3..4decbf84a43 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8642,8 +8642,10 @@ Warn about uses of pointers (or C++ references) to 
objects with automatic
 storage duration after their lifetime has ended.  This includes local
 variables declared in nested blocks, compound literals and other unnamed
 temporary objects.  In addition, warn about storing the address of such
-objects in escaped pointers.  The warning is enabled at all optimization
-levels but may yield different results with optimization than without.
+objects in escaped pointers.  As exception we do not warn when pointers
+are used in equality compares after the lifetime of the storage they 
point
+to ended.  The warning is enabled at all optimization levels but may 
yield
+different results with optimization than without.
 
 @table @gcctabopt
 @item -Wdangling-pointer=1
  
Jakub Jelinek April 27, 2022, 7:29 a.m. UTC | #3
On Wed, Apr 27, 2022 at 09:00:51AM +0200, Richard Biener wrote:
> > > 2022-04-25  Richard Biener  <rguenther@suse.de>
> > > 
> > > 	PR middle-end/104492
> > > 	* gimple-ssa-warn-access.cc
> > > 	(pass_waccess::warn_invalid_pointer): Exclude equality compare
> > > 	diagnostics for all kind of invalidations.
> > > 
> > > 	* c-c++-common/Wdangling-pointer.c: Adjust for changed
> > > 	suppression.
> > > 	* c-c++-common/Wdangling-pointer-2.c: Likewise.
> > 
> > I spoke with Martin on IRC and his comment was that this is ok
> > but should be accompanied with a doc/invoke.texi change that clarifies
> > that behavior in the documentation.
> > I think that is a reasonable request.
> 
> Like so?
> 
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 07b440190c3..4decbf84a43 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -8642,8 +8642,10 @@ Warn about uses of pointers (or C++ references) to 
> objects with automatic
>  storage duration after their lifetime has ended.  This includes local
>  variables declared in nested blocks, compound literals and other unnamed
>  temporary objects.  In addition, warn about storing the address of such
> -objects in escaped pointers.  The warning is enabled at all optimization
> -levels but may yield different results with optimization than without.
> +objects in escaped pointers.  As exception we do not warn when pointers
> +are used in equality compares after the lifetime of the storage they 
> point
> +to ended.  The warning is enabled at all optimization levels but may 
> yield
> +different results with optimization than without.
>  
>  @table @gcctabopt
>  @item -Wdangling-pointer=1

Reading the patch again, I don't think the above is what the patch does,
but furthermore, I'm not sure the patch is right.

Before your change, the code was about 2 warnings, -Wuse-after-free=
with levels 0 (well, -Wno-use-after-free), 1, 2, 3 where 3 is enabling
equality and the warning is done when the invalidating statement is
a call.

Then there is code for the -Wdangling-pointer= warning with levels 0 (well,
-Wno-dangling-pointer), 1, 2.

Your change moves the -Wuse-after-free= stanza for both warnings including
-Wuse-after-free= suppressions for both warnings and kills the
-Wdangling-pointer= stanza.  I think that means there would be no
difference between -Wdangling-pointer=1 and -Wdangling-pointer=2 anymore
and whether the warning is given for the maybe case (or equality case)
would be instead determined by -Wuse-after-free= level.

I'd say the right thing would be to keep the -Wuse-after-free= stuff as is
and just adjust
-  if ((maybe && warn_dangling_pointer < 2)
+  if ((equality && warn_dangling_pointer < 3)
+      || (maybe && warn_dangling_pointer < 2)
       || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_))
     return;
i.e. basically introduce -Wdangling-pointer=3 level.  That would need
 Wdangling-pointer=
-C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_dangling_pointer) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) IntegerRange(0, 2)
+C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_dangling_pointer) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) IntegerRange(0, 3)
change, documentation what the -Wdangling-pointer=3 level means in
invoke.texi (similar to -Wuse-after-free=3 documentation?) and another
testcase with -Wdangling-pointer=3 in dg-options where it warns also
about equality.

	Jakub
  
Jakub Jelinek April 27, 2022, 7:38 a.m. UTC | #4
On Wed, Apr 27, 2022 at 09:29:21AM +0200, Jakub Jelinek via Gcc-patches wrote:
> I'd say the right thing would be to keep the -Wuse-after-free= stuff as is
> and just adjust
> -  if ((maybe && warn_dangling_pointer < 2)
> +  if ((equality && warn_dangling_pointer < 3)
> +      || (maybe && warn_dangling_pointer < 2)
>        || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_))
>      return;
> i.e. basically introduce -Wdangling-pointer=3 level.  That would need

Or, if the intent is what you've documented that equality comparisons
are never warned about, then just if (equality || (maybe && ...
But I think the extra warning level might be better especially if we
document that the level can have many false positives and explain why.
But if somebody is willing to go through the false positives to find
some needle in the haystack, let it be his choice.

	Jakub
  
Richard Biener April 27, 2022, 7:43 a.m. UTC | #5
On Wed, 27 Apr 2022, Jakub Jelinek wrote:

> On Wed, Apr 27, 2022 at 09:00:51AM +0200, Richard Biener wrote:
> > > > 2022-04-25  Richard Biener  <rguenther@suse.de>
> > > > 
> > > > 	PR middle-end/104492
> > > > 	* gimple-ssa-warn-access.cc
> > > > 	(pass_waccess::warn_invalid_pointer): Exclude equality compare
> > > > 	diagnostics for all kind of invalidations.
> > > > 
> > > > 	* c-c++-common/Wdangling-pointer.c: Adjust for changed
> > > > 	suppression.
> > > > 	* c-c++-common/Wdangling-pointer-2.c: Likewise.
> > > 
> > > I spoke with Martin on IRC and his comment was that this is ok
> > > but should be accompanied with a doc/invoke.texi change that clarifies
> > > that behavior in the documentation.
> > > I think that is a reasonable request.
> > 
> > Like so?
> > 
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index 07b440190c3..4decbf84a43 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -8642,8 +8642,10 @@ Warn about uses of pointers (or C++ references) to 
> > objects with automatic
> >  storage duration after their lifetime has ended.  This includes local
> >  variables declared in nested blocks, compound literals and other unnamed
> >  temporary objects.  In addition, warn about storing the address of such
> > -objects in escaped pointers.  The warning is enabled at all optimization
> > -levels but may yield different results with optimization than without.
> > +objects in escaped pointers.  As exception we do not warn when pointers
> > +are used in equality compares after the lifetime of the storage they 
> > point
> > +to ended.  The warning is enabled at all optimization levels but may 
> > yield
> > +different results with optimization than without.
> >  
> >  @table @gcctabopt
> >  @item -Wdangling-pointer=1
> 
> Reading the patch again, I don't think the above is what the patch does,
> but furthermore, I'm not sure the patch is right.
> 
> Before your change, the code was about 2 warnings, -Wuse-after-free=
> with levels 0 (well, -Wno-use-after-free), 1, 2, 3 where 3 is enabling
> equality and the warning is done when the invalidating statement is
> a call.
> 
> Then there is code for the -Wdangling-pointer= warning with levels 0 (well,
> -Wno-dangling-pointer), 1, 2.
> 
> Your change moves the -Wuse-after-free= stanza for both warnings including
> -Wuse-after-free= suppressions for both warnings and kills the
> -Wdangling-pointer= stanza.

Whoops, that wasn't my itention.

> I think that means there would be no
> difference between -Wdangling-pointer=1 and -Wdangling-pointer=2 anymore
> and whether the warning is given for the maybe case (or equality case)
> would be instead determined by -Wuse-after-free= level.
> 
> I'd say the right thing would be to keep the -Wuse-after-free= stuff as is
> and just adjust
> -  if ((maybe && warn_dangling_pointer < 2)
> +  if ((equality && warn_dangling_pointer < 3)
> +      || (maybe && warn_dangling_pointer < 2)
>        || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_))
>      return;
> i.e. basically introduce -Wdangling-pointer=3 level.  That would need
>  Wdangling-pointer=
> -C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_dangling_pointer) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) IntegerRange(0, 2)
> +C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_dangling_pointer) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) IntegerRange(0, 3)
> change, documentation what the -Wdangling-pointer=3 level means in
> invoke.texi (similar to -Wuse-after-free=3 documentation?) and another
> testcase with -Wdangling-pointer=3 in dg-options where it warns also
> about equality.

I didn't want to introduce -Wdangling-pointer=3 since I think the
case in the PR is just the tip of the iceberg so that we suppress
for equality compares would have been an implementation detail ...

So I'd do the below then.  I think if we want to introduce
-Wdangling-pointer=3 then we should restrict =2 way more by
excluding all non-memory use stmts.  So we'd still cover

 a.x = ptr; // escape
 foo (ptr); // escape
 *ptr = ..; // store
 .. = *ptr; // load

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

I'm going to see whether there's any testsuite coverage for !gimple_vuse
use_stmt.

Richard.

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;
 


> 	Jakub
> 
>
  
Jakub Jelinek April 27, 2022, 7:50 a.m. UTC | #6
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.

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

	Jakub
  
Richard Biener April 27, 2022, 9:05 a.m. UTC | #7
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.

Richard.

From 098ab3298b273a56bafe978facdc512789a7628a Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Mon, 25 Apr 2022 10:46:16 +0200
Subject: [PATCH] middle-end/104492 - avoid all equality compare dangling
 pointer diags
To: gcc-patches@gcc.gnu.org

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  <rguenther@suse.de>

	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 April 27, 2022, 9:56 a.m. UTC | #8
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 <rguenther@suse.de>
> Date: Mon, 25 Apr 2022 10:46:16 +0200
> Subject: [PATCH] middle-end/104492 - avoid all equality compare dangling
>  pointer diags
> To: gcc-patches@gcc.gnu.org
> 
> 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  <rguenther@suse.de>
> 
> 	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);
>  }
>  
>
  
Jakub Jelinek April 27, 2022, 9:59 a.m. UTC | #9
On Wed, Apr 27, 2022 at 11:56:55AM +0200, Richard Biener wrote:
> Bootstrapped & tested on x86_64-unknown-linux-gnu.
> 
> OK?

I think a testcase from the #c0 of the PR would be nice, but it can
be added incrementally, so ok for trunk and unless somebody beats me
to it, I'll try to reduce the testcase.  With a fixed and unfixed compiler
around it might be easier for reduction.

	Jakub
  
Richard Biener April 27, 2022, 10:02 a.m. UTC | #10
On Wed, 27 Apr 2022, Jakub Jelinek wrote:

> On Wed, Apr 27, 2022 at 11:56:55AM +0200, Richard Biener wrote:
> > Bootstrapped & tested on x86_64-unknown-linux-gnu.
> > 
> > OK?
> 
> I think a testcase from the #c0 of the PR would be nice, but it can
> be added incrementally, so ok for trunk and unless somebody beats me
> to it, I'll try to reduce the testcase.  With a fixed and unfixed compiler
> around it might be easier for reduction.

I did that but the reduction result did not resemble the same failure
mode.  I've failed to manually construct a testcase as well.  Possibly
a testcase using libstdc++ but less Qt internals might be possible.

Thanks,
Richard.
  

Patch

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 879dbcc1e52..6c404f18db7 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -3896,13 +3896,13 @@  pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt,
 	return;
     }
 
+  if ((equality && warn_use_after_free < 3)
+      || (maybe && warn_use_after_free < 2)
+      || warning_suppressed_p (use_stmt, OPT_Wuse_after_free))
+    return;
+
   if (is_gimple_call (inval_stmt))
     {
-      if ((equality && warn_use_after_free < 3)
-	  || (maybe && warn_use_after_free < 2)
-	  || warning_suppressed_p (use_stmt, OPT_Wuse_after_free))
-	return;
-
       const tree inval_decl = gimple_call_fndecl (inval_stmt);
 
       if ((ref && warning_at (use_loc, OPT_Wuse_after_free,
@@ -3923,10 +3923,6 @@  pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt,
       return;
     }
 
-  if ((maybe && warn_dangling_pointer < 2)
-      || warning_suppressed_p (use_stmt, OPT_Wdangling_pointer_))
-    return;
-
   if (DECL_NAME (var))
     {
       if ((ref
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-2.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-2.c
index 20f11b227d6..11c939cb086 100644
--- a/gcc/testsuite/c-c++-common/Wdangling-pointer-2.c
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-2.c
@@ -356,7 +356,7 @@  NOIPA void warn_cond_if_else (int i)
     }
   else
    {
-     int b[] = { 3, 4 };      // { dg-message "'b' declared" "pr??????" { xfail *-*-* } }
+     int b[] = { 3, 4 };      // { dg-message "'b' declared" }
      sink (b);
      p = b;
    }
@@ -365,7 +365,7 @@  NOIPA void warn_cond_if_else (int i)
      because after the first diagnostic the code suppresses subsequent
      ones for the same use.  This needs to be fixed.  */
   sink (p);                   // { dg-warning "dangling pointer 'p' to 'a' may be used" }
-                              // { dg-warning "dangling pointer 'p' to 'b' may be used" "pr??????" { xfail *-*-* } .-1 }
+                              // { dg-warning "dangling pointer 'p' to 'b' may be used" "" { target *-*-* } .-1 }
 }
 
 
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer.c b/gcc/testsuite/c-c++-common/Wdangling-pointer.c
index 0a18c3c8249..09e4036a4dd 100644
--- a/gcc/testsuite/c-c++-common/Wdangling-pointer.c
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer.c
@@ -370,7 +370,7 @@  void warn_cond_if_else (int i)
     }
   else
    {
-     int b[] = { 3, 4 };      // { dg-message "'b' declared" "note" { xfail *-*-* } }
+     int b[] = { 3, 4 };      // { dg-message "'b' declared" "note" }
      sink (b);
      p = b;
    }
@@ -379,7 +379,7 @@  void warn_cond_if_else (int i)
      because after the first diagnostic the code suppresses subsequent
      ones for the same use.  This needs to be fixed.  */
   sink (p);                   // { dg-warning "dangling pointer 'p' to 'a' may be used" }
-                              // { dg-warning "dangling pointer 'p' to 'b' may be used" "pr??????" { xfail *-*-* } .-1 }
+                              // { dg-warning "dangling pointer 'p' to 'b' may be used" "" { target *-*-* } .-1 }
 }