From owner-svn-src-head@freebsd.org Sat Feb 22 19:21:23 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 30CB2249E04; Sat, 22 Feb 2020 19:21:23 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 48PyqQ6rRWz4804; Sat, 22 Feb 2020 19:21:22 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from [192.168.0.3] (unknown [181.52.72.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: pfg) by smtp.freebsd.org (Postfix) with ESMTPSA id EB3C06E8E; Sat, 22 Feb 2020 19:21:21 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Subject: Re: svn commit: r358248 - head/sys/vm To: Ian Lepore , Dimitry Andric , Mateusz Guzik Cc: Kyle Evans , svn-src-head , svn-src-all , src-committers References: <202002221620.01MGK46E072303@repo.freebsd.org> <6D39FAD8-E581-42A8-97B4-EE63800D78A4@andric.com> From: Pedro Giffuni Organization: FreeBSD Message-ID: Date: Sat, 22 Feb 2020 14:21:22 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 22 Feb 2020 19:21:23 -0000 On 22/02/2020 14:13, Ian Lepore wrote: > On Sat, 2020-02-22 at 20:01 +0100, Dimitry Andric wrote: >> On 22 Feb 2020, at 17:44, Mateusz Guzik wrote: >>> On 2/22/20, Kyle Evans wrote: >>>> On Sat, Feb 22, 2020 at 10:25 AM Ian Lepore >>>> wrote: >>>>> On Sat, 2020-02-22 at 16:20 +0000, Kyle Evans wrote: >>>>>> Author: kevans >>>>>> Date: Sat Feb 22 16:20:04 2020 >>>>>> New Revision: 358248 >>>>>> URL: https://svnweb.freebsd.org/changeset/base/358248 >>>>>> >>>>>> Log: >>>>>> vm_radix: prefer __builtin_unreachable() to an unreachable >>>>>> panic() >>>>>> >>>>>> This provides the needed hint to GCC and offers an >>>>>> annotation for >>>>>> readers to >>>>>> observe that it's in-fact impossible to hit this point. >>>>>> We'll get hit >>>>>> with a >>>>>> a -Wswitch error if the enum applicable to the switch above >>>>>> were to >>>>>> get >>>>>> expanded without the new value(s) being handled. >>>>>> >>>>>> Modified: >>>>>> head/sys/vm/vm_radix.c >>>>>> >>>>>> Modified: head/sys/vm/vm_radix.c >>>>>> ============================================================= >>>>>> ================= >>>>>> --- head/sys/vm/vm_radix.c Sat Feb 22 13:23:27 >>>>>> 2020 (r358247) >>>>>> +++ head/sys/vm/vm_radix.c Sat Feb 22 16:20:04 >>>>>> 2020 (r358248) >>>>>> @@ -208,8 +208,7 @@ vm_radix_node_load(smrnode_t *p, enum >>>>>> vm_radix_access >>>>>> case SMR: >>>>>> return (smr_entered_load(p, vm_radix_smr)); >>>>>> } >>>>>> - /* This is unreachable, silence gcc. */ >>>>>> - panic("vm_radix_node_get: Unknown access type"); >>>>>> + __unreachable(); >>>>>> } >>>>>> >>>>>> static __inline void >>>>> What does __unreachable() do if the code ever becomes >>>>> reachable? Like >>>>> if a new enum value is added and this switch doesn't get >>>>> updated? >>>>> >>>> __unreachable doesn't help here, but the compiler will error out >>>> on >>>> the switch() if all enum values aren't addressed and there's no >>>> default: case. >>>> >>>> IMO, compilers could/should become smart enough to error if >>>> there's an >>>> explicit __builtin_unreachable() and they can trivially determine >>>> that >>>> all paths will terminate before this, independent of >>>> -Werror=switch*. >>>> _______________________________________________ >>> I think this is way too iffy, check this program: >>> >>> >>> #include >>> >>> int >>> main(void) >>> { >>> >>> __builtin_unreachable(); >>> printf("test\n"); >>> } >>> >>> Neither clang nor gcc warn about this and both stop code generation >>> past the statement. >> Indeed, that is exactly the intent. See: >> >> > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005funreachable >> "If control flow reaches the point of the __builtin_unreachable, the >> program is undefined. It is useful in situations where the compiler >> cannot deduce the unreachability of the code." >> >> E.g. this is *not* meant as a way to enforce the program to abort at >> runtime, if the supposedly unreachable part is actually reached. >> >> For this purpose, one should use an abort() or panic() function call, >> with such functions being annotated to never return. >> >> -Dimitry >> > The problem is, people will see usages such as what Kyle did, where the > code truly is unreachable (due to -Werror=switch), and not realizing > that's why it's valid there, they'll assume it's a type of assert- > unreachable and copy it/use it in other places as if that's what it was > for. > > So, IMO, using it should be exceedingly rare and there should be a > comment nearby about why it's valid in that context, or our > __unreachable cover for it should panic on INVARIANTS, as Kyle proposed > in an earlier reply. No __unreachable() as an attribute is meant as a hint for static checkers and compiler optimizations. If you are unsure and want a panic, you can add the panic message after the attribute. The compiler will then be free to optimize out the panic, but that was the idea anyways. Pedro.