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