[v2] warn-access: Prevent -Wuse-after-free on ARM [PR104213]
Commit Message
On Wed, Jan 26, 2022 at 09:39:46AM -0700, Martin Sebor wrote:
> On 1/26/22 08:24, Jason Merrill via Gcc-patches wrote:
> > On 1/25/22 18:33, Marek Polacek wrote:
> > > Here, -Wuse-after-free warns about using 'this' which, on ARM, cdtors
> > > return, as mandated by the EABI. To be entirely correct, it only
> > > requires it for C1 and C2 ctors and D2 and D1 dtors, but I don't feel
> > > like changing that now and possibly running into issues later on.
> > >
> > > This patch uses suppress_warning on 'this' for certain cdtor_returns_this
> > > cases in the C++ FE, and then warn_invalid_pointer makes use of this
> > > information and doesn't warn.
> > >
> > > In my first attempt I tried suppress_warning the MODIFY_EXPR or
> > > RETURN_EXPR
> > > we build in build_delete_destructor_body, but the complication is that
> > > the suppress_warning bits don't always survive gimplification; see e.g.
> > > gimplify_modify_expr where we do
> > >
> > > 6130 if (COMPARISON_CLASS_P (*from_p))
> > > 6131 copy_warning (assign, *from_p);
> > >
> > > but here we're not dealing with a comparison. Removing that check
> > > regresses uninit-pr74762.C. Adding copy_warning (assign, *expr_p)
> > > regresses c-c++-common/uninit-17.c.
> > >
> > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> >
> > OK if Martin doesn't have any suggestions.
>
> Nothing further from me.
Here's what I've pushed then (it adds the testcase Martin mentioned earlier).
Thanks,
-- >8 --
Here, -Wuse-after-free warns about using 'this' which, on ARM, cdtors
return, as mandated by the EABI. To be entirely correct, it only
requires it for C1 and C2 ctors and D2 and D1 dtors, but I don't feel
like changing that now and possibly running into issues later on.
This patch uses suppress_warning on 'this' for certain cdtor_returns_this
cases in the C++ FE, and then warn_invalid_pointer makes use of this
information and doesn't warn.
In my first attempt I tried suppress_warning the MODIFY_EXPR or RETURN_EXPR
we build in build_delete_destructor_body, but the complication is that
the suppress_warning bits don't always survive gimplification; see e.g.
gimplify_modify_expr where we do
6130 if (COMPARISON_CLASS_P (*from_p))
6131 copy_warning (assign, *from_p);
but here we're not dealing with a comparison. Removing that check
regresses uninit-pr74762.C. Adding copy_warning (assign, *expr_p)
regresses c-c++-common/uninit-17.c.
PR target/104213
gcc/cp/ChangeLog:
* decl.cc (finish_constructor_body): Suppress -Wuse-after-free.
(finish_destructor_body): Likewise.
* optimize.cc (build_delete_destructor_body): Likewise.
gcc/ChangeLog:
* gimple-ssa-warn-access.cc (pass_waccess::warn_invalid_pointer): Don't
warn when the SSA_NAME_VAR of REF has supressed -Wuse-after-free.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wuse-after-free2.C: New test.
* g++.dg/warn/Wuse-after-free3.C: New test.
---
gcc/cp/decl.cc | 2 ++
gcc/cp/optimize.cc | 1 +
gcc/gimple-ssa-warn-access.cc | 14 +++++++++++---
gcc/testsuite/g++.dg/warn/Wuse-after-free2.C | 10 ++++++++++
gcc/testsuite/g++.dg/warn/Wuse-after-free3.C | 16 ++++++++++++++++
5 files changed, 40 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wuse-after-free2.C
create mode 100644 gcc/testsuite/g++.dg/warn/Wuse-after-free3.C
base-commit: 00d8321124123daf41f7c51526355a5a610cdeb8
@@ -17315,6 +17315,7 @@ finish_constructor_body (void)
add_stmt (build_stmt (input_location, LABEL_EXPR, cdtor_label));
val = DECL_ARGUMENTS (current_function_decl);
+ suppress_warning (val, OPT_Wuse_after_free);
val = build2 (MODIFY_EXPR, TREE_TYPE (val),
DECL_RESULT (current_function_decl), val);
/* Return the address of the object. */
@@ -17408,6 +17409,7 @@ finish_destructor_body (void)
tree val;
val = DECL_ARGUMENTS (current_function_decl);
+ suppress_warning (val, OPT_Wuse_after_free);
val = build2 (MODIFY_EXPR, TREE_TYPE (val),
DECL_RESULT (current_function_decl), val);
/* Return the address of the object. */
@@ -166,6 +166,7 @@ build_delete_destructor_body (tree delete_dtor, tree complete_dtor)
if (targetm.cxx.cdtor_returns_this ())
{
tree val = DECL_ARGUMENTS (delete_dtor);
+ suppress_warning (val, OPT_Wuse_after_free);
val = build2 (MODIFY_EXPR, TREE_TYPE (val),
DECL_RESULT (delete_dtor), val);
add_stmt (build_stmt (0, RETURN_EXPR, val));
@@ -3880,9 +3880,17 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt,
bool maybe, bool equality /* = false */)
{
/* Avoid printing the unhelpful "<unknown>" in the diagnostics. */
- if (ref && TREE_CODE (ref) == SSA_NAME
- && (!SSA_NAME_VAR (ref) || DECL_ARTIFICIAL (SSA_NAME_VAR (ref))))
- ref = NULL_TREE;
+ if (ref && TREE_CODE (ref) == SSA_NAME)
+ {
+ tree var = SSA_NAME_VAR (ref);
+ if (!var)
+ ref = NULL_TREE;
+ /* Don't warn for cases like when a cdtor returns 'this' on ARM. */
+ else if (warning_suppressed_p (var, OPT_Wuse_after_free))
+ return;
+ else if (DECL_ARTIFICIAL (var))
+ ref = NULL_TREE;
+ }
location_t use_loc = gimple_location (use_stmt);
if (use_loc == UNKNOWN_LOCATION)
new file mode 100644
@@ -0,0 +1,10 @@
+// PR target/104213
+// { dg-do compile }
+// { dg-options "-Wuse-after-free" }
+
+class C
+{
+ virtual ~C();
+};
+
+C::~C() {} // { dg-bogus "used after" }
new file mode 100644
@@ -0,0 +1,16 @@
+// PR target/104213
+// { dg-do compile }
+// { dg-options "-Wuse-after-free" }
+// FIXME: We should not output the warning twice.
+
+struct A
+{
+ virtual ~A ();
+ void f ();
+};
+
+A::~A ()
+{
+ operator delete (this);
+ f (); // { dg-warning "used after" }
+} // { dg-warning "used after" }