From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x32b.google.com (mail-ot1-x32b.google.com [IPv6:2607:f8b0:4864:20::32b]) by sourceware.org (Postfix) with ESMTPS id CA7653858D3C for ; Tue, 30 Nov 2021 23:08:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CA7653858D3C Received: by mail-ot1-x32b.google.com with SMTP id h19-20020a9d3e53000000b0056547b797b2so32469157otg.4 for ; Tue, 30 Nov 2021 15:08:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=+onZLBLw+0a2c4wEQkerKXVx9zQ3ojTBxmMVALgC/g0=; b=RCKe3y/T8Uo7RkXfcBWDLUJKRh6JAA8filfFGQsyDuiWQwvxuYh9MIYPo+dGn4JStZ FGH38PCmI05tUA/SDVAjvbAlM+V8t1SS6pRHfkE9oKq/jqoGhfmo8747GlASDE6jwXFy wwbiZ1xnFjsuAuAuEgSHwf2SsuR1ODWTIZnPYWVI0OkkgxGd2WhaD8pzTPLBc0Pj7rBk qgjhaefZK6euM9osgJ89U0dE5nxeSd/JEQVkOquK9ghiMx57AYGv8Ot01dl9tnnj5M+K AOhH6JMzbDE7n+PO5nfzL4d/hrXE1z7hSjPl02HN1ohAg1XPiHp2lvpdvnyz7VLLDoRU 5I+A== X-Gm-Message-State: AOAM530PyrMyJXdAyOhi4ViLiCPRRCgYNRaGwWpKtGspF8TzftUCfSgW wddMUpCdo6C3lxgwM2Orhj8uKHHFSyE= X-Google-Smtp-Source: ABdhPJzRemheLcZxXdaFI/6O/Y/ObrrcVXkpZXxK7pYk+wXJvdh+Bwb0MRu3Z1AdbeYHHkbs03r3Dw== X-Received: by 2002:a9d:490e:: with SMTP id e14mr2131561otf.161.1638313694733; Tue, 30 Nov 2021 15:08:14 -0800 (PST) Received: from [192.168.0.41] (184-96-227-137.hlrn.qwest.net. [184.96.227.137]) by smtp.gmail.com with ESMTPSA id z20sm214385ote.28.2021.11.30.15.08.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Nov 2021 15:08:14 -0800 (PST) Subject: Re: [PATCH] Remove more stray returns and gcc_unreachable ()s To: Richard Biener Cc: gcc-patches@gcc.gnu.org References: <416nn390-9q5o-6sq0-4p83-nop7q87op6pr@fhfr.qr> <0e306fb4-1ec1-1c33-6a0d-6f0e359aee33@gmail.com> <46pr48os-7po3-rsq6-qs93-n0rsnrr4638@fhfr.qr> From: Martin Sebor Message-ID: <1bf1901e-43b0-8aba-f2b3-8671b98751e2@gmail.com> Date: Tue, 30 Nov 2021 16:08:13 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <46pr48os-7po3-rsq6-qs93-n0rsnrr4638@fhfr.qr> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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 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 23:08:17 -0000 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.) 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. Martin > > Richard. >