[PATCH] call maybe_return_this in build_clone

[PATCH] call maybe_return_this in build_clone

[Alexandre Oliva]

__dt_base doesn't get its body from a maybe_return_this caller, it's
rather cloned with the full body within build_clone, and then it's
left alone, without going through finish_function_body or
build_delete_destructor_body, that call maybe_return_this.

Now, this is correct as far as the generated code is concerned, since
the cloned body of a cdtor that returns this is also a cdtor body that
returns this.  The problem is that the decl for THIS is also cloned,
and it doesn't get the warning suppression introduced by
maybe_return_this, so Wuse-after-free3.C fails with an excess warning
at the closing brace of the dtor body.

I've split out the warning suppression from maybe_return_this, and
arranged to call that bit from the relevant build_clone case.
Unfortunately, because the warning is silenced for all uses of the
THIS decl, rather than only for the ABI-mandated return stmt, this
also silences the very warning that the testcase checks for.

I'm not revamping the warning suppression approach to overcome this,
so I'm xfailing the expected warning on ARM EABI, hoping that's the
only target with cdtor_return_this, and leaving it at that.

Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
cpu on trunk, and with tms570 on gcc-13.  Ok to install?


for  gcc/cp/ChangeLog

	* decl.cc (maybe_prepare_return_this): Split out of...
	(maybe_return_this): ... this.
	* cp-tree.h (maybe_prepare_return_this): Declare.
	* class.cc (build_clone): Call it.

for  gcc/testsuite/ChangeLog

	* g++.dg/warn/Wuse-after-free3.C: xfail on arm_eabi.
---
 gcc/cp/class.cc                              |    2 ++
 gcc/cp/cp-tree.h                             |    1 +
 gcc/cp/decl.cc                               |   23 +++++++++++++++++++----
 gcc/testsuite/g++.dg/warn/Wuse-after-free3.C |    4 +++-
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 9d4d95f85bf75..1abf5d90edcef 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -5053,6 +5053,8 @@ build_clone (tree fn, tree name, bool need_vtt_parm_p,
       clone = copy_fndecl_with_name (fn, name, ERROR_MARK,
 				     need_vtt_parm_p, omit_inherited_parms_p);
       DECL_CLONED_FUNCTION (clone) = fn;
+
+      maybe_prepare_return_this (clone);
     }
 
   /* Remember where this function came from.  */
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 1fa710d71543d..e1abb2a79dd81 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6956,6 +6956,7 @@ extern tree lookup_enumerator			(tree, tree);
 extern bool start_preparsed_function		(tree, tree, int);
 extern bool start_function			(cp_decl_specifier_seq *,
 						 const cp_declarator *, tree);
+extern tree maybe_prepare_return_this		(tree);
 extern void maybe_return_this			(void);
 extern tree begin_function_body			(void);
 extern void finish_function_body		(tree);
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index d2ed46b145318..038c5ab71f201 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -17896,16 +17896,31 @@ store_parm_decls (tree current_function_parms)
 }
 
 \f
+/* Mark CDTOR's implicit THIS argument for returning, if required by
+   the ABI..  Return the decl for THIS, if it is to be returned, and
+   NULL otherwise.  */
+
+tree
+maybe_prepare_return_this (tree cdtor)
+{
+  if (targetm.cxx.cdtor_returns_this ())
+    if (tree val = DECL_ARGUMENTS (cdtor))
+      {
+	suppress_warning (val, OPT_Wuse_after_free);
+	return val;
+      }
+
+  return NULL_TREE;
+}
+
 /* Set the return value of the [cd]tor if the ABI wants that.  */
 
 void
