On 22/02/2020 14:13, Ian Lepore wrote:
On Sat, 2020-02-22 at 20:01 +0100, Dimitry Andric wrote:
On 22 Feb 2020, at 17:44, Mateusz Guzik <mjgu...@gmail.com> wrote:
On 2/22/20, Kyle Evans <kev...@freebsd.org> wrote:
On Sat, Feb 22, 2020 at 10:25 AM Ian Lepore <i...@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.
No __unreachable() as an attribute is meant as a hint for static
checkers and compiler optimizations. If you are unsure and want a panic,
you can add the panic message after the attribute. The compiler will
then be free to optimize out the panic, but that was the idea anyways.
Pedro.
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
