svn commit: r358248 - head/sys/vm

[Dimitry Andric]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 223 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20200222/256142dd/attachment.sig>