Re: [PATCH v2] c++: make build_throw SFINAE-friendly [PR98388]

[Jason Merrill <jason@redhat.com>]

On 2/8/24 11:51, Marek Polacek wrote:
> On Thu, Feb 08, 2024 at 08:49:28AM -0500, Patrick Palka wrote:
>> On Wed, 7 Feb 2024, Marek Polacek wrote:
>>
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>
>>> -- >8 --
>>> Here the problem is that we give hard errors while substituting
>>> template parameters during overload resolution of is_throwable
>>> which has an invalid throw in decltype.
>>>
>>> The backtrace shows that fn_type_unification -> instantiate_template
>>> -> tsubst* passes complain=0 as expected, but build_throw doesn't
>>> have a complain parameter.  So let's add one.  Also remove a redundant
>>> local variable which I should have removed in my P2266 patch.
>>>
>>> But there's still something not quite clear to me.  I claim that 'b'
>>> in the testcase should evaluate to false since the first overload ought
>>> to have been discarded.  EDG 6.6 agrees, but clang++, msvc, and icx evaluate
>>> it to true.  Who's right?

I think it should be true since P1155, which we implement in C++20 mode 
and above (or rather, we implement the sequel P2266); since then we 
implicitly move from the function parameter.

The patch looks good except that we should test this expected value.

