svn commit: r358248 - head/sys/vm

[Ian Lepore]

On Sat, 2020-02-22 at 20:01 +0100, Dimitry Andric wrote:
> On 22 Feb 2020, at 17:44, 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.
> 
> 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