svn commit: r358248 - head/sys/vm

[Mateusz Guzik]

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.

-- 
Mateusz Guzik <mjguzik gmail.com>