On 8/1/24 12:34 PM, Arsen Arsenović wrote:
Tested on x86_64-pc-linux-gnu.
OK for trunk?
TIA, have a lovely day.
---------- >8 ----------
Currently, it is possible to ICE GCC by giving it sufficiently broken
code, where sufficiently broken means a std::coroutine_handle missing a
default on the promise_type template argument, and missing members.
As the code generator relies on lookups in the coroutine_handle never
failing (and has no way to signal that error), lets do it ahead of time,
save the result, and use that. This saves us some lookups and allows us
to propagate an error.
PR c++/105475 - coroutines: ICE in coerce_template_parms, at cp/pt.cc:9183
gcc/cp/ChangeLog:
PR c++/105475
* coroutines.cc (struct coroutine_info): Add from_address_bl.
Carries the from_address member we looked up earlier.
(coro_resume_identifier): Remove. Unused.
(coro_init_identifiers): Do not initialize the above.
(void_coro_handle_address): New variable. Contains the baselink
for the std::coroutine_handle<void>::address() instance method.
(get_handle_type_address_bl): New function. Looks up and
validates handle_type::address in a given handle_type.
(get_handle_type_from_address_bl): New function. Looks up and
validates handle_type::from_address in a given handle_type.
(ensure_coro_initialized):Remove reliance on
coroutine_handle<> defaulting the promise type to void.
Initialize void_coro_handle_address with
get_handle_type_from_address_bl.
(coro_promise_type_found_p): Initialize
coro_info->from_address_bl with the result of
get_handle_type_from_address_bl.
(get_coroutine_from_address_baselink): New helper. Gets the
handle_type::from_address BASELINK from a coroutine_info.
(build_actor_fn): Use the get_coroutine_from_address_baselink
helper and void_coro_handle_address.
I don't think these names need to mention "baselink", but I'm not
strongly against it.
A few other tweaks below:
@@ -90,6 +90,7 @@ struct GTY((for_user)) coroutine_info
tree self_h_proxy; /* A handle instance that is used as the proxy for the
one that will eventually be allocated in the coroutine
frame. */
+ tree from_address_bl; /* handle_type from_address function (BASELINK). */
Inserting this here breaks the "Likewise" reference in the next line.
tree promise_proxy; /* Likewise, a proxy promise instance. */
tree return_void; /* The expression for p.return_void() if it exists. */
location_t first_coro_keyword; /* The location of the keyword that made this
@@ -389,7 +389,105 @@ find_coro_handle_template_decl (location_t kw)
return handle_decl;
}
-/* Instantiate the handle template for a given promise type. */
+/* Get and validate HANDLE_TYPE#address. The resulting function, if any, will
:: instead of #
+ be a non-overloaded member function that takes no arguments and returns
+ void*. If that is not the case, signals an error and returns NULL_TREE. */
+
+static tree
+get_handle_type_address_bl (location_t kw, tree handle_type)
+{
+ tree addr_getter = lookup_member (handle_type, coro_address_identifier, 1,
+ 0, tf_warning_or_error);
+ if (!addr_getter || addr_getter == error_mark_node)
+ {
+ error_at (kw, "could not find %qE in %qT",
+ coro_address_identifier, handle_type);
How about using qualified_name_lookup_error?
+ gcc_assert (TREE_CODE (addr_getter) == BASELINK);
This looks like it will ICE on an invalid handle type (e.g. where
"address" is a type or data member) instead of giving an error.
+ tree fn_t = TREE_TYPE (addr_getter);
+ if (TREE_CODE (fn_t) != METHOD_TYPE)
+ {
+ error_at (kw, "%qE must be a non-overloaded method", addr_getter);
+ return NULL_TREE;
+ }
+
+ tree arg = TYPE_ARG_TYPES (fn_t);
+ /* Given that this is a method, we have an argument to skip (the this
+ pointer). */
+ gcc_checking_assert (TREE_CHAIN (arg));
This assert seems redundant with the following checks.
+ arg = TREE_CHAIN (arg);
+
+ /* Check that from_addr has the argument list (). */
+ if (!arg
+ || !same_type_p (TREE_VALUE (arg), void_type_node)
+ || TREE_CHAIN (arg))
+ {
+ error_at (kw, "%qE must take no arguments", addr_getter);
+ return NULL_TREE;
+ }
I think this could just be
if (arg != void_list_node)
as we share the terminal void between all non-varargs function types.
+ tree ret_t = TREE_TYPE (fn_t);
+ if (!same_type_p (ret_t, ptr_type_node))
+ {
+ error_at (kw, "%qE must return %qT, not %qT",
+ addr_getter, ptr_type_node, ret_t);
+ return NULL_TREE;
+ }
Do you also want to check that it has public access?
@@ -454,10 +552,15 @@ ensure_coro_initialized (location_t loc)
/* We can also instantiate the void coroutine_handle<> */
void_coro_handle_type =
- instantiate_coro_handle_for_promise_type (loc, NULL_TREE);
+ instantiate_coro_handle_for_promise_type (loc, void_type_node);
if (void_coro_handle_type == NULL_TREE)
return false;
+ void_coro_handle_address =
+ get_handle_type_address_bl (loc, void_coro_handle_type);
The = should be at the beginning of the second line for both variables.
@@ -2365,8 +2480,9 @@ build_actor_fn (location_t loc, tree coro_frame_type,
tree actor, tree fnbody,
tree ash = build_class_member_access_expr (actor_frame, ash_m, NULL_TREE,
false, tf_warning_or_error);
/* So construct the self-handle from the frame address. */
- tree hfa_m = lookup_member (handle_type, coro_from_address_identifier, 1,
- 0, tf_warning_or_error);
+ tree hfa_m = get_coroutine_from_address_baselink (orig);
+ /* Should have been set earlier by coro_promise_type_found_p. */
+ gcc_assert (hfa_m);
Maybe move this assert into the new get function?
Jason
