svn commit: r358248 - head/sys/vm

[Kyle Evans]

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.