Message ID | 20220113093921.GT2646553@tucnak |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> 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 143503857C7C for <patchwork@sourceware.org>; Thu, 13 Jan 2022 09:39:57 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 143503857C7C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1642066797; bh=FPL/josF288Rosqzo6yhEVVIaivcAEfpXBv1OB8Hig0=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=vQCFIA0pivN2CcjQaZcioactZwS+mLLS9hPgIJzQGgXFhgb4paYFUOEVMIh7hl0Ix UomKdEyF2jBND6MCqaBMIbDvQUTaYe5VK2dC6+g/ZG7fZ8Jba3hPopvvcObXn9ADpN hnBvRyKvEEGXxgRPUYjKlwv2QesJ7QRO3NkSoOoE= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 1287C3858409 for <gcc-patches@gcc.gnu.org>; Thu, 13 Jan 2022 09:39:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1287C3858409 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-561-jgP9dS4bPA-Ymwy_bpTSvQ-1; Thu, 13 Jan 2022 04:39:26 -0500 X-MC-Unique: jgP9dS4bPA-Ymwy_bpTSvQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6416984B9A4 for <gcc-patches@gcc.gnu.org>; Thu, 13 Jan 2022 09:39:25 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.195.246]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F0E4D5F92C; Thu, 13 Jan 2022 09:39:24 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 20D9dMoT2504290 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 13 Jan 2022 10:39:22 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 20D9dMGB2504289; Thu, 13 Jan 2022 10:39:22 +0100 Date: Thu, 13 Jan 2022 10:39:21 +0100 To: Jason Merrill <jason@redhat.com> Subject: [PATCH] c++: Avoid some -Wreturn-type false positives with const{expr,eval} if [PR103991] Message-ID: <20220113093921.GT2646553@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jakub Jelinek <jakub@redhat.com> Cc: gcc-patches@gcc.gnu.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
c++: Avoid some -Wreturn-type false positives with const{expr,eval} if [PR103991]
|
|
Commit Message
Jakub Jelinek
Jan. 13, 2022, 9:39 a.m. UTC
Hi! The changes done to genericize_if_stmt in order to improve -Wunreachable-code* warning (which Richi didn't actually commit for GCC 12) are I think fine for normal ifs, but for constexpr if and consteval if we have two competing warnings. The problem is that we replace the non-taken clause (then or else) with void_node and keep the if (cond) { something } else {} or if (cond) {} else { something }; in the IL. This helps -Wunreachable-code*, if something can't fallthru but the non-taken clause can, we don't warn about code after it because it is still (in theory) reachable. But if the non-taken branch can't fallthru, we can get false positive -Wreturn-type warnings (which are enabled by default) if there is nothing after the if and the taken branch can't fallthru either. One possibility to fix this is revert at least temporarily to the previous behavior for constexpr and consteval if, yes, we can get false positive -Wunreachable-code* warnings but the warning isn't present in GCC 12. The patch below implements that for constexpr if which throws its clauses very early (either during parsing or during instantiation), and for consteval if it decides based on block_may_fallthru on the non-taken (for constant evaluation only) clause - if the non-taken branch may fallthru, it does what you did in genericize_if_stmt for consteval if, if it can't fallthru, it uses the older way of pretending there wasn't an if and just replacing it with the taken clause. There are some false positive risks with this though, block_may_fallthru is optimistic and doesn't handle some statements at all (like FOR_STMT, WHILE_STMT, DO_STMT - of course handling those is quite hard). For constexpr if (but perhaps for GCC 13?) we could try to block_may_fallthru before we throw it away and remember it in some flag on the IF_STMT, but am not sure how dangerous would it be to call it on the discarded stmts. Or if it is too dangerous e.g. just remember whether the discarded block of consteval if wasn't present or was empty, in that case assume fallthru, and otherwise assume it can't fallthru (-Wunreachable-code possible false positives). Bootstrapped/regtested on x86_64-linux and i686-linux, if needed, I can also test the safer variant with just if (IF_STMT_CONSTEVAL_P (stmt)) stmt = else_; for consteval if. 2022-01-13 Jakub Jelinek <jakub@redhat.com> PR c++/103991 * cp-objcp-common.c (cxx_block_may_fallthru) <case IF_STMT>: For IF_STMT_CONSTEXPR_P with constant false or true condition only check if the taken clause may fall through. * cp-gimplify.c (genericize_if_stmt): For consteval if, revert to r12-5638^ behavior if then_ block can't fall through. For constexpr if, revert to r12-5638^ behavior. * g++.dg/warn/Wreturn-type-13.C: New test. Jakub
Comments
On 1/13/22 04:39, Jakub Jelinek wrote: > Hi! > > The changes done to genericize_if_stmt in order to improve > -Wunreachable-code* warning (which Richi didn't actually commit > for GCC 12) are I think fine for normal ifs, but for constexpr if > and consteval if we have two competing warnings. > The problem is that we replace the non-taken clause (then or else) > with void_node and keep the if (cond) { something } else {} > or if (cond) {} else { something }; in the IL. > This helps -Wunreachable-code*, if something can't fallthru but the > non-taken clause can, we don't warn about code after it because it > is still (in theory) reachable. > But if the non-taken branch can't fallthru, we can get false positive > -Wreturn-type warnings (which are enabled by default) if there is > nothing after the if and the taken branch can't fallthru either. Perhaps we should replace the non-taken clause with __builtin_unreachable() instead of void_node? And/or block_may_fallthru could handle INTEGER_CST op0? > One possibility to fix this is revert at least temporarily > to the previous behavior for constexpr and consteval if, yes, we > can get false positive -Wunreachable-code* warnings but the warning > isn't present in GCC 12. > The patch below implements that for constexpr if which throws its > clauses very early (either during parsing or during instantiation), > and for consteval if it decides based on block_may_fallthru on the > non-taken (for constant evaluation only) clause - if the non-taken > branch may fallthru, it does what you did in genericize_if_stmt > for consteval if, if it can't fallthru, it uses the older way > of pretending there wasn't an if and just replacing it with the > taken clause. There are some false positive risks with this though, > block_may_fallthru is optimistic and doesn't handle some statements > at all (like FOR_STMT, WHILE_STMT, DO_STMT - of course handling those > is quite hard). > For constexpr if (but perhaps for GCC 13?) we could try to > block_may_fallthru before we throw it away and remember it in some > flag on the IF_STMT, but am not sure how dangerous would it be to call > it on the discarded stmts. Or if it is too dangerous e.g. just > remember whether the discarded block of consteval if wasn't present > or was empty, in that case assume fallthru, and otherwise assume > it can't fallthru (-Wunreachable-code possible false positives). > > Bootstrapped/regtested on x86_64-linux and i686-linux, if needed, > I can also test the safer variant with just > if (IF_STMT_CONSTEVAL_P (stmt)) > stmt = else_; > for consteval if. > > 2022-01-13 Jakub Jelinek <jakub@redhat.com> > > PR c++/103991 > * cp-objcp-common.c (cxx_block_may_fallthru) <case IF_STMT>: For > IF_STMT_CONSTEXPR_P with constant false or true condition only > check if the taken clause may fall through. > * cp-gimplify.c (genericize_if_stmt): For consteval if, revert > to r12-5638^ behavior if then_ block can't fall through. For > constexpr if, revert to r12-5638^ behavior. > > * g++.dg/warn/Wreturn-type-13.C: New test. > > --- gcc/cp/cp-objcp-common.c.jj 2022-01-11 23:11:22.091294356 +0100 > +++ gcc/cp/cp-objcp-common.c 2022-01-12 17:57:18.232202275 +0100 > @@ -313,6 +313,13 @@ cxx_block_may_fallthru (const_tree stmt) > return false; > > case IF_STMT: > + if (IF_STMT_CONSTEXPR_P (stmt)) > + { > + if (integer_nonzerop (IF_COND (stmt))) > + return block_may_fallthru (THEN_CLAUSE (stmt)); > + if (integer_zerop (IF_COND (stmt))) > + return block_may_fallthru (ELSE_CLAUSE (stmt)); > + } > if (block_may_fallthru (THEN_CLAUSE (stmt))) > return true; > return block_may_fallthru (ELSE_CLAUSE (stmt)); > --- gcc/cp/cp-gimplify.c.jj 2022-01-11 23:11:22.090294370 +0100 > +++ gcc/cp/cp-gimplify.c 2022-01-12 21:22:17.585212804 +0100 > @@ -166,8 +166,15 @@ genericize_if_stmt (tree *stmt_p) > can contain unfolded immediate function calls, we have to discard > the then_ block regardless of whether else_ has side-effects or not. */ > if (IF_STMT_CONSTEVAL_P (stmt)) > - stmt = build3 (COND_EXPR, void_type_node, boolean_false_node, > - void_node, else_); > + { > + if (block_may_fallthru (then_)) > + stmt = build3 (COND_EXPR, void_type_node, boolean_false_node, > + void_node, else_); > + else > + stmt = else_; > + } > + else if (IF_STMT_CONSTEXPR_P (stmt)) > + stmt = integer_nonzerop (cond) ? then_ : else_; > else > stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_); > protected_set_expr_location_if_unset (stmt, locus); > --- gcc/testsuite/g++.dg/warn/Wreturn-type-13.C.jj 2022-01-12 21:21:36.567794238 +0100 > +++ gcc/testsuite/g++.dg/warn/Wreturn-type-13.C 2022-01-12 21:20:48.487475787 +0100 > @@ -0,0 +1,35 @@ > +// PR c++/103991 > +// { dg-do compile { target c++17 } } > + > +struct S { ~S(); }; > +int > +foo () > +{ > + S s; > + if constexpr (true) > + return 0; > + else > + return 1; > +} // { dg-bogus "control reaches end of non-void function" } > + > +#if __cpp_if_consteval >= 202106L > +constexpr int > +bar () > +{ > + S s; > + if consteval > + { > + return 0; > + } > + else > + { > + return 1; > + } > +} // { dg-bogus "control reaches end of non-void function" } > + > +int > +baz () > +{ > + return bar (); > +} > +#endif > > Jakub >
On Thu, Jan 13, 2022 at 04:09:22PM -0500, Jason Merrill wrote: > > The changes done to genericize_if_stmt in order to improve > > -Wunreachable-code* warning (which Richi didn't actually commit > > for GCC 12) are I think fine for normal ifs, but for constexpr if > > and consteval if we have two competing warnings. > > The problem is that we replace the non-taken clause (then or else) > > with void_node and keep the if (cond) { something } else {} > > or if (cond) {} else { something }; in the IL. > > This helps -Wunreachable-code*, if something can't fallthru but the > > non-taken clause can, we don't warn about code after it because it > > is still (in theory) reachable. > > But if the non-taken branch can't fallthru, we can get false positive > > -Wreturn-type warnings (which are enabled by default) if there is > > nothing after the if and the taken branch can't fallthru either. > > Perhaps we should replace the non-taken clause with __builtin_unreachable() > instead of void_node? It depends. If the non-taken clause doesn't exist, is empty or otherwise can fallthru, then using void_node for it is what we want. If it exists and can't fallthru, then __builtin_unreachable() is one possibility, but for all purpose if (1) something else __builtin_unreachable(); is equivalent to genericization of it as something and if (0) __builtin_unreachable(); else something too. The main problem is what to do for the consteval if that throws away the non-taken clause too early, whether we can do block_may_fallthru already where we throw it away or not. If we can do that, we could as right now clear the non-taken clause if it can fallthru and otherwise either set some flag on the IF_STMT or set the non-taken clause to __builtin_unreachable or endless empty loop etc., ideally something as cheap as possible. > And/or block_may_fallthru could handle INTEGER_CST op0? That is what I'm doing for consteval if in the patch because the info whether the non-taken clause can fallthru is lost. We can't do that for normal if, because the non-taken clause could have labels in it to which something jumps. But, block_may_fallthru isn't actually what is used for the -Wreturn-type warning, I think we warn only at cfg creation. Jakub
On 1/13/22 16:23, Jakub Jelinek wrote: > On Thu, Jan 13, 2022 at 04:09:22PM -0500, Jason Merrill wrote: >>> The changes done to genericize_if_stmt in order to improve >>> -Wunreachable-code* warning (which Richi didn't actually commit >>> for GCC 12) are I think fine for normal ifs, but for constexpr if >>> and consteval if we have two competing warnings. >>> The problem is that we replace the non-taken clause (then or else) >>> with void_node and keep the if (cond) { something } else {} >>> or if (cond) {} else { something }; in the IL. >>> This helps -Wunreachable-code*, if something can't fallthru but the >>> non-taken clause can, we don't warn about code after it because it >>> is still (in theory) reachable. >>> But if the non-taken branch can't fallthru, we can get false positive >>> -Wreturn-type warnings (which are enabled by default) if there is >>> nothing after the if and the taken branch can't fallthru either. >> >> Perhaps we should replace the non-taken clause with __builtin_unreachable() >> instead of void_node? > > It depends. If the non-taken clause doesn't exist, is empty or otherwise > can fallthru, then using void_node for it is what we want. > If it exists and can't fallthru, then __builtin_unreachable() is one > possibility, but for all purpose > if (1) > something > else > __builtin_unreachable(); > is equivalent to genericization of it as > something > and > if (0) > __builtin_unreachable(); > else > something > too. > The main problem is what to do for the consteval if that throws away > the non-taken clause too early, whether we can do block_may_fallthru > already where we throw it away or not. If we can do that, we could > as right now clear the non-taken clause if it can fallthru and otherwise > either set some flag on the IF_STMT or set the non-taken clause to > __builtin_unreachable or endless empty loop etc., ideally something > as cheap as possible. > >> And/or block_may_fallthru could handle INTEGER_CST op0? > > That is what I'm doing for consteval if in the patch because the info > whether the non-taken clause can fallthru is lost. > We can't do that for normal if, because the non-taken clause could > have labels in it to which something jumps. > But, block_may_fallthru isn't actually what is used for the -Wreturn-type > warning, I think we warn only at cfg creation. Fair enough. The patch is OK. Jason
--- gcc/cp/cp-objcp-common.c.jj 2022-01-11 23:11:22.091294356 +0100 +++ gcc/cp/cp-objcp-common.c 2022-01-12 17:57:18.232202275 +0100 @@ -313,6 +313,13 @@ cxx_block_may_fallthru (const_tree stmt) return false; case IF_STMT: + if (IF_STMT_CONSTEXPR_P (stmt)) + { + if (integer_nonzerop (IF_COND (stmt))) + return block_may_fallthru (THEN_CLAUSE (stmt)); + if (integer_zerop (IF_COND (stmt))) + return block_may_fallthru (ELSE_CLAUSE (stmt)); + } if (block_may_fallthru (THEN_CLAUSE (stmt))) return true; return block_may_fallthru (ELSE_CLAUSE (stmt)); --- gcc/cp/cp-gimplify.c.jj 2022-01-11 23:11:22.090294370 +0100 +++ gcc/cp/cp-gimplify.c 2022-01-12 21:22:17.585212804 +0100 @@ -166,8 +166,15 @@ genericize_if_stmt (tree *stmt_p) can contain unfolded immediate function calls, we have to discard the then_ block regardless of whether else_ has side-effects or not. */ if (IF_STMT_CONSTEVAL_P (stmt)) - stmt = build3 (COND_EXPR, void_type_node, boolean_false_node, - void_node, else_); + { + if (block_may_fallthru (then_)) + stmt = build3 (COND_EXPR, void_type_node, boolean_false_node, + void_node, else_); + else + stmt = else_; + } + else if (IF_STMT_CONSTEXPR_P (stmt)) + stmt = integer_nonzerop (cond) ? then_ : else_; else stmt = build3 (COND_EXPR, void_type_node, cond, then_, else_); protected_set_expr_location_if_unset (stmt, locus); --- gcc/testsuite/g++.dg/warn/Wreturn-type-13.C.jj 2022-01-12 21:21:36.567794238 +0100 +++ gcc/testsuite/g++.dg/warn/Wreturn-type-13.C 2022-01-12 21:20:48.487475787 +0100 @@ -0,0 +1,35 @@ +// PR c++/103991 +// { dg-do compile { target c++17 } } + +struct S { ~S(); }; +int +foo () +{ + S s; + if constexpr (true) + return 0; + else + return 1; +} // { dg-bogus "control reaches end of non-void function" } + +#if __cpp_if_consteval >= 202106L +constexpr int +bar () +{ + S s; + if consteval + { + return 0; + } + else + { + return 1; + } +} // { dg-bogus "control reaches end of non-void function" } + +int +baz () +{ + return bar (); +} +#endif