On Tue, Jan 25, 2022 at 05:35:20PM -0700, Martin Sebor wrote:
> On 1/25/22 16:33, Marek Polacek via Gcc-patches 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?
>
> Thanks for picking this up!
>
> My only concern with suppressing the warning for the pointer rather
> than just for the return statement is that it not lead to false
> negatives. I played with the patch a little, trying to trigger one
> but couldn't so it might be unfounded. Still, I'd expect targeting
> the statement to be a simpler change (the warning already checks
> for the suppression bit on the statement). Did you try it?
I did -- that is the "In my first attempt..." paragraph in my patch
(we don't use copy_warning to propagate the suppress_warning bits).
Maybe it'd be a more correct way to approach this, that is, somehow
tweak gimplify_modify_expr to copy_warning for this special case.
> For reference, one of the test cases I tried is below. It would
> be good to add it to the test suite to make sure the bug is caught
> (there's a duplicate warning there that should at some point get
> squashed).
>
> struct A
> {
> virtual ~A ();
> void f ();
> };
>
> A::~A ()
> {
> operator delete (this);
> f ();
> }
Sure, happy to add it.
> Martin
>
> >
> > 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.
> > ---
> > 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 ++++++++++
> > 4 files changed, 24 insertions(+), 3 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/warn/Wuse-after-free2.C
> >
> > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> > index 22d3dd1e2ad..6534a7fd320 100644
> > --- a/gcc/cp/decl.cc
> > +++ b/gcc/cp/decl.cc
> > @@ -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. */
> > diff --git a/gcc/cp/optimize.cc b/gcc/cp/optimize.cc
> > index 4ad3f1dc9aa..13ab8b7361e 100644
> > --- a/gcc/cp/optimize.cc
> > +++ b/gcc/cp/optimize.cc
> > @@ -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));
> > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> > index 8bc33eeb6fa..484bcd34c25 100644
> > --- a/gcc/gimple-ssa-warn-access.cc
> > +++ b/gcc/gimple-ssa-warn-access.cc
> > @@ -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)
> > diff --git a/gcc/testsuite/g++.dg/warn/Wuse-after-free2.C
> > b/gcc/testsuite/g++.dg/warn/Wuse-after-free2.C
> > new file mode 100644
> > index 00000000000..6d5f2bf01b5
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/warn/Wuse-after-free2.C
> > @@ -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" }
> >
> > base-commit: aeac414923aa1e87986c7fc6f9b921d89a9b86cf
>
Marek
