Date: Sat, 22 Feb 2020 12:13:09 -0700 From: Ian Lepore <ian@freebsd.org> To: Dimitry Andric <dimitry@andric.com>, Mateusz Guzik <mjguzik@gmail.com> Cc: Kyle Evans <kevans@freebsd.org>, svn-src-head <svn-src-head@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, src-committers <src-committers@freebsd.org> Subject: Re: svn commit: r358248 - head/sys/vm Message-ID: <b5a0d991f1d03e6cac2f7e0c1c54ac83124e8ca0.camel@freebsd.org> In-Reply-To: <6D39FAD8-E581-42A8-97B4-EE63800D78A4@andric.com> References: <202002221620.01MGK46E072303@repo.freebsd.org> <a3b2125de10d214d6e422d183f1fdc7e0e38e014.camel@freebsd.org> <CACNAnaHZnrqRv9J-B7XRCc7eN7Hkccf1R-7e36LiAXvZR4etVw@mail.gmail.com> <CAGudoHHg5R0zOc7RYge36roz%2B3C_sSRZcsyXC55W0yAyQpuuBA@mail.gmail.com> <6D39FAD8-E581-42A8-97B4-EE63800D78A4@andric.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 2020-02-22 at 20:01 +0100, Dimitry Andric wrote: > On 22 Feb 2020, at 17:44, Mateusz Guzik <mjguzik@gmail.com> wrote: > > > > On 2/22/20, Kyle Evans <kevans@freebsd.org> wrote: > > > On Sat, Feb 22, 2020 at 10:25 AM Ian Lepore <ian@freebsd.org> > > > 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 <stdio.h> > > > > 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. -- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?b5a0d991f1d03e6cac2f7e0c1c54ac83124e8ca0.camel>