Date: Sat, 22 Feb 2020 10:50:26 -0600 From: Kyle Evans <kevans@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Ian Lepore <ian@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: <CACNAnaHV%2BXUFmsxt_ynmte6Qms%2BfSdaFh9wR9uadk4UQT8oBeQ@mail.gmail.com> In-Reply-To: <CAGudoHHg5R0zOc7RYge36roz%2B3C_sSRZcsyXC55W0yAyQpuuBA@mail.gmail.com> References: <202002221620.01MGK46E072303@repo.freebsd.org> <a3b2125de10d214d6e422d183f1fdc7e0e38e014.camel@freebsd.org> <CACNAnaHZnrqRv9J-B7XRCc7eN7Hkccf1R-7e36LiAXvZR4etVw@mail.gmail.com> <CAGudoHHg5R0zOc7RYge36roz%2B3C_sSRZcsyXC55W0yAyQpuuBA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Feb 22, 2020 at 10:44 AM 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. Thus I think for production kernels __unreachable > can expand to to the builtin, but for debug it should be a panic with > func/file/line. This would work fine in terms of analysis since panic > is noreturn or so. I guess I'll repeat this again: our build will error out if this becomes reachable, because we compile with -Werror=switch. There's no point in having a panic that cannot physically be reached, you will never see the func/file/line.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaHV%2BXUFmsxt_ynmte6Qms%2BfSdaFh9wR9uadk4UQT8oBeQ>