svn commit: r358248 - head/sys/vm

[Kyle Evans]

On Sat, Feb 22, 2020 at 11:18 AM Mateusz Guzik <mjguzik at gmail.com> wrote:
>
> On 2/22/20, Kyle Evans <kevans at freebsd.org> wrote:
> > On Sat, Feb 22, 2020 at 10:44 AM Mateusz Guzik <mjguzik at gmail.com> wrote:
> >>
> >> On 2/22/20, Kyle Evans <kevans at freebsd.org> wrote:
> >> > On Sat, Feb 22, 2020 at 10:25 AM Ian Lepore <ian at 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
>

The ddb one reads like crap at first, but the context does make it a
little better- within that context, it's easily proven that all of the
reachable cases are already handled. amdtemp looks more like an
unexpected bug waiting to happen.

> 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")).
>

I don't see much use in adding a panic for this specific call site -
it was deliberately structured for optimization, and the odds of it
getting refactored in a way that anything after the switch becomes
unreachable unintentionally are likely pretty slim.

However, especially given the other cases you've pointed out, I do see
the utility in general and I'm preparing a diff that will more
generally turn __unreachable into a panic under INVARIANTS instead of
dirtying up this particular call-site with the logic.

Thanks,

Kyle Evans