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

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


base-commit: d3cf980217ce13b1eda01d4c42a7e3afd2bf3217

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
>

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