From patchwork Sun Sep 4 19:04:23 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Arsen_Arsenovi=C4=87?= X-Patchwork-Id: 956 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:ecc5:0:0:0:0:0 with SMTP id s5csp1650440wro; Sun, 4 Sep 2022 12:08:08 -0700 (PDT) X-Google-Smtp-Source: AA6agR4D/6kbhXGkXz5nMXBQTpo9Zg6J/XeraQchklUP28LzRfQiw4Qj5D1GO3JniRh6YXIb1j1n X-Received: by 2002:a05:6402:2a06:b0:447:820a:1afe with SMTP id ey6-20020a0564022a0600b00447820a1afemr41119149edb.291.1662318488229; Sun, 04 Sep 2022 12:08:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1662318488; cv=none; d=google.com; s=arc-20160816; b=jxBfVPoZStVOorOcJzUrFpKpyky19QfOf1nDK8xalti7oH/0fJiQwk8wyWGjEyxWar K+um/r306f72bcyebYVIYxL7xx8ggCvp5PMx23GTh8etNK+eu6mYRtWgBUcLLmpxG9h+ 5kuY1rYTrQ+iOobmSPFHB7L/s9uiKFF2EIgwn1IuVouHAmdKt1NiCLhJT/bfG+DkW7xe V4cMFl/Pb3H9v8LTweNKOtErRiGxIboHFy7KQhtR0YPkiTOMoVJj2QafJqSlQjcNapYe zv+PTLRkqVyAO3+kCReyJyhJKfOD34j2bhqG2cDDQqSp3is03ScZuUZuAci6v3kyTL8O pyBw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:cc:reply-to:from:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence :content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:dmarc-filter:delivered-to:dkim-signature :dkim-filter; bh=kveYk2m9L9JZKtEzD5pAQAvMc3PrXxjVXflKc8NEEFo=; b=vIRJl5aUhtzNwudkLmbJ/XJc2OIUjHZyg0h46+TN1o0XfjLN9GbLW7waHrdmKwhmD/ QMFcJR/b9wMVjyPv3rIVSbe2+SAMhnG2oapD0E8z2+FpH1wxJoLN91OCObmb6QmDRhrl QN0AC/u1B9P9WVS+jHhz0cB2A/ubDeLMUASvrQQRflCU76KO3z1ElWvnD6p5qlKtyUk/ 9lnEAPg8XGAE//Ty33RCrW/gxeoFSujlTntwjoUWV+xT2ghZ7mruRNCqrMUGYXaszYfw ZBDIEvBkp4xIeITWFBHiyRPK8smFAvw1pZL1j7JPBmHxYS1AYpMeLHrRh8Yi1vhUyGoo PRjw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=gWb4xagU; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id 9-20020a170906328900b0073dd243ae3bsi5645107ejw.505.2022.09.04.12.08.08 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 04 Sep 2022 12:08:08 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=gWb4xagU; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 04649385AC24 for ; Sun, 4 Sep 2022 19:08:07 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 04649385AC24 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1662318487; bh=kveYk2m9L9JZKtEzD5pAQAvMc3PrXxjVXflKc8NEEFo=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=gWb4xagUCcTw+h+8hamk6Vyqqo4i9BW8JpBYwkdtpIhlkY6Xp5jBG+sKx1My5yJlD vqvEuo4W2OU7BNc8vVb67XKEcOmNbvS9U+Ywp4/7zmE4gt/IyrTlfeqsg2ciFb5m3J uss3KM4xwhZM2sxD4nWWaJ3FLIiDAXCugIKIIN8k= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mout-p-201.mailbox.org (mout-p-201.mailbox.org [IPv6:2001:67c:2050:0:465::201]) by sourceware.org (Postfix) with ESMTPS id 97A2D3858CDB for ; Sun, 4 Sep 2022 19:07:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 97A2D3858CDB Received: from smtp202.mailbox.org (smtp202.mailbox.org [IPv6:2001:67c:2050:b231:465::202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-201.mailbox.org (Postfix) with ESMTPS id 4MLLkH2GF8z9sR7; Sun, 4 Sep 2022 21:07:19 +0200 (CEST) To: Iain Sandoe Subject: [PATCH v2] coroutines: Ensure there's a top level bind when rewriting [PR106188] Date: Sun, 4 Sep 2022 21:04:23 +0200 Message-Id: <20220904190423.1278126-1-arsen@aarsen.me> In-Reply-To: References: MIME-Version: 1.0 X-Rspamd-Queue-Id: 4MLLkH2GF8z9sR7 X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_INFOUSMEBIZ, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: =?utf-8?q?Arsen_Arsenovi=C4=87_via_Gcc-patches?= From: =?utf-8?q?Arsen_Arsenovi=C4=87?= Reply-To: =?utf-8?q?Arsen_Arsenovi=C4=87?= Cc: gcc-patches@gcc.gnu.org, nathan@acm.org Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1743067271098615074?= X-GMAIL-MSGID: =?utf-8?q?1743067271098615074?= In the edge case of a coroutine not containing any locals, the ifcd/swch temporaries would get added to the coroutine frame, corrupting its layout. To prevent this, we can make sure there is always a BIND_EXPR at the top of the function body, and thus, always a place for our new temporaries to go without interfering with the coroutine frame. PR c++/106188 - [coroutines] Incorrect frame layout after transforming conditional statement without top-level bind expression PR c++/106713 - [11/12/13 Regression] Coroutine regression in GCC 11.3.0: if (co_await ...) crashes with a jump to ud2 since r12-3529-g70ee703c479081ac gcc/cp/ChangeLog: PR c++/106188 PR c++/106713 * coroutines.cc (coro_rewrite_function_body): Ensure we have a BIND_EXPR wrapping the function body. gcc/testsuite/ChangeLog: * g++.dg/coroutines/pr106188.C: New test. Signed-off-by: Arsen Arsenović --- gcc/cp/coroutines.cc | 10 +++++++ gcc/testsuite/g++.dg/coroutines/pr106188.C | 35 ++++++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 gcc/testsuite/g++.dg/coroutines/pr106188.C Hi Iain, > I can see that adding the bind expression in this way seems to avoid > adding one unnecessarily***, however it makes a complex part of the > code even more complex (and conflates two jobs). > > It seems to me that it would be better to add the bind expression > consistently and to do it in the part of the code that deals with > outlining the original functiion - so … > > … in coro_rewrite_function_body() we check to see if the outlined > body has a bind expression and connect up the BLOCKs if so (otherwise > debug will not work). > > How about adding the bind expression consistently by appending an else > clause to that check (coroutines.cc:4079..40097) ? > > Since the function has no local variables, there is no BLOCK tree at > this point (so there is no work to do in connecting them up). > > I’d much prefer to keep the work of restructuring the function > separate from the work of analysing the await expressions (if > possible). > > WDYT? Iain I actually quite like the idea, it's a lot more elegant! I'm not quite confident enough to tell whether this will have any adverse effects (as I am a quite fresh contributor to GCC), but I implemented it anyway, ran the tests, and they come up green in comparison to the tree I based this patch on. The reason I implemented maybe_add_bind initially is to make a minimally intrusive change, but I'm not sure that's worth it with the extra potential for confusion (and for error, as this is something that'd have to be handled by every caller to add_var_to_bind). Thank you for the swift response! - Arsen diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index edb3b706ddc..4e7f1c4a08c 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -4095,6 +4095,16 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig, BLOCK_SUPERCONTEXT (replace_blk) = top_block; BLOCK_SUBBLOCKS (top_block) = replace_blk; } + else + { + /* We are missing a top level BIND_EXPR. We need one to ensure that we + * don't shuffle around the coroutine frame and corrupt it. + */ + tree bind_wrap = build3_loc (fn_start, BIND_EXPR, void_type_node, + NULL, NULL, NULL); + BIND_EXPR_BODY (bind_wrap) = fnbody; + fnbody = bind_wrap; + } /* Wrap the function body in a try {} catch (...) {} block, if exceptions are enabled. */ diff --git a/gcc/testsuite/g++.dg/coroutines/pr106188.C b/gcc/testsuite/g++.dg/coroutines/pr106188.C new file mode 100644 index 00000000000..1de3d4cceca --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr106188.C @@ -0,0 +1,35 @@ +// { dg-additional-options "-std=c++20 -w" } +// { dg-do run } +// test case from pr106188, w/o workaround +#include + +struct task { + struct promise_type { + task get_return_object() { return task{}; } + void return_void() {} + void unhandled_exception() {} + auto initial_suspend() noexcept { return std::suspend_never{}; } + auto final_suspend() noexcept { return std::suspend_never{}; } + }; +}; + +struct suspend_and_resume { + bool await_ready() const { return false; } + void await_suspend(std::coroutine_handle<> h) { h.resume(); } + void await_resume() {} +}; + +task f() { + if (co_await suspend_and_resume{}, false) {} +} + +task g() { + switch (co_await suspend_and_resume{}, 0) { + default: break; + } +} + +int main() { + f(); + g(); +}