Date: Sat, 22 Feb 2020 18:18:39 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Kyle Evans <kevans@freebsd.org> 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: <CAGudoHEGQcXc7sdmPbnUuWnQggBPKYCFmvtAAFZ5vDeFQVS_oQ@mail.gmail.com> In-Reply-To: <CACNAnaHV%2BXUFmsxt_ynmte6Qms%2BfSdaFh9wR9uadk4UQT8oBeQ@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> <CACNAnaHV%2BXUFmsxt_ynmte6Qms%2BfSdaFh9wR9uadk4UQT8oBeQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2/22/20, Kyle Evans <kevans@freebsd.org> wrote: > 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. > The keyword in the current lends itself towards misuse and according to my grep this is precisely what happened in the tree: ddb/db_expr.c: switch(t) { .. default: __unreachable(); } similarly in dev/amdtemp/amdtemp.c Another shady user is in dev/nvdimm/nvdimm.c -- read_label has few loops and the function ends with __unreachable() Seems to be in all these cases the intent was to "warn at compilation time if we ever get here and panic at runtime at least with debug" In contrast, the keyword is to explicitly tell the compiler that given piece of code will never be executed no matter it thinks. Thus the least which can be done is injecting a panic into it, according to the original review which added it https://reviews.freebsd.org/D2536 llvm developers do an equivalent (assert(0 && "blah")). -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHEGQcXc7sdmPbnUuWnQggBPKYCFmvtAAFZ5vDeFQVS_oQ>