| Message ID | ZcZvkVVAsxe21kvK@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] c++: make build_throw SFINAE-friendly [PR98388] | expand |
On 2/9/24 13:31, Marek Polacek wrote: > On Fri, Feb 09, 2024 at 10:07:33AM -0500, Jason Merrill wrote: >> On 2/8/24 17:20, Marek Polacek wrote: >>> On Thu, Feb 08, 2024 at 04:53:45PM -0500, Jason Merrill wrote: >>>> 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. >>> >>> I could add >>> #if __cplusplus >= 202002L >>> static_assert (b, "move from the function parameter"); >>> #else >>> static_assert (!b, "no move from the function parameter"); >>> #endif >>> >>> but that's going to fail for C++20 and above. >> >> Ah, because treat_lvalue_as_rvalue_p doesn't recognize t as being from the >> current scope in the trailing return type. But that shouldn't be necessary: >> >> https://eel.is/c++draft/expr.prim.id.unqual#4.2 says it's move-eligible >> "if the id-expression (possibly parenthesized) is the operand of a >> throw-expression ([expr.throw]), and names an implicitly movable entity that >> belongs to a scope that does not contain the compound-statement of the >> innermost lambda-expression, try-block, or function-try-block (if any) whose >> compound-statement or ctor-initializer contains the throw-expression." >> >> here there is no enclosing lambda or try, so it seems move-eligible. > > ...and as a result, 't' will be an xvalue, therefore moveonly(moveonly&&) > should be used, therefore the first overload should be used, which means 'b' > is true. Yep. >>> I wonder if this is the >>> second half of the problem in 113789? >>> >>> I could comment the first static_assert and add a FIXME if that sounds good? >> >> dg-bogus would be better than commenting it out. > > Ok, in this patch the only change I made is: > > +#if __cplusplus >= 202002L > +static_assert (b, "move from the function parameter"); // { dg-bogus "" "PR113853" { xfail c++20 } } > +#else > +static_assert (!b, "no move from the function parameter"); > +#endif OK. >> Will you also look into fixing the treat_ bug? That can be a separate >> patch. > > I'd like to. I opened <https://gcc.gnu.org/PR113853>. > > -- >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. > > There's still one problem for which I opened <https://gcc.gnu.org/PR113853>. > We need to patch up treat_lvalue_as_rvalue_p and remove the dg-bogus. > > 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 | 21 +++++++++++++++++ > libcc1/libcp1plugin.cc | 4 ++-- > 7 files changed, 43 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..361e0b79ba2 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/sfinae69.C > @@ -0,0 +1,21 @@ > +// 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{}); > +#if __cplusplus >= 202002L > +static_assert (b, "move from the function parameter"); // { dg-bogus "" "PR113853" { xfail c++20 } } > +#else > +static_assert (!b, "no move from the function parameter"); > +#endif > 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: 41a6d2560500a202708e7b661b8b2ad432aee3a6
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..361e0b79ba2 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/sfinae69.C @@ -0,0 +1,21 @@ +// 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{}); +#if __cplusplus >= 202002L +static_assert (b, "move from the function parameter"); // { dg-bogus "" "PR113853" { xfail c++20 } } +#else +static_assert (!b, "no move from the function parameter"); +#endif 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;