From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id D66C23858402 for ; Tue, 30 Nov 2021 07:51:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D66C23858402 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id C1FBF2170E; Tue, 30 Nov 2021 07:51:20 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id A54E5A3B8C; Tue, 30 Nov 2021 07:51:20 +0000 (UTC) Date: Tue, 30 Nov 2021 08:51:20 +0100 (CET) From: Richard Biener To: Martin Sebor cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Remove more stray returns and gcc_unreachable ()s In-Reply-To: <0e306fb4-1ec1-1c33-6a0d-6f0e359aee33@gmail.com> Message-ID: <46pr48os-7po3-rsq6-qs93-n0rsnrr4638@fhfr.qr> References: <416nn390-9q5o-6sq0-4p83-nop7q87op6pr@fhfr.qr> <0e306fb4-1ec1-1c33-6a0d-6f0e359aee33@gmail.com> MIME-Version: 1.0 X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, 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 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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-List-Received-Date: Tue, 30 Nov 2021 07:51:23 -0000 On Mon, 29 Nov 2021, Martin Sebor wrote: > On 11/29/21 11:53 AM, Martin Sebor wrote: > > On 11/29/21 6:09 AM, Richard Biener via Gcc-patches wrote: > >> This removes more cases that appear when bootstrap with > >> -Wunreachable-code-return progresses. > >> > > ... > >> diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h > >> index 8ee0529d5a8..18e03c4cb96 100644 > >> --- a/gcc/sel-sched-ir.h > >> +++ b/gcc/sel-sched-ir.h > >> @@ -1493,8 +1493,6 @@ bb_next_bb (basic_block bb) > >>       default: > >>         return bb->next_bb; > >>       } > >> - > >> -  gcc_unreachable (); > >>   } > > > > Just skiming the changes out of curiosity, this one makes me > > wonder if the warning shouldn't be taught to avoid triggering > > on calls to __builtin_unreachable().  They can help make code > > more readable (e.g., after a case and switch statement that > > handles all values). > > I see someone else raised the same question in a patch I hadn't > gotten to yet: > > https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585431.html > > If you do end up removing the gcc_unreachable() calls, I would > suggest to replace them with a comment so as not to lose > the readability benefit. > > But I still wonder if it might make sense to teach the warning > not just about __builtin_unreachable() but also about noreturn > calls like abort() that (as you explained in the thread above) > gcc_unreachable() expands to. Is there a benefit to warning > on such calls? I'm not sure. I've chosen to eliminate only the "obvious" cases, like above where there's a default: that returns immediately visible (not always in the patch context). I've left some in the code base where that's not so obvious. IMHO making the flow obvious without a unreachable marker is superior to obfuscating it and clearing that up with one. Yes, I thought about not diagnosing things like return 1; return 1; but then what about return 1; return 0; ? I've seen cases like gcc_unreachable (); return 0; was that meant to be return 0; gcc_unreachable (); ? So it's not entirely clear. I think that if there was a way to denote definitive 'code should not reach here' function (a new attribute?) then it would make sense to not warn about control flow not reaching that. But then it would make sense to warn about stmts following such annotation. Richard.