From patchwork Thu Oct 31 08:40:08 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Iain Sandoe X-Patchwork-Id: 99899 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5B7E23857831 for ; Thu, 31 Oct 2024 08:40:45 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by sourceware.org (Postfix) with ESMTPS id 83E503858D21 for ; Thu, 31 Oct 2024 08:40:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 83E503858D21 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 83E503858D21 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::332 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1730364016; cv=none; b=Ifaxx7DIUb9wc/mC6CZnwxQFbhIuJYhbySLlfiMB984a1w7ghhjJx3x8idvBrD3Rrewyx+j48F9HgVvMIKvg5paL8XzQJgHIenNGNEhSloKLjYOZx3tfEfoxuVZnTPiEh1DKONZsvxoaXG+PnMYDs2eYP9FwCuEn8f22Wcztsj4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1730364016; c=relaxed/simple; bh=jlAM+HJU3Tt6XmUyTlR7CBn0bzESSny0sf15/GTp6vQ=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=adS9AaM/012tVeGbZIpvGvONdA2YNCsZ3vM6n53DdqgVdN2P45n7/Wbbi2tqzY5dRm1IjAptA4yXI5Ls/c6WFNz1IRMwp/faU9lQJ1hSoY1wVcRzZ6jPhQ32XBFt0xwxfTUxHHpwpdXxuilK4+pqDAp+eaql58DNpqyhL9DCSdU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-x332.google.com with SMTP id 5b1f17b1804b1-43155afca99so10727315e9.1 for ; Thu, 31 Oct 2024 01:40:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730364010; x=1730968810; darn=gcc.gnu.org; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:date:subject:cc:to:from:from:to:cc:subject :date:message-id:reply-to; bh=nWqTclEnrQyOhTRnn/RjZ54Zx0kU9ZcYmeovpTklCDc=; b=J9qUvMhjBMvhehcjD9zrT/ZjgdQM9QcxBic6syhdjlAOXNzvdHGyVjc+45D2puZIWG yoQBFAhq8DG4f18MmTIFYoUPe3hcnE3AcAalK6Z8Wq6UexR7bdkIon9irRWYWQhiTu/B nBjle2vz3E9txxiUBCgUnpOv9WLdWPRkwGGY0ThFhhZ6zBHALiTeKCyQGeXbxrRnByEE ff+xZLEMeMD1lejTj5bFSQ4y/rjCfwupTEBMA9yswIe4bey1RI1m4G47I5BvP0uaixzX wdx+TovTvE0AhX82Q7T5SZD2a2fUqfDZBKedF18eYFnmLIXb6R8yFlBJ4SwRGWC2TZ8j vFHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730364010; x=1730968810; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:date:subject:cc:to:from:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=nWqTclEnrQyOhTRnn/RjZ54Zx0kU9ZcYmeovpTklCDc=; b=JuiLJAHMH/FLkUj1b/WI2rrtTMPNgMg0nRE9aLw+6e97T5vLcSinQWQHETMBRFphAs Y9x+g+jaxpixe5O+57EECGkuxzphe/DlBQhtta1gWNqc4DGXqGm2MtaxaX4QrjpuYZFX IlVXkscwtS2Cv2yJE05e98WkqaIKWBi0/zlCJaWvhUY/2/g5k6oZn+y9hJkuyrLNtPeC n5uX/RjNqRj1pbT2NCNBEk+3q3LCad1Vb9q4VSaNXNaM/eGEyw00C+55Ukkz0fO3ikkA MKK/Ywjv7phqdofd+vSO8Vs+fWT8XAcTB11DsV9HlwX+vytHbQesfZN7XN69uZ78TB9F nWJw== X-Gm-Message-State: AOJu0Yyw2ZVVlmoCM8VUIlDFxQWkwwD/t/4dD5w2QhweiLFOb7HehTuq pm/tKHdjHRQ95qtizJbJLraIIPPTSubYj5BcFIfAdftUgWKFFWK8k0oxkA== X-Google-Smtp-Source: AGHT+IHwiV1hFsv1Y2wEEL9Iu8EJjghYx+sUJk3k+Yw2QKwB1r1gqmA9TwBpq78RrIAHYoMvqlcw4Q== X-Received: by 2002:a5d:64c7:0:b0:374:cd3c:db6d with SMTP id ffacd0b85a97d-381c127c98dmr1131492f8f.6.1730364009693; Thu, 31 Oct 2024 01:40:09 -0700 (PDT) Received: from localhost.localdomain (host81-138-1-83.in-addr.btopenworld.com. [81.138.1.83]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-381c10b7b8csm1433755f8f.5.2024.10.31.01.40.09 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Thu, 31 Oct 2024 01:40:09 -0700 (PDT) From: Iain Sandoe X-Google-Original-From: Iain Sandoe To: gcc-patches@gcc.gnu.org Cc: jason@redhat.com Subject: [PATCH v3] c++, coroutines: Fix awaiter var creation [PR116506]. Date: Thu, 31 Oct 2024 08:40:08 +0000 Message-Id: <20241031084008.85568-1-iain@sandoe.co.uk> X-Mailer: git-send-email 2.39.2 (Apple Git-143) In-Reply-To: References: MIME-Version: 1.0 X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: iain@sandoe.co.uk Errors-To: gcc-patches-bounces~patchwork=sourceware.org@gcc.gnu.org This version tested on x86_64-darwin,linux, powerpc64-linux, on folly and by Sam on wider codebases, >>>Why don't you need a variable to preserve o across suspensions if it's a call returning lvalue reference? >>We always need a space for the awaiter, unless it is already a variable/parameter (or part of one). >>> I suspect that the simple case is not lvalue_p, but !TREE_SIDE_EFFECTS. >>That is likely where I’m going wrong - we must not generate a variable for any case that already has one (or a parm), but we must for any case that is a temporary. >>So, I should adjust the logic to use !TREE_SIDE_EFFECTS. > Or perhaps DECL_P. The difference would be for compound lvalues like *p or a[n]; if the value of p or a or n could change across suspension, the same side-effect-free lvalue expression could refer to a different object. Right, part of the code that was elided catered for the compound values by making a reference to the original entity and placing that in the frame. We restore that behaviour here. Note that there is no point in making a reference to an xvalue (we'd only have to save the expiring value in the frame anyway), so we just go ahead and build that var directly. There is one small additional optimisation, in that building a reference to a non-pointer component ref wastes frame space if the underlying entity is a variable or parameter. Thanks for the help in working through the different cases here, OK for trunk now? thanks Iain. --- 8< --- Awaiters always need to have a coroutine state frame copy since they persist across potential supensions. It simplifies the later analysis considerably to assign these early which we do when building co_await expressions. The cleanups in r15-3146-g47dbd69b1, unfortunately elided some of processing used to cater for cases where the var created from an xvalue, or is a pointer/reference type. Corrected thus. PR c++/116506 PR c++/116880 gcc/cp/ChangeLog: * coroutines.cc (build_co_await): Ensure that xvalues are materialised. Handle references/pointer values in awaiter access expressions. gcc/testsuite/ChangeLog: * g++.dg/coroutines/pr116506.C: New test. * g++.dg/coroutines/pr116880.C: New test. Signed-off-by: Iain Sandoe --- gcc/cp/coroutines.cc | 82 +++++++++++++++++----- gcc/testsuite/g++.dg/coroutines/pr116506.C | 53 ++++++++++++++ gcc/testsuite/g++.dg/coroutines/pr116880.C | 36 ++++++++++ 3 files changed, 154 insertions(+), 17 deletions(-) create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116506.C create mode 100644 gcc/testsuite/g++.dg/coroutines/pr116880.C diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index ba326bcd627..dde8ba4e614 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -1072,6 +1072,30 @@ build_template_co_await_expr (location_t kw, tree type, tree expr, tree kind) return aw_expr; } +/* For a component ref that is not a pointer type, decide if we can use + this directly. */ +static bool +usable_component_ref (tree comp_ref) +{ + if (TREE_CODE (comp_ref) != COMPONENT_REF + || TREE_SIDE_EFFECTS (comp_ref)) + return false; + + while (TREE_CODE (comp_ref) == COMPONENT_REF) + { + comp_ref = TREE_OPERAND (comp_ref, 0); + STRIP_NOPS (comp_ref); + /* x-> */ + if (INDIRECT_REF_P (comp_ref)) + return false; + /* operator-> */ + if (TREE_CODE (comp_ref) == CALL_EXPR) + return false; + STRIP_NOPS (comp_ref); + } + gcc_checking_assert (VAR_P (comp_ref) || TREE_CODE (comp_ref) == PARM_DECL); + return true; +} /* This performs [expr.await] bullet 3.3 and validates the interface obtained. It is also used to build the initial and final suspend points. @@ -1134,13 +1158,12 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind, if (o_type && !VOID_TYPE_P (o_type)) o_type = complete_type_or_else (o_type, o); - if (!o_type) + if (!o_type || o_type == error_mark_node) return error_mark_node; if (TREE_CODE (o_type) != RECORD_TYPE) { - error_at (loc, "awaitable type %qT is not a structure", - o_type); + error_at (loc, "awaitable type %qT is not a structure", o_type); return error_mark_node; } @@ -1166,20 +1189,47 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind, if (!glvalue_p (o)) o = get_target_expr (o, tf_warning_or_error); - tree e_proxy = o; - if (glvalue_p (o)) + /* We know that we need a coroutine state frame variable for the awaiter, + since it must persist across suspension; so where one is needed, build + it now. */ + tree e_proxy = STRIP_NOPS (o); + if (INDIRECT_REF_P (e_proxy)) + e_proxy = TREE_OPERAND (e_proxy, 0); + o_type = TREE_TYPE (e_proxy); + bool is_ptr = TYPE_PTR_P (o_type); + if (!is_ptr + && (TREE_CODE (e_proxy) == PARM_DECL + || usable_component_ref (e_proxy) + || (VAR_P (e_proxy) && !is_local_temp (e_proxy)))) o = NULL_TREE; /* Use the existing entity. */ - else /* We need to materialise it. */ + else /* We need a non-temp var. */ { - e_proxy = get_awaitable_var (suspend_kind, o_type); - o = cp_build_init_expr (loc, e_proxy, o); - e_proxy = convert_from_reference (e_proxy); + tree p_type = o_type; + tree o_a = o; + if (lvalue_p (o) && !TREE_SIDE_EFFECTS (o)) + { + if (is_ptr) + p_type = TREE_TYPE (p_type); + p_type = cp_build_reference_type (p_type, false); + o_a = build_address (o); + } + if (INDIRECT_REF_P (o_a)) + o_a = TREE_OPERAND (o_a, 0); + e_proxy = get_awaitable_var (suspend_kind, p_type); + o = cp_build_init_expr (loc, e_proxy, o_a); } + /* Build an expression for the object that will be used to call the awaitable + methods. */ + tree e_object = convert_from_reference (e_proxy); + if (TYPE_PTR_P (TREE_TYPE (e_object))) + e_object = cp_build_indirect_ref (input_location, e_object, RO_UNARY_STAR, + tf_warning_or_error); + /* I suppose we could check that this is contextually convertible to bool. */ tree awrd_func = NULL_TREE; tree awrd_call - = build_new_method_call (e_proxy, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL, + = build_new_method_call (e_object, awrd_meth, NULL, NULL_TREE, LOOKUP_NORMAL, &awrd_func, tf_warning_or_error); if (!awrd_func || !awrd_call || awrd_call == error_mark_node) @@ -1193,7 +1243,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind, tree h_proxy = get_coroutine_self_handle_proxy (current_function_decl); vec *args = make_tree_vector_single (h_proxy); tree awsp_call - = build_new_method_call (e_proxy, awsp_meth, &args, NULL_TREE, + = build_new_method_call (e_object, awsp_meth, &args, NULL_TREE, LOOKUP_NORMAL, &awsp_func, tf_warning_or_error); release_tree_vector (args); @@ -1225,7 +1275,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind, /* Finally, the type of e.await_resume() is the co_await's type. */ tree awrs_func = NULL_TREE; tree awrs_call - = build_new_method_call (e_proxy, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL, + = build_new_method_call (e_object, awrs_meth, NULL, NULL_TREE, LOOKUP_NORMAL, &awrs_func, tf_warning_or_error); if (!awrs_func || !awrs_call || awrs_call == error_mark_node) @@ -1239,7 +1289,7 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind, return error_mark_node; if (coro_diagnose_throwing_fn (awrs_func)) return error_mark_node; - if (tree dummy = cxx_maybe_build_cleanup (e_proxy, tf_none)) + if (tree dummy = cxx_maybe_build_cleanup (e_object, tf_none)) { if (CONVERT_EXPR_P (dummy)) dummy = TREE_OPERAND (dummy, 0); @@ -1250,7 +1300,8 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind, } /* We now have three call expressions, in terms of the promise, handle and - 'e' proxies. Save them in the await expression for later expansion. */ + 'e' proxy expression. Save them in the await expression for later + expansion. */ tree awaiter_calls = make_tree_vec (3); TREE_VEC_ELT (awaiter_calls, 0) = awrd_call; /* await_ready(). */ @@ -1263,9 +1314,6 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind, } TREE_VEC_ELT (awaiter_calls, 2) = awrs_call; /* await_resume(). */ - if (REFERENCE_REF_P (e_proxy)) - e_proxy = TREE_OPERAND (e_proxy, 0); - tree awrs_type = TREE_TYPE (TREE_TYPE (awrs_func)); tree suspend_kind_cst = build_int_cst (integer_type_node, (int) suspend_kind); diff --git a/gcc/testsuite/g++.dg/coroutines/pr116506.C b/gcc/testsuite/g++.dg/coroutines/pr116506.C new file mode 100644 index 00000000000..57a6e360566 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr116506.C @@ -0,0 +1,53 @@ +// { dg-do run } +// { dg-additional-options "-fno-exceptions" } + +#include + +bool g_too_early = true; +std::coroutine_handle<> g_handle; + +struct Awaiter { + Awaiter() {} + ~Awaiter() { + if (g_too_early) { + __builtin_abort (); + } + } + + bool await_ready() { return false; } + void await_suspend(std::coroutine_handle<> handle) { + g_handle = handle; + } + + void await_resume() {} +}; + +struct SuspendNever { + bool await_ready() noexcept { return true; } + void await_suspend(std::coroutine_handle<>) noexcept {} + void await_resume() noexcept {} +}; + +struct Coroutine { + struct promise_type { + Coroutine get_return_object() { return {}; } + SuspendNever initial_suspend() { return {}; } + SuspendNever final_suspend() noexcept { return {}; } + void return_void() {} + void unhandled_exception() {} + + Awaiter&& await_transform(Awaiter&& u) { + return static_cast(u); + } + }; +}; + +Coroutine foo() { + co_await Awaiter{}; +} + +int main() { + foo(); + g_too_early = false; + g_handle.destroy(); +} diff --git a/gcc/testsuite/g++.dg/coroutines/pr116880.C b/gcc/testsuite/g++.dg/coroutines/pr116880.C new file mode 100644 index 00000000000..f0db6a26044 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr116880.C @@ -0,0 +1,36 @@ +#include + +struct promise_type; +using handle_type = std::coroutine_handle; + +struct Co { + handle_type handle; + using promise_type = ::promise_type; + + explicit Co(handle_type handle) : handle(handle) {} + + bool await_ready() { return false; } + std::coroutine_handle<> await_suspend(handle_type handle); + void await_resume() {} +}; + +struct Done {}; + +struct promise_type { + Co get_return_object(); + + std::suspend_always initial_suspend() { return {}; }; + std::suspend_always final_suspend() noexcept { return {}; }; + void return_value(Done) {} + void return_value(Co&&); + void unhandled_exception() { throw; }; + Co&& await_transform(Co&& co) { return static_cast(co); } +}; + +Co tryToRun(); + +Co init() +{ + co_await tryToRun(); + co_return Done{}; +}