[PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

Transforming a by-value arguments to by-reference as GCC does for some
class types can trigger -Wdangling-pointer when the argument is used
to store the address of a local variable.  Since the stored value is
not accessible in the caller the warning is a false positive.

The attached patch handles this case by excluding PARM_DECLs with
the DECL_BY_REFERENCE bit set from consideration.

While testing the patch I noticed some instances of the warning are
uninitentionally duplicated as the pass runs more than once.  To avoid
that, I also introduce warning suppression into the handler for this
instance of the warning.  (There might still be others.)

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-104436.diff --]
[-- Type: text/x-patch, Size: 3612 bytes --]

Avoid -Wdangling-pointer for by-transparent-reference arguments [PR104436].

Resolves:
PR middle-end/104436 - spurious -Wdangling-pointer assigning local address to a class passed by value

gcc/ChangeLog:

	PR middle-end/104436
	* gimple-ssa-warn-access.cc (pass_waccess::check_dangling_stores):
	Check for warning suppression.  Avoid by-value arguments transformed
	into by-transparent-reference.

gcc/testsuite/ChangeLog:

	PR middle-end/104436
	* c-c++-common/Wdangling-pointer-7.c: New test.
	* g++.dg/warn/Wdangling-pointer-4.C: New test.

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 80d41ea4383..0c319a32b70 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4517,6 +4517,9 @@ pass_waccess::check_dangling_stores (basic_block bb,
       if (!stmt)
 	break;
 
+      if (warning_suppressed_p (stmt, OPT_Wdangling_pointer_))
+	continue;
+
       if (is_gimple_call (stmt)
 	  && !(gimple_call_flags (stmt) & (ECF_CONST | ECF_PURE)))
 	/* Avoid looking before nonconst, nonpure calls since those might
@@ -4542,10 +4545,16 @@ pass_waccess::check_dangling_stores (basic_block bb,
 	}
       else if (TREE_CODE (lhs_ref.ref) == SSA_NAME)
 	{
-	  /* Avoid looking at or before stores into unknown objects.  */
 	  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 (DECL_BY_REFERENCE (var))
+	    /* Avoid by-value arguments transformed into by-reference.  */
+	    continue;
+
 	}
       else if (TREE_CODE (lhs_ref.ref) == MEM_REF)
 	{
@@ -4578,6 +4587,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
 		      "storing the address of local variable %qD in %qE",
 		      rhs_ref.ref, lhs))
 	{
+	  suppress_warning (stmt, OPT_Wdangling_pointer_);
+
 	  location_t loc = DECL_SOURCE_LOCATION (rhs_ref.ref);
 	  inform (loc, "%qD declared here", rhs_ref.ref);
 
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-7.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-7.c
new file mode 100644
index 00000000000..433727dd845
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-7.c
@@ -0,0 +1,20 @@
+/* Verify -Wdangling-pointer is issued only once.
+   { dg-do compile }
+   { dg-options "-O -Wall" } */
+
+void *p;
+
+void escape_global_warn_once (void)
+{
+  int x[5];
+
+  p = &x[3];        // { dg-regexp "\[^\n\r\]+: warning: \[^\n\r\]+ \\\[-Wdangling-pointer.?\\\]" "message" }
+}
+
+
+void escape_param_warn_once (void **p)
+{
+  int x[5];
+
+  *p = &x[3];       // { dg-regexp "\[^\n\r\]+: warning: \[^\n\r\]+ \\\[-Wdangling-pointer.?\\\]" "message" }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C
new file mode 100644
index 00000000000..b3d144a9e6d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C
@@ -0,0 +1,34 @@
+/* PR middle-end/104436 - spurious -Wdangling-pointer assigning local
+   address to a class passed by value
+   { dg-do compile }
+   { dg-options "-O1 -Wall" } */
+
+struct S
+{
+  S (void *p): p (p) { }
+  S (const S &s): p (s.p) { }
+
+  void *p;
+};
+
+
+void nowarn_assign_by_value (S s)
+{
+  int i;
+  S t (&i);
+  s = t;            // { dg-bogus "-Wdangling-pointer" }
+}
+
+void nowarn_assign_by_value_arg (S s)
+{
+  S t (&s);
+  s = t;            // { dg-bogus "-Wdangling-pointer" }
+}
+
+
+void warn_assign_local_by_reference (S &s)
+{
+  int i;
+  S t (&i);
+  s = t;            // { dg-warning "-Wdangling-pointer" }
+}

Re: [PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[Jason Merrill]

On 2/8/22 16:59, Martin Sebor wrote:
> Transforming a by-value arguments to by-reference as GCC does for some
> class types can trigger -Wdangling-pointer when the argument is used
> to store the address of a local variable.  Since the stored value is
> not accessible in the caller the warning is a false positive.
> 
> The attached patch handles this case by excluding PARM_DECLs with
> the DECL_BY_REFERENCE bit set from consideration.
> 
> While testing the patch I noticed some instances of the warning are
> uninitentionally duplicated as the pass runs more than once.  To avoid
> that, I also introduce warning suppression into the handler for this
> instance of the warning.  (There might still be others.)

The second test should verify that we do warn about returning 't' from a 
function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.

> +	  tree var = SSA_NAME_VAR (lhs_ref.ref);
> +	  if (DECL_BY_REFERENCE (var))
> +	    /* Avoid by-value arguments transformed into by-reference.  */
> +	    continue;

I wonder if we can we express this property of invisiref parms somewhere 
more general?  I imagine optimizations would find it useful as well. 
Could pointer_query somehow treat the reference as pointing to a 
function-local object?

I previously tried to express this by marking the reference as 
'restrict', but that was wrong 
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).

Jason

Re: [PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[Richard Biener]

On Tue, Feb 8, 2022 at 11:38 PM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 2/8/22 16:59, Martin Sebor wrote:
> > Transforming a by-value arguments to by-reference as GCC does for some
> > class types can trigger -Wdangling-pointer when the argument is used
> > to store the address of a local variable.  Since the stored value is
> > not accessible in the caller the warning is a false positive.
> >
> > The attached patch handles this case by excluding PARM_DECLs with
> > the DECL_BY_REFERENCE bit set from consideration.
> >
> > While testing the patch I noticed some instances of the warning are
> > uninitentionally duplicated as the pass runs more than once.  To avoid
> > that, I also introduce warning suppression into the handler for this
> > instance of the warning.  (There might still be others.)
>
> The second test should verify that we do warn about returning 't' from a
> function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>
> > +       tree var = SSA_NAME_VAR (lhs_ref.ref);
> > +       if (DECL_BY_REFERENCE (var))

I think you need to test var && TREE_CODE (var) == PARM_DECL here since
for DECL_BY_REFERENCE RESULT_DECL we _do_ escape to the caller.  Also
SSA_NAME_VAR var might be NULL.

> > +         /* Avoid by-value arguments transformed into by-reference.  */
> > +         continue;
>
> I wonder if we can we express this property of invisiref parms somewhere
> more general?  I imagine optimizations would find it useful as well.
> Could pointer_query somehow treat the reference as pointing to a
> function-local object?

I think points-to analysis got this correct when the reference was marked
restrict but now it also fails at this, making DSE fail to eliminate the
store in

struct A { A(); ~A(); int *p; };

void foo (struct A a, int *p)
{
  a.p = p;
}

> I previously tried to express this by marking the reference as
> 'restrict', but that was wrong
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
>
> Jason
>

Re: [PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[Jason Merrill]

On 2/9/22 03:30, Richard Biener wrote:
> On Tue, Feb 8, 2022 at 11:38 PM Jason Merrill via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 2/8/22 16:59, Martin Sebor wrote:
>>> Transforming a by-value arguments to by-reference as GCC does for some
>>> class types can trigger -Wdangling-pointer when the argument is used
>>> to store the address of a local variable.  Since the stored value is
>>> not accessible in the caller the warning is a false positive.
>>>
>>> The attached patch handles this case by excluding PARM_DECLs with
>>> the DECL_BY_REFERENCE bit set from consideration.
>>>
>>> While testing the patch I noticed some instances of the warning are
>>> uninitentionally duplicated as the pass runs more than once.  To avoid
>>> that, I also introduce warning suppression into the handler for this
>>> instance of the warning.  (There might still be others.)
>>
>> The second test should verify that we do warn about returning 't' from a
>> function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>>
>>> +       tree var = SSA_NAME_VAR (lhs_ref.ref);
>>> +       if (DECL_BY_REFERENCE (var))
> 
> I think you need to test var && TREE_CODE (var) == PARM_DECL here since
> for DECL_BY_REFERENCE RESULT_DECL we _do_ escape to the caller.  Also
> SSA_NAME_VAR var might be NULL.
> 
>>> +         /* Avoid by-value arguments transformed into by-reference.  */
>>> +         continue;
>>
>> I wonder if we can we express this property of invisiref parms somewhere
>> more general?  I imagine optimizations would find it useful as well.
>> Could pointer_query somehow treat the reference as pointing to a
>> function-local object?
> 
> I think points-to analysis got this correct when the reference was marked
> restrict but now it also fails at this, making DSE fail to eliminate the
> store in
> 
> struct A { A(); ~A(); int *p; };
> 
> void foo (struct A a, int *p)
> {
>    a.p = p;
> }

Well, that's conservatively correct; since we don't know the definition 
of ~A, we don't know whether it copies p somewhere, e.g.

int *global_p;
A::~A() { global_p = p; }

in which case eliminating the store would be an invalid optimization, 
just as it would be if 'a' were a local variable.

>> I previously tried to express this by marking the reference as
>> 'restrict', but that was wrong
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).

Re: [PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

On 2/8/22 15:37, Jason Merrill wrote:
> On 2/8/22 16:59, Martin Sebor wrote:
>> Transforming a by-value arguments to by-reference as GCC does for some
>> class types can trigger -Wdangling-pointer when the argument is used
>> to store the address of a local variable.  Since the stored value is
>> not accessible in the caller the warning is a false positive.
>>
>> The attached patch handles this case by excluding PARM_DECLs with
>> the DECL_BY_REFERENCE bit set from consideration.
>>
>> While testing the patch I noticed some instances of the warning are
>> uninitentionally duplicated as the pass runs more than once.  To avoid
>> that, I also introduce warning suppression into the handler for this
>> instance of the warning.  (There might still be others.)
> 
> The second test should verify that we do warn about returning 't' from a 
> function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.

The indirect aggregate case isn't handled and needs more work but
since you brought it up I thought I should look into finishing it.
The attached patch #2 adds support for it.  It also incorporates
Richard's suggestion to test PARM_DECL.  Patch #2 applies on top
of patch #1 which is unchanged from the first revision.

I have retested it on x86_64-linux and by building Glibc and
Binutils + GDB.

If now is too late for the aggregate enhancement I'm okay with
deferring it until stage 1.

> 
>> +      tree var = SSA_NAME_VAR (lhs_ref.ref);
>> +      if (DECL_BY_REFERENCE (var))
>> +        /* Avoid by-value arguments transformed into by-reference.  */
>> +        continue;
> 
> I wonder if we can we express this property of invisiref parms somewhere 
> more general?  I imagine optimizations would find it useful as well. 
> Could pointer_query somehow treat the reference as pointing to a 
> function-local object?

I don't quite see where in the pointer_query class this would be
useful (the class also isn't used for optimization).  I could add
a helper to the access_ref class to query this property in warning
code but as this is the only potential client I'm also not sure
that's quite what you had in mind.  I'd need some guidance as to
what you're thinking of here.

Martin


> 
> I previously tried to express this by marking the reference as 
> 'restrict', but that was wrong 
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
> 
> Jason
> 

[-- Attachment #2: 0001-Avoid-Wdangling-pointer-for-by-transparent-reference.diff --]
[-- Type: text/x-patch, Size: 4138 bytes --]

Subject: [PATCH 1/2] Avoid -Wdangling-pointer for by-transparent-reference
 arguments [PR104436].

This change avoids -Wdangling-pointer for by-value arguments transformed
into by-transparent-reference.

Resolves:
PR middle-end/104436 - spurious -Wdangling-pointer assigning local address to a class passed by value

gcc/ChangeLog:

	PR middle-end/104436
	* gimple-ssa-warn-access.cc (pass_waccess::check_dangling_stores):
	Check for warning suppression.  Avoid by-value arguments transformed
	into by-transparent-reference.

gcc/testsuite/ChangeLog:

	PR middle-end/104436
	* c-c++-common/Wdangling-pointer-7.c: New test.
	* g++.dg/warn/Wdangling-pointer-4.C: New test.
---
 gcc/gimple-ssa-warn-access.cc                 | 13 ++++++-
 .../c-c++-common/Wdangling-pointer-7.c        | 20 +++++++++++
 .../g++.dg/warn/Wdangling-pointer-4.C         | 34 +++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wdangling-pointer-7.c
 create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 80d41ea4383..0c319a32b70 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4517,6 +4517,9 @@ pass_waccess::check_dangling_stores (basic_block bb,
       if (!stmt)
 	break;
 
+      if (warning_suppressed_p (stmt, OPT_Wdangling_pointer_))
+	continue;
+
       if (is_gimple_call (stmt)
 	  && !(gimple_call_flags (stmt) & (ECF_CONST | ECF_PURE)))
 	/* Avoid looking before nonconst, nonpure calls since those might
@@ -4542,10 +4545,16 @@ pass_waccess::check_dangling_stores (basic_block bb,
 	}
       else if (TREE_CODE (lhs_ref.ref) == SSA_NAME)
 	{
-	  /* Avoid looking at or before stores into unknown objects.  */
 	  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 (DECL_BY_REFERENCE (var))
+	    /* Avoid by-value arguments transformed into by-reference.  */
+	    continue;
+
 	}
       else if (TREE_CODE (lhs_ref.ref) == MEM_REF)
 	{
@@ -4578,6 +4587,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
 		      "storing the address of local variable %qD in %qE",
 		      rhs_ref.ref, lhs))
 	{
+	  suppress_warning (stmt, OPT_Wdangling_pointer_);
+
 	  location_t loc = DECL_SOURCE_LOCATION (rhs_ref.ref);
 	  inform (loc, "%qD declared here", rhs_ref.ref);
 
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-7.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-7.c
new file mode 100644
index 00000000000..433727dd845
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-7.c
@@ -0,0 +1,20 @@
+/* Verify -Wdangling-pointer is issued only once.
+   { dg-do compile }
+   { dg-options "-O -Wall" } */
+
+void *p;
+
+void escape_global_warn_once (void)
+{
+  int x[5];
+
+  p = &x[3];        // { dg-regexp "\[^\n\r\]+: warning: \[^\n\r\]+ \\\[-Wdangling-pointer.?\\\]" "message" }
+}
+
+
+void escape_param_warn_once (void **p)
+{
+  int x[5];
+
+  *p = &x[3];       // { dg-regexp "\[^\n\r\]+: warning: \[^\n\r\]+ \\\[-Wdangling-pointer.?\\\]" "message" }
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C
new file mode 100644
index 00000000000..b3d144a9e6d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C
@@ -0,0 +1,34 @@
+/* PR middle-end/104436 - spurious -Wdangling-pointer assigning local
+   address to a class passed by value
+   { dg-do compile }
+   { dg-options "-O1 -Wall" } */
+
+struct S
+{
+  S (void *p): p (p) { }
+  S (const S &s): p (s.p) { }
+
+  void *p;
+};
+
+
+void nowarn_assign_by_value (S s)
+{
+  int i;
+  S t (&i);
+  s = t;            // { dg-bogus "-Wdangling-pointer" }
+}
+
+void nowarn_assign_by_value_arg (S s)
+{
+  S t (&s);
+  s = t;            // { dg-bogus "-Wdangling-pointer" }
+}
+
+
+void warn_assign_local_by_reference (S &s)
+{
+  int i;
+  S t (&i);
+  s = t;            // { dg-warning "-Wdangling-pointer" }
+}
-- 
2.34.1


[-- Attachment #3: 0002-Handle-aggregates-in-Wdangling-pointer.diff --]
[-- Type: text/x-patch, Size: 10477 bytes --]

Subject: [PATCH 2/2] Handle aggregates in -Wdangling-pointer.

This set of changes enables -Wdangling-pointer for returning an aggregate
one of whose members stores the address of an auto variables.

gcc/ChangeLog:

	* gimple-ssa-warn-access.cc (pass_waccess::deferred_sets): New type.
	(pass_waccess::check_dangling_stores): Use it as an argument.
	Handle returning and storing aggregates with members storing local
	addresses.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wdangling-pointer-4.C: Add a test case.
	* c-c++-common/Wdangling-pointer-8.c: New test.
---
 gcc/gimple-ssa-warn-access.cc                 | 110 ++++++++++++++----
 .../c-c++-common/Wdangling-pointer-8.c        |  50 ++++++++
 .../g++.dg/warn/Wdangling-pointer-4.C         |  65 +++++++++--
 3 files changed, 192 insertions(+), 33 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wdangling-pointer-8.c

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 0c319a32b70..b675fa13960 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -2130,7 +2130,20 @@ private:
   void check_dangling_uses (tree, tree, bool = false, bool = false);
   void check_dangling_uses ();
   void check_dangling_stores ();
-  void check_dangling_stores (basic_block, hash_set<tree> &, auto_bitmap &);
+
+  /* Holds data used by check_dangling_stores.  */
+  struct deferred_sets
+  {
+    /* Visited basic blocks.  */
+    auto_bitmap bbs;
+    /* Set of stores already seen.  */
+    hash_set<tree> stores;
+    /* Set of aggregates either returned (and their return statements)
+       or assigned to.  */
+    hash_map<tree, gimple *> aggrs;
+  };
+
+  void check_dangling_stores (basic_block, deferred_sets &);
 
   void warn_invalid_pointer (tree, gimple *, gimple *, tree, bool, bool = false);
 
@@ -4498,14 +4511,13 @@ pass_waccess::check_dangling_uses (tree var, tree decl, bool maybe /* = false */
 
 /* Diagnose stores in BB and (recursively) its predecessors of the addresses
    of local variables into nonlocal pointers that are left dangling after
-   the function returns.  BBS is a bitmap of basic blocks visited.  */
+   the function returns.  DS contains data to control recursion and for
+   handling aggregates.  */
 
 void
-pass_waccess::check_dangling_stores (basic_block bb,
-				     hash_set<tree> &stores,
-				     auto_bitmap &bbs)
+pass_waccess::check_dangling_stores (basic_block bb, deferred_sets &ds)
 {
-  if (!bitmap_set_bit (bbs, bb->index))
+  if (!bitmap_set_bit (ds.bbs, bb->index))
     /* Avoid cycles. */
     return;
 
@@ -4526,6 +4538,23 @@ pass_waccess::check_dangling_stores (basic_block bb,
 	   use the escaped locals.  */
 	return;
 
+      if (greturn *ret = dyn_cast <greturn *> (stmt))
+	{
+	  /* If the statement returns an aggregate add it to AGGRs to
+	     check for assignments of dangling pointers in subsequent
+	     iterations.  Otherwise simply continue.  */
+	  tree arg = gimple_return_retval (ret);
+	  if (!arg)
+	    continue;
+
+	  tree type = TREE_TYPE (arg);
+	  if (TREE_CODE (type) == REFERENCE_TYPE)
+	    type = TREE_TYPE (type);
+	  if (RECORD_OR_UNION_TYPE_P (type))
+	    ds.aggrs.put (arg, stmt);
+	  continue;
+	}
+
       if (!is_gimple_assign (stmt) || gimple_clobber_p (stmt))
 	continue;
 
@@ -4534,13 +4563,17 @@ pass_waccess::check_dangling_stores (basic_block bb,
       if (!m_ptr_qry.get_ref (lhs, stmt, &lhs_ref, 0))
 	continue;
 
-      if (is_auto_decl (lhs_ref.ref))
+      /* Set if LHS is a local aggregate one of whose members has been
+	 set to the address of a dangling local.  */
+      gimple **aggr_ret = ds.aggrs.get (lhs_ref.ref);
+      if (is_auto_decl (lhs_ref.ref) && !aggr_ret)
 	continue;
 
       if (DECL_P (lhs_ref.ref))
 	{
-	  if (!POINTER_TYPE_P (TREE_TYPE (lhs_ref.ref))
-	      || lhs_ref.deref > 0)
+	  if (!aggr_ret
+	      && (!POINTER_TYPE_P (TREE_TYPE (lhs_ref.ref))
+		  || lhs_ref.deref > 0))
 	    continue;
 	}
       else if (TREE_CODE (lhs_ref.ref) == SSA_NAME)
@@ -4551,7 +4584,7 @@ pass_waccess::check_dangling_stores (basic_block bb,
 	    return;
 
 	  tree var = SSA_NAME_VAR (lhs_ref.ref);
-	  if (DECL_BY_REFERENCE (var))
+	  if (TREE_CODE (var) == PARM_DECL && DECL_BY_REFERENCE (var))
 	    /* Avoid by-value arguments transformed into by-reference.  */
 	    continue;
 
@@ -4569,19 +4602,36 @@ pass_waccess::check_dangling_stores (basic_block bb,
       else
 	continue;
 
-      if (stores.add (lhs_ref.ref))
+      if (ds.stores.add (lhs_ref.ref))
 	continue;
 
       /* FIXME: Handle stores of alloca() and VLA.  */
       access_ref rhs_ref;
       tree rhs = gimple_assign_rhs1 (stmt);
-      if (!m_ptr_qry.get_ref (rhs, stmt, &rhs_ref, 0)
-	  || rhs_ref.deref != -1)
+      if (!m_ptr_qry.get_ref (rhs, stmt, &rhs_ref, 0))
 	continue;
 
+      tree rhs_type = TREE_TYPE (rhs);
+      if (POINTER_TYPE_P (rhs_type))
+	{
+	  if (rhs_ref.deref != -1)
+	    continue;
+	}
+      else
+	{
+	  if (RECORD_OR_UNION_TYPE_P (rhs_type))
+	    ds.aggrs.put (rhs_ref.ref, stmt);
+	  continue;
+	}
+
       if (!is_auto_decl (rhs_ref.ref))
 	continue;
 
+      if (warning_suppressed_p (rhs_ref.ref, OPT_Wdangling_pointer_))
+	/* Avoid warning for a variable that's already been diagnosed
+	   (see below).  */
+	continue;
+
       location_t loc = gimple_location (stmt);
       if (warning_at (loc, OPT_Wdangling_pointer_,
 		      "storing the address of local variable %qD in %qE",
@@ -4589,16 +4639,31 @@ pass_waccess::check_dangling_stores (basic_block bb,
 	{
 	  suppress_warning (stmt, OPT_Wdangling_pointer_);
 
-	  location_t loc = DECL_SOURCE_LOCATION (rhs_ref.ref);
-	  inform (loc, "%qD declared here", rhs_ref.ref);
+	  location_t ref_loc = DECL_SOURCE_LOCATION (rhs_ref.ref);
+	  inform (ref_loc, "%qD declared here", rhs_ref.ref);
+
+	  if (aggr_ret)
+	    {
+	      /* Also suppress the warning for the variable whose address
+		 is being (indirectly) returned to keep subsequent runs
+		 over the IL after optimization from issuing the same
+		 warning again.  */
+	      suppress_warning (rhs_ref.ref, OPT_Wdangling_pointer_);
+
+	      location_t ret_loc = gimple_location (*aggr_ret);
+	      inform (ret_loc, "%qE returned here", lhs_ref.ref);
+	      continue;
+	    }
 
 	  if (DECL_P (lhs_ref.ref))
-	    loc = DECL_SOURCE_LOCATION (lhs_ref.ref);
+	    ref_loc = DECL_SOURCE_LOCATION (lhs_ref.ref);
 	  else if (EXPR_HAS_LOCATION (lhs_ref.ref))
-	    loc = EXPR_LOCATION (lhs_ref.ref);
+	    ref_loc = EXPR_LOCATION (lhs_ref.ref);
+	  else
+	    ref_loc = UNKNOWN_LOCATION;
 
-	  if (loc != UNKNOWN_LOCATION)
-	    inform (loc, "%qE declared here", lhs_ref.ref);
+	  if (ref_loc != UNKNOWN_LOCATION && ref_loc != loc)
+	    inform (ref_loc, "%qE declared here", lhs_ref.ref);
 	}
     }
 
@@ -4607,7 +4672,7 @@ pass_waccess::check_dangling_stores (basic_block bb,
   FOR_EACH_EDGE (e, ei, bb->preds)
     {
       basic_block pred = e->src;
-      check_dangling_stores (pred, stores, bbs);
+      check_dangling_stores (pred, ds);
     }
 }
 
@@ -4617,9 +4682,8 @@ pass_waccess::check_dangling_stores (basic_block bb,
 void
 pass_waccess::check_dangling_stores ()
 {
-  auto_bitmap bbs;
-  hash_set<tree> stores;
-  check_dangling_stores (EXIT_BLOCK_PTR_FOR_FN (m_func), stores, bbs);
+  deferred_sets ds;
+  check_dangling_stores (EXIT_BLOCK_PTR_FOR_FN (m_func), ds);
 }
 
 /* Check for and diagnose uses of dangling pointers to auto objects
diff --git a/gcc/testsuite/c-c++-common/Wdangling-pointer-8.c b/gcc/testsuite/c-c++-common/Wdangling-pointer-8.c
new file mode 100644
index 00000000000..6495515d629
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wdangling-pointer-8.c
@@ -0,0 +1,50 @@
+/* Verify -Wdangling-pointer for returning a struct with the address
+   of a local variable.
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+#if __cplusplus
+#  define EXTERN_C extern "C"
+#else
+#  define EXTERN_C extern
+#endif
+
+EXTERN_C void* memset (void*, int, __SIZE_TYPE__);
+
+struct S
+{
+  void *p, *q;
+};
+
+
+extern int eia[];
+
+struct S nowarn_eia (void)
+{
+  struct S s = { eia, eia };
+  return s;
+}
+
+
+struct S warn_eia_ia (void)
+{
+  int ia[1];                  // { dg-note "'ia' declared here" "note" }
+  struct S s = { eia, ia };   // { dg-warning "storing the address of local variable 'ia' in 's\.\(S::\)?q'" }
+  return s;                   // { dg-note "'s' returned here" "note" }
+}
+
+struct S nowarn_ia_ia_memset (void)
+{
+  int ia[1];
+  struct S s = { ia, ia };
+  memset (&s, 0, sizeof s);
+  return s;
+}
+
+struct S nowarn_ia_ia_reset (void)
+{
+  int ia[1];
+  struct S s = { ia, ia };
+  s = (struct S) { };
+  return s;
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C b/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C
index b3d144a9e6d..0aa2a19698b 100644
--- a/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C
+++ b/gcc/testsuite/g++.dg/warn/Wdangling-pointer-4.C
@@ -3,32 +3,77 @@
    { dg-do compile }
    { dg-options "-O1 -Wall" } */
 
-struct S
+struct A
 {
-  S (void *p): p (p) { }
-  S (const S &s): p (s.p) { }
+  A (void *p): p (p) { }
+  A (const A &s): p (s.p) { }
 
   void *p;
 };
 
+void sink (void*);
 
-void nowarn_assign_by_value (S s)
+void nowarn_assign_by_value (A s)
 {
   int i;
-  S t (&i);
+  A t (&i);
   s = t;            // { dg-bogus "-Wdangling-pointer" }
 }
 
-void nowarn_assign_by_value_arg (S s)
+void nowarn_assign_by_value_arg (A s)
 {
-  S t (&s);
+  A t (&s);
   s = t;            // { dg-bogus "-Wdangling-pointer" }
 }
 
 
-void warn_assign_local_by_reference (S &s)
+void warn_assign_local_by_reference (A &s)
 {
-  int i;
-  S t (&i);
+  int i;            // { dg-note "'i' declared here" }
+  A t (&i);
   s = t;            // { dg-warning "-Wdangling-pointer" }
 }
+
+
+struct B
+{
+  B (void *p)
+    : p (p)         // { dg-warning "-Wdangling-pointer" }
+  { }
+
+  B (const B &b): p (b.p) { }
+
+  void *p;
+};
+
+B warn_assign_return ()
+{
+  int i;            // { dg-note "'i' declared here" }
+  B b (&i);
+  *(int*)b.p = 1;
+  return b;         // { dg-note "returned here" }
+}
+
+
+A nowarn_assign_sink_return ()
+{
+  int i;
+  A a (&i);
+  sink (&a);
+  return a;
+}
+
+
+A warn_zero_assign_return (int x)
+{
+  int i = 0;        // { dg-message "'i' declared here" }
+  A a (0);
+  sink (&a);
+  a.p = &i;         // { dg-warning "-Wdangling-pointer" }
+  if (x)
+    i = x;
+  else
+    i = ++*(int*)a.p;
+
+  return a;         // { dg-message "returned here" }
+}
-- 
2.34.1

PING [PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[Martin Sebor]

Pinging the two patches here:
https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590230.html

On 2/10/22 16:04, Martin Sebor wrote:
> On 2/8/22 15:37, Jason Merrill wrote:
>> On 2/8/22 16:59, Martin Sebor wrote:
>>> Transforming a by-value arguments to by-reference as GCC does for some
>>> class types can trigger -Wdangling-pointer when the argument is used
>>> to store the address of a local variable.  Since the stored value is
>>> not accessible in the caller the warning is a false positive.
>>>
>>> The attached patch handles this case by excluding PARM_DECLs with
>>> the DECL_BY_REFERENCE bit set from consideration.
>>>
>>> While testing the patch I noticed some instances of the warning are
>>> uninitentionally duplicated as the pass runs more than once.  To avoid
>>> that, I also introduce warning suppression into the handler for this
>>> instance of the warning.  (There might still be others.)
>>
>> The second test should verify that we do warn about returning 't' from 
>> a function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
> 
> The indirect aggregate case isn't handled and needs more work but
> since you brought it up I thought I should look into finishing it.
> The attached patch #2 adds support for it.  It also incorporates
> Richard's suggestion to test PARM_DECL.  Patch #2 applies on top
> of patch #1 which is unchanged from the first revision.
> 
> I have retested it on x86_64-linux and by building Glibc and
> Binutils + GDB.
> 
> If now is too late for the aggregate enhancement I'm okay with
> deferring it until stage 1.
> 
>>
>>> +      tree var = SSA_NAME_VAR (lhs_ref.ref);
>>> +      if (DECL_BY_REFERENCE (var))
>>> +        /* Avoid by-value arguments transformed into by-reference.  */
>>> +        continue;
>>
>> I wonder if we can we express this property of invisiref parms 
>> somewhere more general?  I imagine optimizations would find it useful 
>> as well. Could pointer_query somehow treat the reference as pointing 
>> to a function-local object?
> 
> I don't quite see where in the pointer_query class this would be
> useful (the class also isn't used for optimization).  I could add
> a helper to the access_ref class to query this property in warning
> code but as this is the only potential client I'm also not sure
> that's quite what you had in mind.  I'd need some guidance as to
> what you're thinking of here.
> 
> Martin
> 
> 
>>
>> I previously tried to express this by marking the reference as 
>> 'restrict', but that was wrong 
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
>>
>> Jason
>>

Re: [PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[Richard Biener]

On Fri, Feb 11, 2022 at 12:05 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 2/8/22 15:37, Jason Merrill wrote:
> > On 2/8/22 16:59, Martin Sebor wrote:
> >> Transforming a by-value arguments to by-reference as GCC does for some
> >> class types can trigger -Wdangling-pointer when the argument is used
> >> to store the address of a local variable.  Since the stored value is
> >> not accessible in the caller the warning is a false positive.
> >>
> >> The attached patch handles this case by excluding PARM_DECLs with
> >> the DECL_BY_REFERENCE bit set from consideration.
> >>
> >> While testing the patch I noticed some instances of the warning are
> >> uninitentionally duplicated as the pass runs more than once.  To avoid
> >> that, I also introduce warning suppression into the handler for this
> >> instance of the warning.  (There might still be others.)
> >
> > The second test should verify that we do warn about returning 't' from a
> > function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>
> The indirect aggregate case isn't handled and needs more work but
> since you brought it up I thought I should look into finishing it.
> The attached patch #2 adds support for it.  It also incorporates
> Richard's suggestion to test PARM_DECL.  Patch #2 applies on top
> of patch #1 which is unchanged from the first revision.

patch #1 would be OK if you'd do the PARM_DECL check there - I'd
rather defer #2 to stage1.

Richard.

>
> I have retested it on x86_64-linux and by building Glibc and
> Binutils + GDB.
>
> If now is too late for the aggregate enhancement I'm okay with
> deferring it until stage 1.
>
> >
> >> +      tree var = SSA_NAME_VAR (lhs_ref.ref);
> >> +      if (DECL_BY_REFERENCE (var))
> >> +        /* Avoid by-value arguments transformed into by-reference.  */
> >> +        continue;
> >
> > I wonder if we can we express this property of invisiref parms somewhere
> > more general?  I imagine optimizations would find it useful as well.
> > Could pointer_query somehow treat the reference as pointing to a
> > function-local object?
>
> I don't quite see where in the pointer_query class this would be
> useful (the class also isn't used for optimization).  I could add
> a helper to the access_ref class to query this property in warning
> code but as this is the only potential client I'm also not sure
> that's quite what you had in mind.  I'd need some guidance as to
> what you're thinking of here.
>
> Martin
>
>
> >
> > I previously tried to express this by marking the reference as
> > 'restrict', but that was wrong
> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
> >
> > Jason
> >

Re: [PATCH] handle "invisible" reference in -Wdangling-pointer (PR104436)

[Martin Sebor]

On 3/9/22 06:17, Richard Biener wrote:
> On Fri, Feb 11, 2022 at 12:05 AM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 2/8/22 15:37, Jason Merrill wrote:
>>> On 2/8/22 16:59, Martin Sebor wrote:
>>>> Transforming a by-value arguments to by-reference as GCC does for some
>>>> class types can trigger -Wdangling-pointer when the argument is used
>>>> to store the address of a local variable.  Since the stored value is
>>>> not accessible in the caller the warning is a false positive.
>>>>
>>>> The attached patch handles this case by excluding PARM_DECLs with
>>>> the DECL_BY_REFERENCE bit set from consideration.
>>>>
>>>> While testing the patch I noticed some instances of the warning are
>>>> uninitentionally duplicated as the pass runs more than once.  To avoid
>>>> that, I also introduce warning suppression into the handler for this
>>>> instance of the warning.  (There might still be others.)
>>>
>>> The second test should verify that we do warn about returning 't' from a
>>> function; we don't want to ignore the DECL_BY_REFERENCE RESULT_DECL.
>>
>> The indirect aggregate case isn't handled and needs more work but
>> since you brought it up I thought I should look into finishing it.
>> The attached patch #2 adds support for it.  It also incorporates
>> Richard's suggestion to test PARM_DECL.  Patch #2 applies on top
>> of patch #1 which is unchanged from the first revision.
> 
> patch #1 would be OK if you'd do the PARM_DECL check there - I'd
> rather defer #2 to stage1.
> 
> Richard.
> 
>>
>> I have retested it on x86_64-linux and by building Glibc and
>> Binutils + GDB.
>>
>> If now is too late for the aggregate enhancement I'm okay with
>> deferring it until stage 1.

I committed the adjusted patch in r12-7650.  I'll try to remember to
ping the second patch in stage 1.

Martin

>>
>>>
>>>> +      tree var = SSA_NAME_VAR (lhs_ref.ref);
>>>> +      if (DECL_BY_REFERENCE (var))
>>>> +        /* Avoid by-value arguments transformed into by-reference.  */
>>>> +        continue;
>>>
>>> I wonder if we can we express this property of invisiref parms somewhere
>>> more general?  I imagine optimizations would find it useful as well.
>>> Could pointer_query somehow treat the reference as pointing to a
>>> function-local object?
>>
>> I don't quite see where in the pointer_query class this would be
>> useful (the class also isn't used for optimization).  I could add
>> a helper to the access_ref class to query this property in warning
>> code but as this is the only potential client I'm also not sure
>> that's quite what you had in mind.  I'd need some guidance as to
>> what you're thinking of here.
>>
>> Martin
>>
>>
>>>
>>> I previously tried to express this by marking the reference as
>>> 'restrict', but that was wrong
>>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97474).
>>>
>>> Jason
>>>