[RFA (gimplify) PATCH] PR c++/33799 - destroy return value if local cleanup throws.

[RFA (gimplify) PATCH] PR c++/33799 - destroy return value if local cleanup throws.

[Jason Merrill]

This is a pretty rare situation since the C++11 change to make all
destructors default to noexcept, but it is still possible to define throwing
destructors, and if a destructor for a local variable throws during the
return, we've already constructed the return value, so now we need to
destroy it.  I handled this somewhat like the new-expression cleanup; as in
that case, this cleanup can't properly nest with the cleanups for local
variables, so I introduce a cleanup region around the whole function and a
flag variable to indicate whether the return value actually needs to be
destroyed.

Setting the flag requires giving a COMPOUND_EXPR as the operand of a
RETURN_EXPR.  Simply allowing that in gimplify_return_expr makes the most
sense to me, is it OK for trunk?

This doesn't currently work with deduced return type because we don't know
the type when we're deciding whether to introduce the cleanup region.

Tested x86_64-pc-linux-gnu.

gcc/
	* gimplify.c (gimplify_return_expr): Handle COMPOUND_EXPR.
gcc/cp/
	* cp-tree.h (current_retval_sentinel): New macro.
	* decl.c (start_preparsed_function): Set up cleanup for retval.
	* typeck.c (check_return_expr): Set current_retval_sentinel.
---
 gcc/cp/cp-tree.h                  |  7 ++++++
 gcc/cp/decl.c                     | 14 +++++++++++
 gcc/cp/typeck.c                   |  9 +++++++
 gcc/gimplify.c                    |  8 +++++++
 gcc/testsuite/g++.dg/eh/return1.C | 40 +++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/eh/return1.C

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 98572bdbad1..c0f780df685 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -1952,6 +1952,13 @@ struct GTY(()) language_function {
 
 #define current_vtt_parm cp_function_chain->x_vtt_parm
 
+/* A boolean flag to control whether we need to clean up the return value if a
+   local destructor throws.  Only used in functions that return by value a
+   class with a destructor.  Which 'tors don't, so we can use the same
+   field as current_vtt_parm.  */
+
+#define current_retval_sentinel current_vtt_parm
+
 /* Set to 0 at beginning of a function definition, set to 1 if
    a return statement that specifies a return value is seen.  */
 
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 094e32edf58..52da0deef40 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -16418,6 +16418,20 @@ start_preparsed_function (tree decl1, tree attrs, int flags)
   if (!DECL_OMP_DECLARE_REDUCTION_P (decl1))
     start_lambda_scope (decl1);
 
+  /* If cleaning up locals on return throws an exception, we need to destroy
+     the return value that we just constructed.  */
+  if (!processing_template_decl
+      && TYPE_HAS_NONTRIVIAL_DESTRUCTOR (TREE_TYPE (TREE_TYPE (decl1))))
+    {
+      tree retval = DECL_RESULT (decl1);
+      tree dtor = build_cleanup (retval);
+      current_retval_sentinel = get_temp_regvar (boolean_type_node,
+						 boolean_false_node);
+      dtor = build3 (COND_EXPR, void_type_node, current_retval_sentinel,
+		     dtor, void_node);
+      push_cleanup (retval, dtor, /*eh-only*/true);
+    }
+
   return true;
 }
 
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 7b653cebca0..8288b662ec0 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -10088,6 +10088,15 @@ check_return_expr (tree retval, bool *no_warning)
   if (retval && retval != result)
     retval = build2 (INIT_EXPR, TREE_TYPE (result), result, retval);
 
+  if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (valtype)
+      /* FIXME doesn't work with deduced return type.  */
+      && current_retval_sentinel)
+    {
+      tree set = build2 (MODIFY_EXPR, boolean_type_node,
+			 current_retval_sentinel, boolean_true_node);
+      retval = build2 (COMPOUND_EXPR, void_type_node, retval, set);
+    }
+
   return retval;
 }
 
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index fe7236de4c3..05d7922116b 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1599,6 +1599,14 @@ gimplify_return_expr (tree stmt, gimple_seq *pre_p)
 
   if (VOID_TYPE_P (TREE_TYPE (TREE_TYPE (current_function_decl))))
     result_decl = NULL_TREE;
