On 22/02/2020 14:37, Kyle Evans wrote:
On Sat, Feb 22, 2020 at 1:21 PM Pedro Giffuni <p...@freebsd.org> wrote:
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.
The current form of __unreachable is only half-useful and apparently
prone to misuse, as has been pointed out earlier in this thread,
without any form of detection that it's been misused. IMO this is of
limited utility, but the review I had included you on will turn it
into a panic under INVARIANTS or into the proper __builtin_unreachable
for stable/release branches (read as "sans debugging"). I also noted
in the review that we didn't have to turn __unreachable into this, but
I had just done so initially, though I'd debate that there's a better
name for what it currently does (e.g. __hint_unreachable) to make it
sound less like an assertion.
The current form of __unreachable() is exactly just the compiler
attribute on purpose. It indeed never got much use except for cleaning
some unreachable paths detected by Coverity.
D2536, which was the predecessor of the attribute hinted the idea of a
more metamorfic call in the lines of your proposal, which I don't think
I like but I can live with, as the current __unreachable seems to have
lived its original purpose.
Don't ask me to approve the differential though :)
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"
