From owner-svn-src-all@freebsd.org Sat Feb 22 19:38:03 2020 Return-Path: Delivered-To: svn-src-all@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 BD81024A782; Sat, 22 Feb 2020 19:38:03 +0000 (UTC) (envelope-from kevans@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 48PzBg3R7Hz4Ywp; Sat, 22 Feb 2020 19:38:03 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qk1-f173.google.com (mail-qk1-f173.google.com [209.85.222.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) (Authenticated sender: kevans) by smtp.freebsd.org (Postfix) with ESMTPSA id F10B470F7; Sat, 22 Feb 2020 19:38:02 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qk1-f173.google.com with SMTP id p7so5183760qkh.10; Sat, 22 Feb 2020 11:38:02 -0800 (PST) X-Gm-Message-State: APjAAAWTap0cMB9tRFZ4+eIlafzYD02YwiXil6cg2B7qlcdg1IM4y6dk uXw9P+/1bKvSqYMwKUqQUtStG0YmUSEw9giwy6A= X-Google-Smtp-Source: APXvYqzydW7bHeAsrYRx1j2jCD/gCpeSskALU9AGQa1nYSqFL3+yct1QsM4guFINbhKEx9re2SfKfzt0JYpgVXa2vz4= X-Received: by 2002:a05:620a:7f5:: with SMTP id k21mr15596758qkk.493.1582400282426; Sat, 22 Feb 2020 11:38:02 -0800 (PST) MIME-Version: 1.0 References: <202002221620.01MGK46E072303@repo.freebsd.org> <6D39FAD8-E581-42A8-97B4-EE63800D78A4@andric.com> In-Reply-To: From: Kyle Evans Date: Sat, 22 Feb 2020 13:37:51 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r358248 - head/sys/vm To: Pedro Giffuni Cc: Ian Lepore , Dimitry Andric , Mateusz Guzik , Kyle Evans , svn-src-head , svn-src-all , src-committers Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 22 Feb 2020 19:38:03 -0000 On Sat, Feb 22, 2020 at 1:21 PM Pedro Giffuni wrote: > > > 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. > The current form of __unreachable is only half-useful and apparently prone to misuse, as has been pointed out earlier in this thread, without any form of detection that it's been misused. IMO this is of limited utility, but the review I had included you on will turn it into a panic under INVARIANTS or into the proper __builtin_unreachable for stable/release branches (read as "sans debugging"). I also noted in the review that we didn't have to turn __unreachable into this, but I had just done so initially, though I'd debate that there's a better name for what it currently does (e.g. __hint_unreachable) to make it sound less like an assertion.