+  else if (TREE_CODE (ret_expr) == COMPOUND_EXPR)
+    {
+      /* Used in C++ for handling EH cleanup of the return value if a local
+	 cleanup throws.  Assume the front-end knows what it's doing.  */
+      result_decl = DECL_RESULT (current_function_decl);
+      /* But crash if we end up trying to modify ret_expr below.  */
+      ret_expr = NULL_TREE;
+    }
   else
     {
       result_decl = TREE_OPERAND (ret_expr, 0);
diff --git a/gcc/testsuite/g++.dg/eh/return1.C b/gcc/testsuite/g++.dg/eh/return1.C
new file mode 100644
index 00000000000..7469d3128dc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/eh/return1.C
@@ -0,0 +1,40 @@
+// PR c++/33799
+// { dg-do run }
+
+extern "C" void abort();
+
+int c, d;
+
+#if __cplusplus >= 201103L
+#define THROWS noexcept(false)
+#else
+#define THROWS
+#endif
+
+struct X
+{
+  X(bool throws) : throws_(throws) { ++c; }
+  X(const X& x) : throws_(x.throws_) { ++c; }
+  ~X() THROWS
+  {
+    ++d;
+    if (throws_) { throw 1; }
+  }
+private:
+  bool throws_;
+};
+
+X f()
+{
+  X x(true);
+  return X(false);
+}
+
+int main()
+{
+  try { f(); }
+  catch (...) {}
+
+  if (c != d)
+    throw;
+}

base-commit: d3cf980217ce13b1eda01d4c42a7e3afd2bf3217
-- 
2.18.1

Re: [RFA (gimplify) PATCH] PR c++/33799 - destroy return value if local cleanup throws.

[Richard Biener]

On Sat, Jan 11, 2020 at 6:13 AM Jason Merrill <jason@redhat.com> wrote:
>
> This is a pretty rare situation since the C++11 change to make all
> destructors default to noexcept, but it is still possible to define throwing
> destructors, and if a destructor for a local variable throws during the
> return, we've already constructed the return value, so now we need to
> destroy it.  I handled this somewhat like the new-expression cleanup; as in
> that case, this cleanup can't properly nest with the cleanups for local
> variables, so I introduce a cleanup region around the whole function and a
> flag variable to indicate whether the return value actually needs to be
> destroyed.
>
> Setting the flag requires giving a COMPOUND_EXPR as the operand of a
> RETURN_EXPR.  Simply allowing that in gimplify_return_expr makes the most
> sense to me, is it OK for trunk?

It works for me.

Richard.

> This doesn't currently work with deduced return type because we don't know
> the type when we're deciding whether to introduce the cleanup region.
>
> Tested x86_64-pc-linux-gnu.
>
> gcc/
>         * gimplify.c (gimplify_return_expr): Handle COMPOUND_EXPR.
> gcc/cp/
>         * cp-tree.h (current_retval_sentinel): New macro.
>         * decl.c (start_preparsed_function): Set up cleanup for retval.
>         * typeck.c (check_return_expr): Set current_retval_sentinel.
> ---
>  gcc/cp/cp-tree.h                  |  7 ++++++
>  gcc/cp/decl.c                     | 14 +++++++++++
>  gcc/cp/typeck.c                   |  9 +++++++
>  gcc/gimplify.c                    |  8 +++++++
>  gcc/testsuite/g++.dg/eh/return1.C | 40 +++++++++++++++++++++++++++++++
>  5 files changed, 78 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/eh/return1.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 98572bdbad1..c0f780df685 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -1952,6 +1952,13 @@ struct GTY(()) language_function {
>
>  #define current_vtt_parm cp_function_chain->x_vtt_parm
>
> +/* A boolean flag to control whether we need to clean up the return value if a
> +   local destructor throws.  Only used in functions that return by value a
> +   class with a destructor.  Which 'tors don't, so we can use the same
> +   field as current_vtt_parm.  */
> +
> +#define current_retval_sentinel current_vtt_parm
> +
>  /* Set to 0 at beginning of a function definition, set to 1 if
>     a return statement that specifies a return value is seen.  */
>
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 094e32edf58..52da0deef40 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -16418,6 +16418,20 @@ start_preparsed_function (tree decl1, tree attrs, int flags)
>    if (!DECL_OMP_DECLARE_REDUCTION_P (decl1))
>      start_lambda_scope (decl1);
>
> +  /* If cleaning up locals on return throws an exception, we need to destroy
> +     the return value that we just constructed.  */
> +  if (!processing_template_decl
> +      && TYPE_HAS_NONTRIVIAL_DESTRUCTOR (TREE_TYPE (TREE_TYPE (decl1))))
> +    {
> +      tree retval = DECL_RESULT (decl1);
> +      tree dtor = build_cleanup (retval);
> +      current_retval_sentinel = get_temp_regvar (boolean_type_node,
> +                                                boolean_false_node);
> +      dtor = build3 (COND_EXPR, void_type_node, current_retval_sentinel,
> +                    dtor, void_node);
> +      push_cleanup (retval, dtor, /*eh-only*/true);
> +    }
> +
>    return true;
>  }
>
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index 7b653cebca0..8288b662ec0 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -10088,6 +10088,15 @@ check_return_expr (tree retval, bool *no_warning)
>    if (retval && retval != result)
>      retval = build2 (INIT_EXPR, TREE_TYPE (result), result, retval);
>
> +  if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (valtype)
> +      /* FIXME doesn't work with deduced return type.  */
> +      && current_retval_sentinel)
> +    {
> +      tree set = build2 (MODIFY_EXPR, boolean_type_node,
> +                        current_retval_sentinel, boolean_true_node);
> +      retval = build2 (COMPOUND_EXPR, void_type_node, retval, set);
> +    }
> +
>    return retval;
>  }
>
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index fe7236de4c3..05d7922116b 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1599,6 +1599,14 @@ gimplify_return_expr (tree stmt, gimple_seq *pre_p)
>
>    if (VOID_TYPE_P (TREE_TYPE (TREE_TYPE (current_function_decl))))
>      result_decl = NULL_TREE;
> +  else if (TREE_CODE (ret_expr) == COMPOUND_EXPR)
> +    {
> +      /* Used in C++ for handling EH cleanup of the return value if a local
> +        cleanup throws.  Assume the front-end knows what it's doing.  */
> +      result_decl = DECL_RESULT (current_function_decl);
> +      /* But crash if we end up trying to modify ret_expr below.  */
> +      ret_expr = NULL_TREE;
> +    }
>    else
>      {
>        result_decl = TREE_OPERAND (ret_expr, 0);
> diff --git a/gcc/testsuite/g++.dg/eh/return1.C b/gcc/testsuite/g++.dg/eh/return1.C
> new file mode 100644
> index 00000000000..7469d3128dc
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/eh/return1.C
> @@ -0,0 +1,40 @@
> +// PR c++/33799
> +// { dg-do run }
> +
> +extern "C" void abort();
> +
> +int c, d;
> +
> +#if __cplusplus >= 201103L
> +#define THROWS noexcept(false)
> +#else
> +#define THROWS
> +#endif
> +
> +struct X
> +{
> +  X(bool throws) : throws_(throws) { ++c; }
> +  X(const X& x) : throws_(x.throws_) { ++c; }
> +  ~X() THROWS
> +  {
> +    ++d;
> +    if (throws_) { throw 1; }
> +  }
> +private:
> +  bool throws_;
> +};
> +
> +X f()
> +{
> +  X x(true);
> +  return X(false);
> +}
> +
> +int main()
> +{
> +  try { f(); }
> +  catch (...) {}
> +
> +  if (c != d)
> +    throw;
> +}
>
> base-commit: d3cf980217ce13b1eda01d4c42a7e3afd2bf3217
> --
> 2.18.1
>

Re: [RFA (gimplify) PATCH] PR c++/33799 - destroy return value if local cleanup throws.

[Jason Merrill]

On 1/11/20 12:12 AM, Jason Merrill wrote:
> This is a pretty rare situation since the C++11 change to make all
> destructors default to noexcept, but it is still possible to define throwing
> destructors, and if a destructor for a local variable throws during the
> return, we've already constructed the return value, so now we need to
> destroy it.  I handled this somewhat like the new-expression cleanup; as in
> that case, this cleanup can't properly nest with the cleanups for local
> variables, so I introduce a cleanup region around the whole function and a
> flag variable to indicate whether the return value actually needs to be
> destroyed.

This patch was interfering with the coroutines merge, so I've reverted 
it for the moment and will look at adjusting my approach.

Jason

[C++ PATCH] PR c++/33799 - destroy return value, take 2.

[Jason Merrill]

This patch differs from the reverted patch for 33799 in that it adds the
CLEANUP_STMT for the return value at the end of the function, and only if
we've seen a cleanup that might throw, so it should not affect most C++11
code.

Tested x86_64-pc-linux-gnu, applying to trunk.

	* cp-tree.h (current_retval_sentinel): New macro.
	(struct language_function): Add throwing_cleanup bitfield.
	* decl.c (cxx_maybe_build_cleanup): Set it.
	* except.c (maybe_set_retval_sentinel)
	(maybe_splice_retval_cleanup): New functions.
	* parser.c (cp_parser_compound_statement): Call
	maybe_splice_retval_cleanup.
	* typeck.c (check_return_expr): Call maybe_set_retval_sentinel.
---
 gcc/cp/cp-tree.h                  | 11 +++++
 gcc/cp/decl.c                     |  3 ++
 gcc/cp/except.c                   | 72 +++++++++++++++++++++++++++++++
 gcc/cp/parser.c                   |  4 ++
 gcc/cp/typeck.c                   |  3 ++
 gcc/testsuite/g++.dg/eh/return1.C | 20 ++++++++-
 6 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 3d76096b041..890d5a27350 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -1904,6 +1904,7 @@ struct GTY(()) language_function {
   BOOL_BITFIELD can_throw : 1;
 
   BOOL_BITFIELD invalid_constexpr : 1;
+  BOOL_BITFIELD throwing_cleanup : 1;
 
   hash_table<named_label_hash> *x_named_labels;
 
@@ -1954,6 +1955,13 @@ struct GTY(()) language_function {
 
 #define current_vtt_parm cp_function_chain->x_vtt_parm
 
+/* A boolean flag to control whether we need to clean up the return value if a
+   local destructor throws.  Only used in functions that return by value a
+   class with a destructor.  Which 'tors don't, so we can use the same
+   field as current_vtt_parm.  */
+
+#define current_retval_sentinel current_vtt_parm
+
 /* Set to 0 at beginning of a function definition, set to 1 if
    a return statement that specifies a return value is seen.  */
 
@@ -6686,6 +6694,9 @@ extern tree begin_eh_spec_block			(void);
 extern void finish_eh_spec_block		(tree, tree);
 extern tree build_eh_type_type			(tree);
 extern tree cp_protect_cleanup_actions		(void);
+extern void maybe_splice_retval_cleanup		(tree);
+extern tree maybe_set_retval_sentinel		(void);
+
 extern tree template_parms_to_args		(tree);
 extern tree template_parms_level_to_args	(tree);
 extern tree generic_targs_for			(tree);
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index e58fecc9de7..28a79029d92 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -17402,6 +17402,9 @@ cxx_maybe_build_cleanup (tree decl, tsubst_flags_t complain)
       && !mark_used (decl, complain) && !(complain & tf_error))
     return error_mark_node;
 
+  if (cleanup && cfun && !expr_noexcept_p (cleanup, tf_none))
+    cp_function_chain->throwing_cleanup = true;
+
   return cleanup;
 }
 
diff --git a/gcc/cp/except.c b/gcc/cp/except.c
index 55b4b6af442..0b40234e228 100644
--- a/gcc/cp/except.c
+++ b/gcc/cp/except.c
@@ -1325,4 +1325,76 @@ build_noexcept_spec (tree expr, tsubst_flags_t complain)
     }
 }
 