-maybe_return_this (void)
+maybe_return_this ()
 {
-  if (targetm.cxx.cdtor_returns_this ())
+  if (tree val = maybe_prepare_return_this (current_function_decl))
     {
       /* Return the address of the object.  */
-      tree val = DECL_ARGUMENTS (current_function_decl);
-      suppress_warning (val, OPT_Wuse_after_free);
       val = fold_convert (TREE_TYPE (DECL_RESULT (current_function_decl)), val);
       val = build2 (MODIFY_EXPR, TREE_TYPE (val),
 		    DECL_RESULT (current_function_decl), val);
diff --git a/gcc/testsuite/g++.dg/warn/Wuse-after-free3.C b/gcc/testsuite/g++.dg/warn/Wuse-after-free3.C
index e5b157865bf6e..8ef82021380a3 100644
--- a/gcc/testsuite/g++.dg/warn/Wuse-after-free3.C
+++ b/gcc/testsuite/g++.dg/warn/Wuse-after-free3.C
@@ -11,5 +11,7 @@ struct A
 A::~A ()
 {
   operator delete (this);
-  f (); // { dg-warning "used after" }
+  f (); // { dg-warning "used after" "" { xfail arm_eabi } }
+  // arm_eabi's cdtors return this, which disables -Wuse-after-free
+  // warnings for cdtors' "this".
 }

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] call maybe_return_this in build_clone

[Jason Merrill]

On 11/19/23 02:38, Alexandre Oliva wrote:
> 
> __dt_base doesn't get its body from a maybe_return_this caller, it's
> rather cloned with the full body within build_clone, and then it's
> left alone, without going through finish_function_body or
> build_delete_destructor_body, that call maybe_return_this.
> 
> Now, this is correct as far as the generated code is concerned, since
> the cloned body of a cdtor that returns this is also a cdtor body that
> returns this.  The problem is that the decl for THIS is also cloned,
> and it doesn't get the warning suppression introduced by
> maybe_return_this, so Wuse-after-free3.C fails with an excess warning
> at the closing brace of the dtor body.
> 
> I've split out the warning suppression from maybe_return_this, and
> arranged to call that bit from the relevant build_clone case.
> Unfortunately, because the warning is silenced for all uses of the
> THIS decl, rather than only for the ABI-mandated return stmt, this
> also silences the very warning that the testcase checks for.
> 
> I'm not revamping the warning suppression approach to overcome this,
> so I'm xfailing the expected warning on ARM EABI, hoping that's the
> only target with cdtor_return_this, and leaving it at that.

So it only passed on that platform before because of the bug?

> Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
> cpu on trunk, and with tms570 on gcc-13.  Ok to install?

OK.

Jason

Re: [PATCH] call maybe_return_this in build_clone

[Alexandre Oliva]

On Nov 20, 2023, Jason Merrill <jason@redhat.com> wrote:

> So it only passed on that platform before because of the bug?

I'm not sure it passed (we've had patches for that testcase before), but
I didn't look closely enough into their history to tell.  I suspected
the warning suppression machinery changed, or details on cloning did, or
something.  It's been fragile historically.  But yeah, recently, the
test for the warning was only passing because of the bug.  But we were
also getting excess warnings, so it wasn't fully passing.


Thanks for the reviews!

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] call maybe_return_this in build_clone

[Torbjorn SVENSSON]

Hi,

Is it okay to backport 0d24289d129639efdc79338a64188d6d404375e8 to 
releases/gcc-13?

Without this backport, I see these failures on arm-none-eabi:

FAIL: g++.dg/warn/Wuse-after-free3.C  -std=gnu++14 (test for excess errors)
FAIL: g++.dg/warn/Wuse-after-free3.C  -std=gnu++17 (test for excess errors)
FAIL: g++.dg/warn/Wuse-after-free3.C  -std=gnu++20 (test for excess errors)

Kind regards,
Torbjörn

On 2023-11-22 09:26, Alexandre Oliva wrote:
> On Nov 20, 2023, Jason Merrill <jason@redhat.com> wrote:
> 
>> So it only passed on that platform before because of the bug?
> 
> I'm not sure it passed (we've had patches for that testcase before), but
> I didn't look closely enough into their history to tell.  I suspected
> the warning suppression machinery changed, or details on cloning did, or
> something.  It's been fragile historically.  But yeah, recently, the
> test for the warning was only passing because of the bug.  But we were
> also getting excess warnings, so it wasn't fully passing.
> 
> 
> Thanks for the reviews!
> 

Re: [PATCH] call maybe_return_this in build_clone

[Jason Merrill]

On 2/7/24 11:24, Torbjorn SVENSSON wrote:
> Hi,
> 
> Is it okay to backport 0d24289d129639efdc79338a64188d6d404375e8 to 
> releases/gcc-13?

OK.

> Without this backport, I see these failures on arm-none-eabi:
> 
> FAIL: g++.dg/warn/Wuse-after-free3.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/warn/Wuse-after-free3.C  -std=gnu++17 (test for excess errors)
> FAIL: g++.dg/warn/Wuse-after-free3.C  -std=gnu++20 (test for excess errors)
> 
> Kind regards,
> Torbjörn
> 
> On 2023-11-22 09:26, Alexandre Oliva wrote:
>> On Nov 20, 2023, Jason Merrill <jason@redhat.com> wrote:
>>
>>> So it only passed on that platform before because of the bug?
>>
>> I'm not sure it passed (we've had patches for that testcase before), but
>> I didn't look closely enough into their history to tell.  I suspected
>> the warning suppression machinery changed, or details on cloning did, or
>> something.  It's been fragile historically.  But yeah, recently, the
>> test for the warning was only passing because of the bug.  But we were
>> also getting excess warnings, so it wasn't fully passing.
>>
>>
>> Thanks for the reviews!
>>
> 

Re: [PATCH] call maybe_return_this in build_clone

[Torbjorn SVENSSON]

On 2024-02-09 19:57, Jason Merrill wrote:
> On 2/7/24 11:24, Torbjorn SVENSSON wrote:
>> Hi,
>>
>> Is it okay to backport 0d24289d129639efdc79338a64188d6d404375e8 to 
>> releases/gcc-13?
> 
> OK.

Pushed as 583bd84075d23cde5fd489ab726093060870d773.