>>> Thanks to Patrick for notifying me of this PR.  This doesn't fully fix
>>> 113789; there I think I'll have to figure our why a candidate wasn't
>>> discarded from the overload set.
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* coroutines.cc (coro_rewrite_function_body): Pass tf_warning_or_error
>>> 	to build_throw.
>>> 	(morph_fn_to_coro): Likewise.
>>> 	* cp-tree.h (build_throw): Adjust.
>>> 	* except.cc (expand_end_catch_block): Pass tf_warning_or_error to
>>> 	build_throw.
>>> 	(build_throw): Add a tsubst_flags_t parameter.  Use it.  Remove
>>> 	redundant variable.  Guard an inform call.
>>> 	* parser.cc (cp_parser_throw_expression): Pass tf_warning_or_error
>>> 	to build_throw.
>>> 	* pt.cc (tsubst_expr) <case THROW_EXPR>: Pass complain to build_throw.
>>>
>>> libcc1/ChangeLog:
>>>
>>> 	* libcp1plugin.cc (plugin_build_unary_expr): Pass tf_error to
>>> 	build_throw.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/cpp0x/sfinae69.C: New test.
>>> ---
>>>   gcc/cp/coroutines.cc                  |  4 ++--
>>>   gcc/cp/cp-tree.h                      |  3 ++-
>>>   gcc/cp/except.cc                      | 31 +++++++++++----------------
>>>   gcc/cp/parser.cc                      |  2 +-
>>>   gcc/cp/pt.cc                          |  2 +-
>>>   gcc/testsuite/g++.dg/cpp0x/sfinae69.C | 16 ++++++++++++++
>>>   libcc1/libcp1plugin.cc                |  4 ++--
>>>   7 files changed, 37 insertions(+), 25 deletions(-)
>>>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/sfinae69.C
>>>
>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>> index 3194c911e8c..9b037edbd14 100644
>>> --- a/gcc/cp/coroutines.cc
>>> +++ b/gcc/cp/coroutines.cc
>>> @@ -4246,7 +4246,7 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig,
>>>   				  boolean_type_node, i_a_r_c);
>>>         finish_if_stmt_cond (not_iarc, not_iarc_if);
>>>         /* If the initial await resume called value is false, rethrow...  */
>>> -      tree rethrow = build_throw (fn_start, NULL_TREE);
>>> +      tree rethrow = build_throw (fn_start, NULL_TREE, tf_warning_or_error);
>>>         suppress_warning (rethrow);
>>>         finish_expr_stmt (rethrow);
>>>         finish_then_clause (not_iarc_if);
>>> @@ -5151,7 +5151,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>>>         tree del_coro_fr = coro_get_frame_dtor (coro_fp, orig, frame_size,
>>>   					      promise_type, fn_start);
>>>         finish_expr_stmt (del_coro_fr);
>>> -      tree rethrow = build_throw (fn_start, NULL_TREE);
>>> +      tree rethrow = build_throw (fn_start, NULL_TREE, tf_warning_or_error);
>>>         suppress_warning (rethrow);
>>>         finish_expr_stmt (rethrow);
>>>         finish_handler (handler);
>>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>>> index 969c7239c97..334c11396c2 100644
>>> --- a/gcc/cp/cp-tree.h
>>> +++ b/gcc/cp/cp-tree.h
>>> @@ -7185,7 +7185,8 @@ extern void init_exception_processing		(void);
>>>   extern tree expand_start_catch_block		(tree);
>>>   extern void expand_end_catch_block		(void);
>>>   extern tree build_exc_ptr			(void);
>>> -extern tree build_throw				(location_t, tree);
>>> +extern tree build_throw				(location_t, tree,
>>> +						 tsubst_flags_t);
>>>   extern int nothrow_libfn_p			(const_tree);
>>>   extern void check_handlers			(tree);
>>>   extern tree finish_noexcept_expr		(tree, tsubst_flags_t);
>>> diff --git a/gcc/cp/except.cc b/gcc/cp/except.cc
>>> index d17a57d3fbc..ed704b6c28a 100644
>>> --- a/gcc/cp/except.cc
>>> +++ b/gcc/cp/except.cc
>>> @@ -486,7 +486,8 @@ expand_end_catch_block (void)
>>>   	  || DECL_DESTRUCTOR_P (current_function_decl))
>>>         && !in_nested_catch ())
>>>       {
>>> -      tree rethrow = build_throw (input_location, NULL_TREE);
>>> +      tree rethrow = build_throw (input_location, NULL_TREE,
>>> +				  tf_warning_or_error);
>>>         /* Disable all warnings for the generated rethrow statement.  */
>>>         suppress_warning (rethrow);
>>>         finish_expr_stmt (rethrow);
>>> @@ -607,7 +608,7 @@ wrap_cleanups_r (tree *tp, int *walk_subtrees, void * /*data*/)
>>>   /* Build a throw expression.  */
>>>   
>>>   tree
>>> -build_throw (location_t loc, tree exp)
>>> +build_throw (location_t loc, tree exp, tsubst_flags_t complain)
>>>   {
>>>     if (exp == error_mark_node)
>>>       return exp;
>>
>> The "throwing NULL, which has integral, not pointer type" warning near
>> the top of build_throw should probably be guarded by tf_warning now.
> 
> Ah, I missed that.  Fixed.
>   
>>> @@ -642,8 +643,6 @@ build_throw (location_t loc, tree exp)
>>>         tree object, ptr;
>>>         tree allocate_expr;
>>>   
>>> -      tsubst_flags_t complain = tf_warning_or_error;
>>> -
>>>         /* The CLEANUP_TYPE is the internal type of a destructor.  */
>>>         if (!cleanup_type)
>>>   	{
>>> @@ -714,7 +713,6 @@ build_throw (location_t loc, tree exp)
>>>         if (CLASS_TYPE_P (temp_type))
>>>   	{
>>>   	  int flags = LOOKUP_NORMAL | LOOKUP_ONLYCONVERTING;
>>> -	  bool converted = false;
>>>   	  location_t exp_loc = cp_expr_loc_or_loc (exp, loc);
>>>   
>>>   	  /* Under C++0x [12.8/16 class.copy], a thrown lvalue is sometimes
>>> @@ -727,23 +725,20 @@ build_throw (location_t loc, tree exp)
>>>   	    exp = moved;
>>>   
>>>   	  /* Call the copy constructor.  */
>>> -	  if (!converted)
>>> -	    {
>>> -	      releasing_vec exp_vec (make_tree_vector_single (exp));
>>> -	      exp = (build_special_member_call
>>> -		     (object, complete_ctor_identifier, &exp_vec,
>>> -		      TREE_TYPE (object), flags, tf_warning_or_error));
>>> -	    }
>>> -
>>> +	  releasing_vec exp_vec (make_tree_vector_single (exp));
>>> +	  exp = build_special_member_call (object, complete_ctor_identifier,
>>> +					   &exp_vec, TREE_TYPE (object), flags,
>>> +					   complain);
>>>   	  if (exp == error_mark_node)
>>>   	    {
>>> -	      inform (exp_loc, "  in thrown expression");
>>> +	      if (complain & tf_warning)
>>
>> tf_error instead of tf_warning for this note?
> 
> Not sure it makes a difference in practice, but I've changed it.  Thanks,
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> -- >8 --
> Here the problem is that we give hard errors while substituting
> template parameters during overload resolution of is_throwable
> which has an invalid throw in decltype.
> 
> The backtrace shows that fn_type_unification -> instantiate_template
> -> tsubst* passes complain=0 as expected, but build_throw doesn't
> have a complain parameter.  So let's add one.  Also remove a redundant
> local variable which I should have removed in my P2266 patch.
> 
> But there's still something not quite clear to me.  I claim that 'b'
> in the testcase should evaluate to false since the first overload ought
> to have been discarded.  EDG 6.6 agrees, but clang++, msvc, and icx evaluate
> it to true.  Who's right?
> 
> Thanks to Patrick for notifying me of this PR.  This doesn't fully fix
> 113789; there I think I'll have to figure our why a candidate wasn't
> discarded from the overload set.
> 
> gcc/cp/ChangeLog:
> 
> 	* coroutines.cc (coro_rewrite_function_body): Pass tf_warning_or_error
> 	to build_throw.
> 	(morph_fn_to_coro): Likewise.
> 	* cp-tree.h (build_throw): Adjust.
> 	* except.cc (expand_end_catch_block): Pass tf_warning_or_error to
> 	build_throw.
> 	(build_throw): Add a tsubst_flags_t parameter.  Use it.  Remove
> 	redundant variable.  Guard an inform call.
> 	* parser.cc (cp_parser_throw_expression): Pass tf_warning_or_error
> 	to build_throw.
> 	* pt.cc (tsubst_expr) <case THROW_EXPR>: Pass complain to build_throw.
> 
> libcc1/ChangeLog:
> 
> 	* libcp1plugin.cc (plugin_build_unary_expr): Pass tf_error to
> 	build_throw.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/sfinae69.C: New test.
> ---
>   gcc/cp/coroutines.cc                  |  4 ++--
>   gcc/cp/cp-tree.h                      |  3 ++-
>   gcc/cp/except.cc                      | 33 ++++++++++++---------------
>   gcc/cp/parser.cc                      |  2 +-
>   gcc/cp/pt.cc                          |  2 +-
>   gcc/testsuite/g++.dg/cpp0x/sfinae69.C | 16 +++++++++++++
>   libcc1/libcp1plugin.cc                |  4 ++--
>   7 files changed, 38 insertions(+), 26 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/sfinae69.C
> 
> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
> index 3194c911e8c..9b037edbd14 100644
> --- a/gcc/cp/coroutines.cc
> +++ b/gcc/cp/coroutines.cc
> @@ -4246,7 +4246,7 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig,
>   				  boolean_type_node, i_a_r_c);
>         finish_if_stmt_cond (not_iarc, not_iarc_if);
>         /* If the initial await resume called value is false, rethrow...  */
> -      tree rethrow = build_throw (fn_start, NULL_TREE);
> +      tree rethrow = build_throw (fn_start, NULL_TREE, tf_warning_or_error);
>         suppress_warning (rethrow);
>         finish_expr_stmt (rethrow);
>         finish_then_clause (not_iarc_if);
> @@ -5151,7 +5151,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
>         tree del_coro_fr = coro_get_frame_dtor (coro_fp, orig, frame_size,
>   					      promise_type, fn_start);
>         finish_expr_stmt (del_coro_fr);
> -      tree rethrow = build_throw (fn_start, NULL_TREE);
> +      tree rethrow = build_throw (fn_start, NULL_TREE, tf_warning_or_error);
>         suppress_warning (rethrow);
>         finish_expr_stmt (rethrow);
>         finish_handler (handler);
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 969c7239c97..334c11396c2 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7185,7 +7185,8 @@ extern void init_exception_processing		(void);
>   extern tree expand_start_catch_block		(tree);
>   extern void expand_end_catch_block		(void);
>   extern tree build_exc_ptr			(void);
> -extern tree build_throw				(location_t, tree);
> +extern tree build_throw				(location_t, tree,
> +						 tsubst_flags_t);
>   extern int nothrow_libfn_p			(const_tree);
>   extern void check_handlers			(tree);
>   extern tree finish_noexcept_expr		(tree, tsubst_flags_t);
> diff --git a/gcc/cp/except.cc b/gcc/cp/except.cc
> index d17a57d3fbc..ea3d6f57396 100644
> --- a/gcc/cp/except.cc
> +++ b/gcc/cp/except.cc
> @@ -486,7 +486,8 @@ expand_end_catch_block (void)
>   	  || DECL_DESTRUCTOR_P (current_function_decl))
>         && !in_nested_catch ())
>       {
> -      tree rethrow = build_throw (input_location, NULL_TREE);
> +      tree rethrow = build_throw (input_location, NULL_TREE,
> +				  tf_warning_or_error);
>         /* Disable all warnings for the generated rethrow statement.  */
>         suppress_warning (rethrow);
>         finish_expr_stmt (rethrow);
> @@ -607,7 +608,7 @@ wrap_cleanups_r (tree *tp, int *walk_subtrees, void * /*data*/)
>   /* Build a throw expression.  */
>   
>   tree
> -build_throw (location_t loc, tree exp)
> +build_throw (location_t loc, tree exp, tsubst_flags_t complain)
>   {
>     if (exp == error_mark_node)
>       return exp;
> @@ -621,7 +622,7 @@ build_throw (location_t loc, tree exp)
>         return exp;
>       }
>   
> -  if (exp && null_node_p (exp))
> +  if (exp && null_node_p (exp) && (complain & tf_warning))
>       warning_at (loc, 0,
>   		"throwing NULL, which has integral, not pointer type");
>   
> @@ -642,8 +643,6 @@ build_throw (location_t loc, tree exp)
>         tree object, ptr;
>         tree allocate_expr;
>   
> -      tsubst_flags_t complain = tf_warning_or_error;
> -
>         /* The CLEANUP_TYPE is the internal type of a destructor.  */
>         if (!cleanup_type)
>   	{
> @@ -714,7 +713,6 @@ build_throw (location_t loc, tree exp)
>         if (CLASS_TYPE_P (temp_type))
>   	{
>   	  int flags = LOOKUP_NORMAL | LOOKUP_ONLYCONVERTING;
> -	  bool converted = false;
>   	  location_t exp_loc = cp_expr_loc_or_loc (exp, loc);
>   
>   	  /* Under C++0x [12.8/16 class.copy], a thrown lvalue is sometimes
> @@ -727,23 +725,20 @@ build_throw (location_t loc, tree exp)
>   	    exp = moved;
>   
>   	  /* Call the copy constructor.  */
> -	  if (!converted)
> -	    {
> -	      releasing_vec exp_vec (make_tree_vector_single (exp));
> -	      exp = (build_special_member_call
> -		     (object, complete_ctor_identifier, &exp_vec,
> -		      TREE_TYPE (object), flags, tf_warning_or_error));
> -	    }
> -
> +	  releasing_vec exp_vec (make_tree_vector_single (exp));
> +	  exp = build_special_member_call (object, complete_ctor_identifier,
> +					   &exp_vec, TREE_TYPE (object), flags,
> +					   complain);
>   	  if (exp == error_mark_node)
>   	    {
> -	      inform (exp_loc, "  in thrown expression");
> +	      if (complain & tf_error)
> +		inform (exp_loc, "  in thrown expression");
>   	      return error_mark_node;
>   	    }
>   	}
>         else
>   	{
> -	  tree tmp = decay_conversion (exp, tf_warning_or_error);
> +	  tree tmp = decay_conversion (exp, complain);
>   	  if (tmp == error_mark_node)
>   	    return error_mark_node;
>   	  exp = cp_build_init_expr (object, tmp);
> @@ -768,7 +763,7 @@ build_throw (location_t loc, tree exp)
>   	  tree binfo = TYPE_BINFO (TREE_TYPE (object));
>   	  tree dtor_fn = lookup_fnfields (binfo,
>   					  complete_dtor_identifier, 0,
> -					  tf_warning_or_error);
> +					  complain);
>   	  dtor_fn = BASELINK_FUNCTIONS (dtor_fn);
>   	  if (!mark_used (dtor_fn)
>   	      || !perform_or_defer_access_check (binfo, dtor_fn,
> @@ -785,7 +780,7 @@ build_throw (location_t loc, tree exp)
>   	cleanup = build_int_cst (cleanup_type, 0);
>   
>         /* ??? Indicate that this function call throws throw_type.  */
> -      tree tmp = cp_build_function_call_nary (throw_fn, tf_warning_or_error,
> +      tree tmp = cp_build_function_call_nary (throw_fn, complain,
>   					      ptr, throw_type, cleanup,
>   					      NULL_TREE);
>   
> @@ -807,7 +802,7 @@ build_throw (location_t loc, tree exp)
>   
>         /* ??? Indicate that this function call allows exceptions of the type
>   	 of the enclosing catch block (if known).  */
> -      exp = cp_build_function_call_vec (rethrow_fn, NULL, tf_warning_or_error);
> +      exp = cp_build_function_call_vec (rethrow_fn, NULL, complain);
>       }
>   
>     exp = build1_loc (loc, THROW_EXPR, void_type_node, exp);
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index c4292c49ce2..09ecfa23b5d 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -29310,7 +29310,7 @@ cp_parser_throw_expression (cp_parser* parser)
>        the end at the end of the final token we consumed.  */
>     location_t combined_loc = make_location (start_loc, start_loc,
>   					   parser->lexer);
> -  expression = build_throw (combined_loc, expression);
> +  expression = build_throw (combined_loc, expression, tf_warning_or_error);
>   
>     return expression;
>   }
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index f9abb21a9a2..9c225c095c8 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -21180,7 +21180,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
>   
>       case THROW_EXPR:
>         RETURN (build_throw
> -       (input_location, RECUR (TREE_OPERAND (t, 0))));
> +       (input_location, RECUR (TREE_OPERAND (t, 0)), complain));
>   
>       case CONSTRUCTOR:
>         {
> diff --git a/gcc/testsuite/g++.dg/cpp0x/sfinae69.C b/gcc/testsuite/g++.dg/cpp0x/sfinae69.C
> new file mode 100644
> index 00000000000..c4896050b9d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/sfinae69.C
> @@ -0,0 +1,16 @@
> +// PR c++/98388
> +// { dg-do compile { target c++11 } }
> +
> +struct moveonly {
> +    moveonly() = default;
> +    moveonly(moveonly&&) = default;
> +};
> +
> +template<class T>
> +constexpr auto is_throwable(T t) -> decltype(throw t, true) {
> +    return true;
> +}
> +template<class T>
> +constexpr bool is_throwable(...) { return false; }
> +
> +constexpr bool b = is_throwable<moveonly>(moveonly{});
> diff --git a/libcc1/libcp1plugin.cc b/libcc1/libcp1plugin.cc
> index 7f4e4c98060..0eff7c68d29 100644
> --- a/libcc1/libcp1plugin.cc
> +++ b/libcc1/libcp1plugin.cc
> @@ -2640,7 +2640,7 @@ plugin_build_unary_expr (cc1_plugin::connection *self,
>         break;
>   
>       case THROW_EXPR:
> -      result = build_throw (input_location, op0);
> +      result = build_throw (input_location, op0, tf_error);
>         break;
>   
>       case TYPEID_EXPR:
> @@ -2664,7 +2664,7 @@ plugin_build_unary_expr (cc1_plugin::connection *self,
>         result = make_pack_expansion (op0);
>         break;
>   
> -      // We're using this for sizeof...(pack).  */
> +      /* We're using this for sizeof...(pack).  */
>       case TYPE_PACK_EXPANSION:
>         result = make_pack_expansion (op0);
>         PACK_EXPANSION_SIZEOF_P (result) = true;
> 
> base-commit: 5fb204aaf34b68c427f5b2bfb933fed72fe3eafb