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 &, 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 stores; + /* Set of aggregates either returned (and their return statements) + or assigned to. */ + hash_map 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 &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 (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 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