+/* If the current function has a cleanup that might throw, and the return value
+   has a non-trivial destructor, return a MODIFY_EXPR to set
+   current_retval_sentinel so that we know that the return value needs to be
+   destroyed on throw.  Otherwise, returns NULL_TREE.  */
+
+tree
+maybe_set_retval_sentinel ()
+{
+  if (processing_template_decl)
+    return NULL_TREE;
+  tree retval = DECL_RESULT (current_function_decl);
+  if (!TYPE_HAS_NONTRIVIAL_DESTRUCTOR (TREE_TYPE (retval)))
+    return NULL_TREE;
+  if (!cp_function_chain->throwing_cleanup)
+    return NULL_TREE;
+
+  if (!current_retval_sentinel)
+    {
+      /* Just create the temporary now, maybe_splice_retval_cleanup
+	 will do the rest.  */
+      current_retval_sentinel = create_temporary_var (boolean_type_node);
+      DECL_INITIAL (current_retval_sentinel) = boolean_false_node;
+      pushdecl_outermost_localscope (current_retval_sentinel);
+    }
+
+  return build2 (MODIFY_EXPR, boolean_type_node,
+		 current_retval_sentinel, boolean_true_node);
+}
+
+/* COMPOUND_STMT is the STATEMENT_LIST for the current function body.  If
+   current_retval_sentinel was set in this function, wrap the body in a
+   CLEANUP_STMT to destroy the return value on throw.  */
+
+void
+maybe_splice_retval_cleanup (tree compound_stmt)
+{
+  /* If need_retval_cleanup set current_retval_sentinel, wrap the function body
+     in a CLEANUP_STMT to handle destroying the return value.  */
+  if (!DECL_CONSTRUCTOR_P (current_function_decl)
+      && !DECL_DESTRUCTOR_P (current_function_decl)
+      && current_retval_sentinel)
+    {
+      location_t loc = DECL_SOURCE_LOCATION (current_function_decl);
+
+      /* Add a DECL_EXPR for current_retval_sentinel.  */
+      tree_stmt_iterator iter = tsi_start (compound_stmt);
+      tree retval = DECL_RESULT (current_function_decl);
+      tree decl_expr = build_stmt (loc, DECL_EXPR, current_retval_sentinel);
+      tsi_link_before (&iter, decl_expr, TSI_SAME_STMT);
+
+      /* Skip past other decls, they can't contain a return.  */
+      while (TREE_CODE (tsi_stmt (iter)) == DECL_EXPR)
+	tsi_next (&iter);
+      gcc_assert (!tsi_end_p (iter));
+
+      /* Wrap the rest of the STATEMENT_LIST in a CLEANUP_STMT.  */
+      tree stmts = NULL_TREE;
+      while (!tsi_end_p (iter))
+	{
+	  append_to_statement_list_force (tsi_stmt (iter), &stmts);
+	  tsi_delink (&iter);
+	}
+      tree dtor = build_cleanup (retval);
+      tree cond = build3 (COND_EXPR, void_type_node, current_retval_sentinel,
+			  dtor, void_node);
+      tree cleanup = build_stmt (loc, CLEANUP_STMT,
+				 stmts, cond, retval);
+      CLEANUP_EH_ONLY (cleanup) = true;
+      append_to_statement_list_force (cleanup, &compound_stmt);
+    }
+}
+
 #include "gt-cp-except.h"
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 98c1beb400f..caafbefda8e 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -11716,6 +11716,10 @@ cp_parser_compound_statement (cp_parser *parser, tree in_statement_expr,
     cp_parser_label_declaration (parser);
   /* Parse an (optional) statement-seq.  */
   cp_parser_statement_seq_opt (parser, in_statement_expr);
+
+  if (function_body)
+    maybe_splice_retval_cleanup (compound_stmt);
+
   /* Finish the compound-statement.  */
   finish_compound_stmt (compound_stmt);
   /* Consume the `}'.  */
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 8955442349f..5964c34272e 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -10090,6 +10090,9 @@ check_return_expr (tree retval, bool *no_warning)
   if (retval && retval != result)
     retval = build2 (INIT_EXPR, TREE_TYPE (result), result, retval);
 
+  if (tree set = maybe_set_retval_sentinel ())
+    retval = build2 (COMPOUND_EXPR, void_type_node, retval, set);
+
   return retval;
 }
 
diff --git a/gcc/testsuite/g++.dg/eh/return1.C b/gcc/testsuite/g++.dg/eh/return1.C
index ca0b8046e1b..5ef2f1dee85 100644
--- a/gcc/testsuite/g++.dg/eh/return1.C
+++ b/gcc/testsuite/g++.dg/eh/return1.C
@@ -1,5 +1,5 @@
 // PR c++/33799
-// { dg-do run { xfail *-*-* } }
+// { dg-do run }
 
 extern "C" void abort();
 
@@ -30,11 +30,29 @@ X f()
   return X(false);
 }
 
+X g()
+{
+  return X(true),X(false);
+}
+
+void h()
+{
+#if __cplusplus >= 201103L
+  []{ return X(true),X(false); }();
+#endif
+}
+
 int main()
 {
   try { f(); }
   catch (...) {}
 
+  try { g(); }
+  catch (...) {}
+
+  try { h(); }
+  catch (...) {}
+
   if (c != d)
     throw;
 }

base-commit: 52354dadb80b60c3fd05fb1b5aa3feb15a98b8af
-- 
2.18.1