From patchwork Mon Mar 15 00:17:28 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Iain Sandoe X-Patchwork-Id: 1452868 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4DzH773kmJz9sVb for ; Mon, 15 Mar 2021 11:17:37 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 863A6385F01F; Mon, 15 Mar 2021 00:17:34 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp002.apm-internet.net (smtp002.apm-internet.net [85.119.248.221]) by sourceware.org (Postfix) with ESMTPS id A3E46385F01F for ; Mon, 15 Mar 2021 00:17:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A3E46385F01F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sandoe.co.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=iain@sandoe.co.uk Received: (qmail 26787 invoked from network); 15 Mar 2021 00:17:29 -0000 X-APM-Out-ID: 16157674492678 X-APM-Authkey: 257869/1(257869/1) 2 Received: from unknown (HELO ?192.168.1.212?) (81.138.1.83) by smtp002.apm-internet.net with SMTP; 15 Mar 2021 00:17:29 -0000 From: Iain Sandoe Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [PATCH] coroutines : Avoid generating empty statements [96749]. Message-Id: <3E51A1E6-55E5-4AB3-A259-2E68184CF2C9@sandoe.co.uk> Date: Mon, 15 Mar 2021 00:17:28 +0000 To: gcc-patches@gcc.gnu.org X-Mailer: Apple Mail (2.3273) X-Spam-Status: No, score=-17.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_COUK, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: , Cc: Nathan Sidwell Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" Hi In the compiler-only idiom: " a = (target expr creates temp, op uses temp) " the target expression variable needs to be promoted to a frame one (if the expression has a suspend point). However, the only uses of the var are in the second part of the compound expression - and we were creating an empty statement corresponding to the (now unused) first arm. This then produces the spurious warnings noted. Fixed by avoiding generation of a separate variable nest for isolated target expressions (or similarly isolated co_awaits used in a function call). tested on x86_64-darwin, x86_64-linux-gnu and with cppcoro and folly/coroutines OK for master/10.x? thanks Iain gcc/cp/ChangeLog: * coroutines.cc (flatten_await_stmt): Allow for the case where a target expression variable only has uses in the second part of a compound expression. (maybe_promote_temps): Avoid emiting empty statements. gcc/testsuite/ChangeLog: * g++.dg/coroutines/pr96749-1.C: New test. * g++.dg/coroutines/pr96749-2.C: New test. --- gcc/cp/coroutines.cc | 64 ++++++++++++++------- gcc/testsuite/g++.dg/coroutines/pr96749-1.C | 42 ++++++++++++++ gcc/testsuite/g++.dg/coroutines/pr96749-2.C | 37 ++++++++++++ 3 files changed, 123 insertions(+), 20 deletions(-) create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96749-1.C create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96749-2.C diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index c5aeb660ae8..712431583d5 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -2955,7 +2955,9 @@ flatten_await_stmt (var_nest_node *n, hash_set *promoted, break; case TARGET_EXPR: { - /* We have a temporary; promote it. */ + /* We have a temporary; promote it, but allow for the idiom in code + generated by the compiler like + a = (target_expr produces temp, op uses temp). */ tree init = t; temps_used->add (init); tree var_type = TREE_TYPE (init); @@ -2976,20 +2978,35 @@ flatten_await_stmt (var_nest_node *n, hash_set *promoted, } else init = build2 (INIT_EXPR, var_type, var, init); - var_nest_node *ins - = new var_nest_node (var, init, n->prev, n); - /* We have to replace the target expr... */ - *v.entry = var; - /* ... and any uses of its var. */ - proxy_replace pr = {TREE_OPERAND (t, 0), var}; - cp_walk_tree (&n->init, replace_proxy, &pr, NULL); - /* Compiler-generated temporaries can also have uses in following - arms of compound expressions, which will be listed in 'replace_in' - if present. */ - if (replace_in) - cp_walk_tree (replace_in, replace_proxy, &pr, NULL); - flatten_await_stmt (ins, promoted, temps_used, NULL); - flatten_await_stmt (n, promoted, temps_used, NULL); + /* Simplify for the case that we have an init containing the temp + alone. */ + if (t == n->init && n->var == NULL_TREE) + { + n->var = var; + proxy_replace pr = {TREE_OPERAND (t, 0), var}; + cp_walk_tree (&init, replace_proxy, &pr, NULL); + n->init = init; + if (replace_in) + cp_walk_tree (replace_in, replace_proxy, &pr, NULL); + flatten_await_stmt (n, promoted, temps_used, NULL); + } + else + { + var_nest_node *ins + = new var_nest_node (var, init, n->prev, n); + /* We have to replace the target expr... */ + *v.entry = var; + /* ... and any uses of its var. */ + proxy_replace pr = {TREE_OPERAND (t, 0), var}; + cp_walk_tree (&n->init, replace_proxy, &pr, NULL); + /* Compiler-generated temporaries can also have uses in + following arms of compound expressions, which will be listed + in 'replace_in' if present. */ + if (replace_in) + cp_walk_tree (replace_in, replace_proxy, &pr, NULL); + flatten_await_stmt (ins, promoted, temps_used, NULL); + flatten_await_stmt (n, promoted, temps_used, NULL); + } return; } break; @@ -3178,7 +3195,6 @@ maybe_promote_temps (tree *stmt, void *d) gcc_checking_assert (root->next == NULL); tree vlist = NULL_TREE; var_nest_node *t = root; - gcc_checking_assert (!t->var); /* We build the bind scope expression from the bottom-up. EXPR_LIST holds the inner expression nest at the current cleanup level (becoming the final expression list when we've exhausted the @@ -3214,9 +3230,12 @@ maybe_promote_temps (tree *stmt, void *d) add_stmt (cl); /* push this onto the level above. */ } else if (expr_list) - add_stmt (expr_list); - else - gcc_unreachable (); + { + if (TREE_CODE (expr_list) != STATEMENT_LIST) + add_stmt (expr_list); + else if (!tsi_end_p (tsi_start (expr_list))) + add_stmt (expr_list); + } } else { @@ -3225,7 +3244,12 @@ maybe_promote_temps (tree *stmt, void *d) else finish_expr_stmt (t->init); if (expr_list) - add_stmt (expr_list); + { + if (TREE_CODE (expr_list) != STATEMENT_LIST) + add_stmt (expr_list); + else if (!tsi_end_p (tsi_start (expr_list))) + add_stmt (expr_list); + } } expr_list = pop_stmt_list (new_list); var_nest_node *old = t; diff --git a/gcc/testsuite/g++.dg/coroutines/pr96749-1.C b/gcc/testsuite/g++.dg/coroutines/pr96749-1.C new file mode 100644 index 00000000000..941a64e6379 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr96749-1.C @@ -0,0 +1,42 @@ +// { dg-additional-options "-Wall" } + +#include + +template struct promise; +template struct task { + using promise_type = promise<_Tp>; + bool await_ready() noexcept { return false; } + void await_suspend(std::coroutine_handle<> awaiter) noexcept { m_a = awaiter; } + auto await_resume() { return _Tp(); } + std::coroutine_handle<> m_a; +}; + +template struct promise { + auto get_return_object() noexcept { return task<_Tp>(); } + auto initial_suspend() noexcept { return std::suspend_always(); } + auto final_suspend() noexcept { return std::suspend_always(); } + void return_value(_Tp value) noexcept { m_v = value; } + void unhandled_exception() noexcept {} + _Tp m_v; +}; + +task test_coro(void) { + int r = 0; +#if 1 + // this code causes the unexpected warning below + r += co_await task(); +#else + // this code causes no warning + auto b = co_await task(); + r += b; +#endif + co_return r; + // test1.cpp: In function ‘task test_coro(int)’: + // test1.cpp:36:1: warning: statement has no effect [-Wunused-value] + // 36 | } + // | ^ +} + +int main(void) { + return 0; +} diff --git a/gcc/testsuite/g++.dg/coroutines/pr96749-2.C b/gcc/testsuite/g++.dg/coroutines/pr96749-2.C new file mode 100644 index 00000000000..43052b57dd9 --- /dev/null +++ b/gcc/testsuite/g++.dg/coroutines/pr96749-2.C @@ -0,0 +1,37 @@ +// { dg-additional-options "-Wall" } + +#include + +#if 1 +// with a struct, GCC emits "statement has no effect" +struct S {}; +#else +// no warning with built-in types +using S = int; +#endif + +S Func1(int); + +struct C { + auto operator co_await() { + struct Awaitable final { + bool await_ready() const { return true; } + std::coroutine_handle<> await_suspend(std::coroutine_handle<>) { return {}; } + int await_resume() { return 42; } + }; + return Awaitable{}; + } +}; + +struct Task { + struct promise_type { + auto initial_suspend() { return std::suspend_always{}; } + auto final_suspend() noexcept { return std::suspend_always{}; } + void get_return_object() {} + void unhandled_exception() {} + }; +}; + +Task Func2() { + Func1(co_await C()); +}