From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id D74BA3858C60 for ; Wed, 1 Dec 2021 08:05:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D74BA3858C60 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 55EBF1FD34; Wed, 1 Dec 2021 08:05:43 +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 39903A3B83; Wed, 1 Dec 2021 08:05:43 +0000 (UTC) Date: Wed, 1 Dec 2021 09:05:43 +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: <1bf1901e-43b0-8aba-f2b3-8671b98751e2@gmail.com> Message-ID: References: <416nn390-9q5o-6sq0-4p83-nop7q87op6pr@fhfr.qr> <0e306fb4-1ec1-1c33-6a0d-6f0e359aee33@gmail.com> <46pr48os-7po3-rsq6-qs93-n0rsnrr4638@fhfr.qr> <1bf1901e-43b0-8aba-f2b3-8671b98751e2@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: Wed, 01 Dec 2021 08:05:48 -0000 On Tue, 30 Nov 2021, Martin Sebor wrote: > On 11/30/21 12:51 AM, Richard Biener wrote: > > 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. > > How would such an attribute be different from > __builtin_unreachable? (By the way, there is or was a proposal > before WG14 to add an annotation like it: > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2816.pdf > If I recall, a function was preferred by more in a discussion > of the proposal than an attribute.) __builtin_unrechable () would be exactly such a thing. But gcc_unreachable () expands to fatal_error () which is marked noreturn and _not_ "unreachable". The idea was to add a wrapper that has the suggested new "unreachable" attribute on it. > I agree the cases above are not entirely clear but it occurs to > me that it's possible to discern at least two broad categories > of cases: 1) a statement made unreachable by a prior one with > the same effect where swapping the two wouldn't change anything > (the double return 1; above), and 2) an unreachable statement > (or a series of statements) with a different effect than > the prior one (the last three above). > > The set in (1) are completely innocuous and removing them might > considered just a matter of cleanup. Those in (2) are less > clear cut and more likely to harbor bugs and so when adopting > the warning in a code base like Binutils with lots of instances > of both kinds I'd expect to focus on (2) first and worry about > (1) later. > > Even within (2) there might be separable subsets, like a return > statement followed by a break in a switch (common in Binutils > and I think you also cleaned up some in GCC). In at least some > of these the return is hidden in a macro so the break after it > might serve as a visual clue that the case isn't meant to fall > through. This subset would be different from two apparently > contradictory return statements each with a different value, > or from a return followed by more than one statement. It might > make sense to treat these two classes separately (e.g., add > a level for them). > > But these are just ideas for heuristics based on my limited > insight, and YMMV. It's just food for thought. Agreed. I concentrated on getting the basics working - it would indeed be nice to suppress the 100% harmless cases, but it starts to look like GIMPLE lowering is already too late and too early at the same time to recover the important details. Like a 'break' is visible as 'goto' only and w/o a built CFG it's difficult to tell if it was a 'break' originally or if it was switch (i) { case 0: return 0; goto fail; ... } fail: return -1; since GENERIC doesn't have BREAK or CONTINUE stmts we'd have to annotate the GOTO_EXPR somehow to tell those apart. Richard.